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

[Merged by Bors] - Allow some keywords as identifiers #2269

Closed
wants to merge 6 commits into from

Conversation

tunz
Copy link
Contributor

@tunz tunz commented Sep 8, 2022

This PR fixes #2275

There are keywords that are allowed as identifiers.
https://tc39.es/ecma262/#sec-keywords-and-reserved-words

Those that are always allowed as identifiers, but also appear as keywords within certain syntactic productions, at places where Identifier is not allowed: as, async, from, get, meta, of, set, and target.

This PR adds test cases for them, and fixes some cases such as
class A { set(a, b) { } }
function of() { }
let obj = {async: true}
async()

@tunz tunz changed the title Allow non-reserved-keywords as identifiers Allow some keywords as identifiers Sep 8, 2022
@jedel1043 jedel1043 added technical debt parser Issues surrounding the parser execution Issues or PRs related to code execution labels Sep 8, 2022
@jedel1043 jedel1043 requested review from raskad and Razican and removed request for raskad September 8, 2022 05:33
@jedel1043 jedel1043 added this to the v0.16.0 milestone Sep 8, 2022
@jedel1043 jedel1043 requested a review from raskad September 8, 2022 05:34
@codecov
Copy link

codecov bot commented Sep 8, 2022

Codecov Report

Merging #2269 (d858f17) into main (90ec460) will increase coverage by 0.12%.
The diff coverage is 29.11%.

@@            Coverage Diff             @@
##             main    #2269      +/-   ##
==========================================
+ Coverage   41.35%   41.48%   +0.12%     
==========================================
  Files         234      234              
  Lines       22019    21987      -32     
==========================================
+ Hits         9107     9121      +14     
+ Misses      12912    12866      -46     
Impacted Files Coverage Δ
...syntax/parser/statement/iteration/for_statement.rs 37.15% <0.00%> (-2.38%) ⬇️
boa_interner/src/sym.rs 0.00% <ø> (ø)
boa_engine/src/syntax/parser/statement/mod.rs 29.51% <12.50%> (-0.77%) ⬇️
...rser/expression/primary/function_expression/mod.rs 40.90% <20.00%> (-0.56%) ⬇️
...engine/src/syntax/parser/expression/primary/mod.rs 35.40% <25.00%> (-1.39%) ⬇️
.../statement/declaration/hoistable/class_decl/mod.rs 27.25% <40.00%> (+3.33%) ⬆️
...arser/expression/primary/object_initializer/mod.rs 42.44% <50.00%> (+0.64%) ⬆️
...engine/src/syntax/parser/expression/identifiers.rs 33.70% <100.00%> (+3.11%) ⬆️
boa_engine/src/syntax/lexer/template.rs 36.95% <0.00%> (-2.63%) ⬇️
boa_engine/src/builtins/function/mod.rs 22.91% <0.00%> (-0.80%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Razican
Copy link
Member

Razican commented Sep 8, 2022

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 91,733 91,733 0
Passed 64,894 64,910 +16
Ignored 16,580 16,580 0
Failed 10,259 10,243 -16
Panics 0 0 0
Conformance 70.74% 70.76% +0.02%
Fixed tests (16):
test/built-ins/TypedArray/of/invoked-as-func.js [strict mode] (previously Failed)
test/built-ins/TypedArray/of/invoked-as-func.js (previously Failed)
test/built-ins/TypedArrayConstructors/of/invoked-as-func.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/of/invoked-as-func.js (previously Failed)
test/built-ins/TypedArrayConstructors/of/BigInt/invoked-as-func.js [strict mode] (previously Failed)
test/built-ins/TypedArrayConstructors/of/BigInt/invoked-as-func.js (previously Failed)
test/language/statements/for-of/head-lhs-async-escaped.js [strict mode] (previously Failed)
test/language/statements/for-of/head-lhs-async-escaped.js (previously Failed)
test/language/statements/for-of/head-lhs-async-parens.js [strict mode] (previously Failed)
test/language/statements/for-of/head-lhs-async-parens.js (previously Failed)
test/language/expressions/division/no-magic-asi.js [strict mode] (previously Failed)
test/language/expressions/division/no-magic-asi.js (previously Failed)
test/language/expressions/division/no-magic-asi-from-block-eval.js [strict mode] (previously Failed)
test/language/expressions/division/no-magic-asi-from-block-eval.js (previously Failed)
test/language/expressions/async-arrow-function/async-lineterminator-identifier-throws.js [strict mode] (previously Failed)
test/language/expressions/async-arrow-function/async-lineterminator-identifier-throws.js (previously Failed)

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.

Thank you for the fix!

I added some comments for potential enhancements. It seems that this test is failing, though:
https://github.com/tc39/test262/blob/f1870753fa616261193b99f79d996889921b3404/test/language/statements/for-of/head-lhs-async-invalid.js

It seems that the async cannot be placed in a for-of. The spec for this is here. It seems it requires a var, a let or a const for it to be valid. This might require changes in the for-of parser.

boa_engine/src/syntax/parser/cursor/mod.rs Outdated Show resolved Hide resolved
boa_engine/src/syntax/parser/expression/primary/mod.rs Outdated Show resolved Hide resolved
@Razican
Copy link
Member

Razican commented Sep 9, 2022

It seems that the fix for the for...off has broken these tests, that were passing with your previous version:

It turns out that the async keyword can be valid in a for...of but only if it's either surrounded with parenthesis or has escaped characters. The "escape characters" information is surely at the lexing level, but we might be missing it, I'm not sure though where is the parenthesis information getting lost.

It's not that critical, since this was broken before, but if you can find a solution that would be perfect :)

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.

It's ready to be merged from my side :) thanks!

@Razican Razican requested a review from raskad September 10, 2022 21:55
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Great work!

@raskad
Copy link
Member

raskad commented Sep 10, 2022

bors r+

bors bot pushed a commit that referenced this pull request Sep 10, 2022
This PR fixes #2275 

There are keywords that are allowed as identifiers.
https://tc39.es/ecma262/#sec-keywords-and-reserved-words
> Those that are always allowed as identifiers, but also appear as keywords within certain syntactic productions, at places where Identifier is not allowed: as, async, from, get, meta, of, set, and target.

This PR adds test cases for them, and fixes some cases such as
`class A { set(a, b) { } }`
`function of() { }`
`let obj = {async: true}`
`async()`
@bors
Copy link

bors bot commented Sep 10, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Allow some keywords as identifiers [Merged by Bors] - Allow some keywords as identifiers Sep 10, 2022
@bors bors bot closed this Sep 10, 2022
@tunz tunz deleted the tunz/non-reserved-id branch September 12, 2022 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
execution Issues or PRs related to code execution parser Issues surrounding the parser technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use "async" as a function identifier.
4 participants