Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't Panic! #564

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Don't Panic! #564

wants to merge 7 commits into from

Conversation

Canop
Copy link
Contributor

@Canop Canop commented Jan 3, 2025

(in large friendly letters)

Syntect can panic when highlighting on a syntax file containing a regular expression with a feature not supported by the regex engine.

This isn't theoretical and leads to panics in programs using syntect:

With this fork, the behavior on a syntax having an invalid rule isn't to panic anymore, and is tuned with options:

let options = HighlightOptions {
    ignore_errors: true,
};
HighlightLines::new(syntax, theme, options)

With ignore_errors: false, a faulty regex in a syntax, detected when highlighting some lines, will result in an error.

With ignore_errors: true, the rule containing the faulty regex won't be applied but rest of the highlighting process will proceed.

So it never panics but the calling program can opt for the old behavior by specifying ignore_errors: false and panicking on a returned error.

When a syntax contains a regex which can't compile, highlight_lines can
either return an error or ignore the faulty rule, depending on an
option:

    let options = HighlightOptions {
        ignore_errors: true,
        ..Default::default()
    };
    HighlightLines::new(syntax, theme, options)
@Enselic
Copy link
Collaborator

Enselic commented Jan 3, 2025

Thanks for taking the time to create a PR. For the record I'm in favor of preventing panics. Any chance you can split this into smaller self-contained PRs for easier digestion and review? The smaller the better.

@Canop
Copy link
Contributor Author

Canop commented Jan 3, 2025

I don't think it can really get smaller. Changes are just consequences of regex::is_match and regex::search returning a Result rather than just a bool.

But if it's welcome (I'm also waiting for the bat people to check), I can make a clean PR without the changes in Cargo.toml.

@Canop Canop marked this pull request as ready for review January 3, 2025 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants