Repository navigation
Compiler should error when encountering invalid regular expressions #3432
Description
In tests/cases/conformance/parser/ecmascript5/RegressionTests/parser579071.ts we have an invalid regex:
var x = /fo(o/;This is not valid JavaScript, but we don't give any errors. It would be helpful to let users know when their regular expressions are invalid.
Activity
pygy commentedon May 4, 2022
I had missed this by only searching for
RegExp...I'm working on a Spec-based RegExp tokenizer written in JS, hopefully you'll be able to make use of it.
graphemecluster commentedon Jun 23, 2023
Moved from #54744:
Previously worked on #51837, I found that TypeScript gives almost no syntax errors for regular expressions. I would like to file a PR about it.
Something I would like to do are:
But this should be done only for RegExps with
uorvflag, i.e. inUnicodeMode, that means if we encounter auorvflag we will need to rescan the whole RegExp again (!!) (i.e. redoing what is done in the currentreScanSlashTokenmethod)And to check for invalid
DecimalEscapes and k<GroupName>s we will also need to count the number of capture groups and record the names of all named capture groups along the way.Am I doing too much or too less? I know doing too much may cause serious performace regressions (well, luckily regular expression literals are not that common compared with string literals). It should be better than doing nothing after all though.
RyanCavanaugh commentedon Jun 23, 2023
Imbalanced parens / bracket seems like where 99% of the value is.
Duplicate flags seems like an error no one's ever made before; usually its
/gor/m,/gmi, etc, I can't imagine writing/mgmunless a cat walked on my keyboard.Erroring on escapes that are invalid regardless of flags seems like a fine compromise.
IMO it's really actually fine if once a year your program unconditionally crashes on startup in the cases where you made an extremely rare mistake. The value is in flagging errors that are made every day.
graphemecluster commentedon Jun 25, 2023
Yup, duplicate flags checking is valueless 😅, but unknown seems worth a bit (like if someone accidentally typed the Cyrillic у instead of the Latin y for some reason with the "Editor > Unicode Highlight: Ambiguous Characters" config turned off or any of the "Editor > Unicode Highlight: Allowed Locales" permit the character). (Plus, there are no reasons not to show errors if the flag part isn’t really flags (like
foo_bar).) So far checking for flag availability according to target language version seems to be the most worthwhile part for flag check.I am still wondering if a full parse should be done. Doing that does affect performance, but it would be helpful to further TypeScript extensions like #41160. Of course, this requires sub-nodes to be added under
RegularExpressionLiteral. The actual implementation is not that tough, as we can take engine262 as a reference. (If we go with this, where should the class be put?scanner.ts?parser.ts? Or a new file?)graphemecluster commentedon Jun 27, 2023
Pinging Ryan Cavanaugh (@RyanCavanaugh) and Daniel Rosenwasser (@DanielRosenwasser) for opinions.
RyanCavanaugh commentedon Jun 27, 2023
I feel like we should be able to do a parse-only pass (i.e. just scan and descend in order to validate) of the regex without creating nodes as you go, since there's no consumer of that output, just the production of errors as a side effect
graphemecluster commentedon Jun 28, 2023
The output are useful to TypeScript API users for creating RegExp-related type utilities. We could make use of the parsed result to make methods like
String.prototype.replacesafer too.After all, I think we should at least store the number of capture groups and the names of the named captured groups.
RyanCavanaugh commentedon Jun 28, 2023
The downstream tools can re-parse if they really need that data.
We're very sensitive to perf papercuts and not likely to accept the feature if the perf cost is nontrivial, and allocating more objects is something that is likely to incur broad perf hits due to slowing down GC, etc..
zm-cttae commentedon Jul 4, 2023
Surely this is a case of attempting to reserialise the raw regexp to the string representation then into RegExp class? If it blows up, the string is invalid! That should be fast too.
zm-cttae commentedon Jul 5, 2023
I see Ryan has suggested the same thing but in CS speak 😛
RyanCavanaugh commentedon Jul 6, 2023
I actually hadn't thought of it in that much simpler way!
graphemecluster commentedon Jul 7, 2023
ZM (@zm-cttae) Ryan Cavanaugh (@RyanCavanaugh) There are already attempts like #4387 and #35957 using this approach but was closed. And this is definitely not a good solution, because:
Luckily a simple parser without node generation has little effect on the performance, and I plan to make my PR available at the end of this month.
Besides I have a follow-up proposal to further enforce type safety of RegExp-related methods and enhance UX (providing auto-completion) that make use of the implementation, but that would be another separate issue and PR afterwards.
zm-cttae commentedon Jul 7, 2023
To be fair there are only two types of error that can be assessed independently of each other - bad escapes, and invalid grouping
zm-cttae commentedon Jul 7, 2023
I agree however that the error isn't too useful for assessing what broke the regex in the first place.
Combine it with the platform-specificity problem and yes we will be needing a parser
graphemecluster commentedon Jul 31, 2023
I didn’t manage to finish the implementation within this month due to my work but it’s coming along nicely. I’ll be back on it shortly and make the PR available ASAP.