-
Notifications
You must be signed in to change notification settings - Fork 47
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
Fix bugs when parsing regex #458
Conversation
/** | ||
* The tokens that might preceed a regex literal | ||
*/ | ||
export const PreceedingRegexTypes = new Set([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the full list of tokens that can come before a regex literal. If it's not in this list, then a regex literal will not be tokenized even if we encounter a forward slash. I find it safer to create an allowlist rather than a denylist, as it's easier to test, and we won't accidentally break functionality when we forgot to put something in the denylist, whereas only new adopters of regexp literals would encounter issues when something is missing from the allowlist.
//consume all flag-like chars (let the parser validate the actual values) | ||
while (/[a-z]/i.exec(this.peek())) { | ||
//preceeded by an allowed token, or if there are no previous tokens (i.e. this is the first token in the file). | ||
if (PreceedingRegexTypes.has(previousKind) || !previousKind) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the core of the change. Check for only an approved token preceeding the opening slash.
f4abf16
to
f62ee46
Compare
I believe the RegexLiteral check is being done in Brightscript (.BRS) mode? If it is, I don't think it should be |
Oops! You're right. Good catch. |
@markwpearce I addressed your concern in 57cca5d |
Resolves #455