Check regular expressions for compile errors #165
Conversation
There was a problem hiding this comment.
Looks good thanks, just some nits I'd prefer you fix but aren't actually important.
This is technically a breaking API change, but it is to a previously unused error, and searching all of Github brings up no uses of it, so I think it's fine to change without a major version bump.
Maybe just pinging @robinst since he's the only major non-open-source user I know.
src/parsing/yaml_load.rs
Outdated
| assert!(def.is_err()); | ||
| match def.unwrap_err() { | ||
| ParseSyntaxError::RegexCompileError(ref regex, _) => assert_eq!("[a", regex), | ||
| _ => assert!(false, "Got unexpedted ParseSyntaxError"), |
src/parsing/yaml_load.rs
Outdated
| regex_str = regex_str.replace(&format!("\\{}", i), "placeholder"); | ||
| } | ||
|
|
||
| match Regex::new(®ex_str) { |
There was a problem hiding this comment.
I think this may need to be a with_options with ONIG_OPTION_CAPTURE_GROUP like the other compiles in the library. From some Googling it appears this is the standard for textmate-like things and some syntaxes may rely on it.
There was a problem hiding this comment.
Oh, good point. I saw Regex::new somewhere and didn't check at the relevant places.
Oh, right - due to the
👍 Sounds good to me. |
src/parsing/yaml_load.rs
Outdated
| let mut regex_str = String::from(regex_str); | ||
| for i in 0..10 { | ||
| regex_str = regex_str.replace(&format!("\\{}", i), "placeholder"); | ||
| } |
There was a problem hiding this comment.
Can we make the regex_with_substitutes function a bit more abstract (or extract another function), so that we can use the same logic here?:
syntect/src/parsing/syntax_definition.rs
Line 214 in af810cd
This is necessary to handle double backslashes like \\1 properly. While I was noticing things I also made the placeholders nicer for error messages and used a capacity when building the substitute result.
|
@sharkdp I realized that @robinst's suggestion was important not just for cleanliness but also to handle the case of I pushed my changes to the branch, they look okay? @sharkdp @robinst |
|
Looks great! |
|
Thank you! Very much looking forward to the next release 😄. |
This PR implements a basic version of regex syntax checking as proposed in #98 by @robinst. This would hopefully allow us to catch a lot of possible errors at
SyntaxSet-generation time (and preventpanic!s at syntax-highlighting time).I have also implemented
ErrorforParseSyntaxError(which includesRegexCompileError). This allows user code to display nice error messages when loading syntax sets. For example, I get the following error when I try to loadCMake.sublime-syntax:(this has already been addressed upstream: zyxar/Sublime-CMakeLists#32)
Note that the tests currently do not pass due to #164 (I believe this is a good thing!), so #164 should be fixed before merging this. All tests pass if
Rd (R Documentation).sublime-syntaxis removed.The backreference "rewriting" is very immature (and maybe even "unsafe"?). I ended up using this simple search-and-replace after trying two other approaches first:
Simply ignore regex compile errors with code
-208(onig_sys::ONIGERR_INVALID_BACKREF).However, using this approach, we wouldn't catch other syntax errors in these regexes.
Use
onigsSyntaxBehaviorto disable backreference checking (because I thought thatSYNTAX_BEHAVIOR_STRICT_CHECK_BACKREF might help - it did not).