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

feat(lexer): support new RegExp Indices flag "d" #217

Merged
merged 1 commit into from
May 3, 2022
Merged

Conversation

3cp
Copy link
Member

@3cp 3cp commented May 1, 2022

Related to #214

@3cp 3cp requested a review from aladdin-add May 1, 2022 11:14
@3cp
Copy link
Member Author

3cp commented May 1, 2022

Not behind next since it's a stage 4 feature.

@@ -171,21 +173,19 @@ describe('Lexer - Regular expressions', () => {
t.throws(() => scanSingleToken(state, context, 0));
});
}
fail('fails on /i/ii', '/ ', Context.AllowRegExp);
Copy link
Member Author

@3cp 3cp May 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are lots of wrong message in this section. And there are many duplicated tests on /i/yy and /i/ss.

@aladdin-add aladdin-add merged commit b98e3bd into master May 3, 2022
@aladdin-add aladdin-add deleted the regex-indice branch May 3, 2022 01:10
@3cp
Copy link
Member Author

3cp commented May 3, 2022

I overlooked the validation result is used.

parser.tokenValue = validate(parser, pattern, flags);

So the tokenValue is wrong for regex with 'd' flag. Not sure what's the best solution. It might need a try catch to fall back to removing "d", so that at least for Nodejs v18, the tokenValue is correct.

@aladdin-add
Copy link
Collaborator

a straightforward way is to do a check in the entry, something like:

var isDsupported = true;
try {new Regexp('.', 'd');}catch(e){isDsupported = false;}

we may also want to check others like /s, /u, /y...

btw, I don't think it's a good resolution to just replace them to '', as they may be misused as normal values. e.g. /./d => /./. what do you think just return an exceptional value, e.g. null?

@fisker
Copy link
Collaborator

fisker commented May 3, 2022

Is this stored as RegExpLiteral value? It should be null per ESTree spec https://github.com/estree/estree/blob/master/es5.md#regexpliteral

@3cp
Copy link
Member Author

3cp commented May 3, 2022

Oh, sorry. I got it now.

@3cp
Copy link
Member Author

3cp commented May 3, 2022

What about

try {
  return new RegExp(pattern, flags);
} catch (e) {
  try {
    // Fallback to only validate patterns on systems 
    // where some flags are not supported.
    new RegExp(pattern);
    return null;
  } catch (e) {
    report(parser, Errors.UnterminatedRegExp);
  }
}

@aladdin-add
Copy link
Collaborator

it will allow unknown/dulp flags, e.g. /foo/a, /foo/gg?

@3cp
Copy link
Member Author

3cp commented May 3, 2022

Validation happens after the duplication check and the unknown check.

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.

None yet

3 participants