-
-
Notifications
You must be signed in to change notification settings - Fork 476
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(js_lexer): regex escape char #2044
Conversation
✅ Deploy Preview for biomejs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Nice! I would appreciate if we can add a test case for this, but the fix makes sense 👍 |
CodSpeed Performance ReportMerging #2044 will degrade performances by 18.31%Comparing Summary
Benchmarks breakdown
|
Lexer test cases can be added here, btw: https://github.com/biomejs/biome/blob/main/crates/biome_js_parser/src/lexer/tests.rs |
Could you add a changelog entry in EDIT: you can find some guidelines in our contributing guide. |
Yes I was hoping to add a test here, something like this: #[test]
fn issue_1941() {
assert_lex! {
r#"/\’/"#,
JS_REGEX_LITERAL:6
}
} But the lexer needs some context to read this as a regex, or it will produce |
I think |
Still slash and error token. Did I do something wrong? [
(
LET_KW,
0..3,
),
(
WHITESPACE,
3..4,
),
(
IDENT,
4..5,
),
(
WHITESPACE,
5..6,
),
(
EQ,
6..7,
),
(
WHITESPACE,
7..8,
),
(
SLASH,
8..9,
),
(
ERROR_TOKEN,
9..10,
),
(
ERROR_TOKEN,
10..13,
),
(
SLASH,
13..14,
),
] |
No, it's not your fault. It looks like the lexer cannot determine the presence of regexes on its own. It only happens after the parser tells it to "re-lex" certain nodes. So instead, I think you should add your test case here: https://github.com/biomejs/biome/blob/main/crates/biome_js_parser/test_data/inline/ok/literals.js |
@arendjr Yes here it works, thanks! |
837f47b
to
426af43
Compare
@Conaclos Changelog entry added. Should I add changelogs for other PRs (post v1.60) as well? Some are already merged. |
03b188e
to
426af43
Compare
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.
Looks good!
Might be better to do that in a separate PR, I think. |
Could you run the codegen one more time to update the changelog for the website? |
There're over 100 files modified after |
Thanks, I guess something was pushed to main without running the codegen. Still waiting for the CI, but I think it will pass 🤞 |
Thanks for the patch! |
Summary
After the lexer reads an escape char
\
in a regular expression, it should advance a byte or a char, so that valid regexes like/\’/
won't fail.Fixes #1941.
Test Plan
A test case is added in
crates/biome_js_parser/test_data/inline/ok/literals.js