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.

Sign up for GitHub

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

Jump to bottom

panic with regex-fancy feature on one line JS files #3156

Open
Canop opened this issue Dec 30, 2024 · 7 comments
Open

panic with regex-fancy feature on one line JS files #3156

Canop opened this issue Dec 30, 2024 · 7 comments
Labels
bug Something isn't working

Comments

@Canop
Copy link

Canop commented Dec 30, 2024

With current main (commit 3e07483):

echo "let a = 0;" > crash.js
cargo run --no-default-features --features "clap,etcetera,paging,wild,regex-fancy" -- crash.js

results in

thread 'main' panicked at /home/dys/.cargo/registry/src/index.crates.io-6f17d22bba15001f/syntect-5.2.0/src/parsing/regex.rs:70:53:
regex string should be pre-tested: ParseError(147, InvalidEscape("\\g"))

It may be due to https://github.com/sublimehq/Packages/blob/master/JavaScript/Indentation%20Rules.tmPreferences#L73

The reason I want to use regex-fancy is that the onig crate fails to build with gcc 15.

Background: I'm the author of another program using syntect, I have the same problem and just verified it was also happening in bat. A solution should apply to both bat and broot.
@Canop Canop added the bug Something isn't working label Dec 30, 2024
@Canop Canop mentioned this issue Dec 30, 2024
Merged
Canop added a commit to Canop/broot that referenced this issue Jan 1, 2025
@Canop
Use a new version of the forked syntect (#968)
e516208 
This fork doesn't panic on non compiling regexes.

Fix #967

The reason the fork is needed is that syntect panics when a regex from a syntax can't compile, which happens with sublime syntaxes when the engine isn't onig but fancy-regex.

The default regex engine of syntect is onig, but this unmaintained engine isn't pure-rust and isn't compatible with gcc 15, which limits compatibility for broot. So, right now, this fork is useful to prevent crashes.

The ideal solution would be either to have syntect completely handle regex compiling errors without panic (which looks easily doable but requires some work and may not appear as the best behavior for some other crates using syntect) and|or to have fancy-regex support all the regex features that are required by sublime syntaxes.

See also:
* fancy-regex/fancy-regex#74
* sharkdp/bat#3156 
* #956
* trishume/syntect#508
@Canop
Copy link
Author

Canop commented Jan 1, 2025

The solution I used in broot was to use a forked version of syntect to prevent panics (and not applying not computable rules).

A more general solution might be to do a PR for syntect, adding a no-panic mode. I don't know if that would be of interest for bat.

@keith-hall
Copy link
Collaborator

I believe it would be useful for bat. Better to have slightly wrong highlighting than panicking halfway through I think 🙂

@Canop
Copy link
Author

Canop commented Jan 1, 2025

I believe it would be useful for bat. Better to have slightly wrong highlighting than panicking halfway through I think 🙂

So I'll try to propose a clean PR to syntect.

@Canop Canop mentioned this issue Jan 3, 2025
Open
@Canop
Copy link
Author

Canop commented Jan 3, 2025

@keith-hall Can you please have a look at trishume/syntect#564 and give me your opinion ?

This is the exact code of the fork used in broot current's main, avoiding runtime panics.

@keith-hall
Copy link
Collaborator

Seems reasonable to me :) btw I tracked the incompatible regex pattern down to https://github.com/babel/babel-sublime/blob/f4579f9107996c16208466248a85dc2296083a5f/JavaScript%20(Babel).YAML-tmLanguage#L357

@Canop
Copy link
Author

Canop commented Jan 7, 2025

@keith-hall Any idea to unblock this PR ?

@keith-hall
Copy link
Collaborator

I guess it makes sense to get CI passing as much as possible - the bat regression tests step is expected to fail due to the public API changing, so I think either we ignore it or (temporarily) point it to a branch of bat which would compile against the new version of syntect. As there is likely no way to make the PR smaller, I think it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Canop @keith-hall