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

Support Unicode escape in identifier names #1102

Merged
merged 1 commit into from
May 22, 2021

Conversation

jevancc
Copy link
Contributor

@jevancc jevancc commented Jan 27, 2021

This Pull Request add the support of Unicode escapes (\uXXXX and \u{ cp }) in identifier names. An example for this is:

let \u0078 = 10;
console.log(x); // x = 10

It changes the following:

  • Update identifier lexer
  • Throw syntax error when there are Unicode escapes in keyword
  • Add True, False, Null to keywords (used only in lexing)
  • Add unit tests

Some methods in lexer/cursor.rs are not used after this PR. Instead of removing them from the source code, I kept them for future use. The #[allow(dead_code)] is added to prevent clippy error.

@codecov
Copy link

codecov bot commented Jan 27, 2021

Codecov Report

Merging #1102 (8b50842) into master (038acb4) will decrease coverage by 0.03%.
The diff coverage is 53.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1102      +/-   ##
==========================================
- Coverage   58.81%   58.77%   -0.04%     
==========================================
  Files         176      176              
  Lines       12467    12501      +34     
==========================================
+ Hits         7332     7347      +15     
- Misses       5135     5154      +19     
Impacted Files Coverage Δ
boa/src/syntax/lexer/cursor.rs 70.24% <ø> (-0.98%) ⬇️
boa/src/syntax/ast/keyword.rs 47.57% <50.00%> (+0.15%) ⬆️
boa/src/syntax/lexer/mod.rs 66.08% <50.00%> (-1.17%) ⬇️
boa/src/syntax/lexer/identifier.rs 55.38% <54.00%> (-6.16%) ⬇️
boa/src/syntax/lexer/string.rs 66.10% <0.00%> (+1.69%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 038acb4...8b50842. Read the comment docs.

@jevancc jevancc force-pushed the unicode-escape-identifier branch from c98ef89 to 8b50842 Compare February 1, 2021 18:38
@jevancc jevancc marked this pull request as ready for review February 1, 2021 19:18
@RageKnify RageKnify added the lexer Issues surrounding the lexer label Feb 3, 2021
@RageKnify RageKnify added this to the v0.12.0 milestone Feb 3, 2021
@jevancc
Copy link
Contributor Author

jevancc commented Feb 5, 2021

This PR is ready for review. The bug related to Unicode escapes in reserved word as property is mentioned in #1117 and I prefer to address it separately. Let me know if you have any comments and suggestions.

@RageKnify RageKnify requested a review from tofpie February 9, 2021 02:13
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks good, just a comment for my understanding on dead code.

@@ -130,6 +130,7 @@ where
/// predicate on the ascii char
///
/// The buffer is not incremented.
#[allow(dead_code)]
Copy link
Member

Choose a reason for hiding this comment

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

Why do we allow dead code here? If it's not being used, it should be removed, right?

Same applies to line 195.

@Razican
Copy link
Member

Razican commented May 10, 2021

Would it be possible to re-base it to see conformance / benchmark changes?

@Razican
Copy link
Member

Razican commented May 22, 2021

I ran this locally, and got these results:

Test262 conformance changes:

Test result master count PR count difference
Total 78,701 78,701 0
Passed 26,400 26,530 +130
Ignored 15,519 15,519 0
Failed 36,782 36,652 -130
Panics 26 27 +1
Conformance 33.54% 33.71% +0.17%

@Razican Razican merged commit 08f232f into boa-dev:master May 22, 2021
Razican pushed a commit that referenced this pull request May 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lexer Issues surrounding the lexer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants