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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Rewrite scope analysis operations using visitors #2408

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Rewrite scope analysis operations using visitors #2408

wants to merge 5 commits into from

Conversation

jedel1043
Copy link
Member

@jedel1043 jedel1043 commented Nov 5, 2022 •
edited
Loading

This PR rewrites all syntax-directed operations that find declared names and variables using visitors.

Hopefully, this should be the last step before finally being able to separate the parser from the engine.

I checked the failing tests and they're apparently false positives, since they return Promise { <rejected> ReferenceError: x is not initialized } on the main branch.

@jedel1043 jedel1043 added technical debt parser Issues surrounding the parser labels Nov 5, 2022
@jedel1043 jedel1043 added this to the v0.17.0 milestone Nov 5, 2022
@github-actions
Copy link

github-actions bot commented Nov 5, 2022 •
edited
Loading

Test262 conformance changes

Test result main count PR count difference
Total 93,789 93,789 0
Passed 69,482 69,500 +18
Ignored 18,422 18,422 0
Failed 5,885 5,867 -18
Panics 0 0 0
Conformance 74.08% 74.10% +0.02%
Fixed tests (20): 
test/language/eval-code/indirect/lex-env-distinct-cls.js [strict mode] (previously Failed)
test/language/eval-code/indirect/lex-env-distinct-cls.js (previously Failed)
test/language/eval-code/indirect/var-env-global-lex-strict.js [strict mode] (previously Failed)
test/language/eval-code/indirect/var-env-global-lex-strict.js (previously Failed)
test/language/eval-code/indirect/lex-env-distinct-const.js [strict mode] (previously Failed)
test/language/eval-code/indirect/lex-env-distinct-const.js (previously Failed)
test/language/eval-code/indirect/lex-env-distinct-let.js [strict mode] (previously Failed)
test/language/eval-code/indirect/lex-env-distinct-let.js (previously Failed)
test/language/eval-code/direct/lex-env-distinct-cls.js [strict mode] (previously Failed)
test/language/eval-code/direct/lex-env-distinct-cls.js (previously Failed)
test/language/eval-code/direct/var-env-global-lex-strict-source.js [strict mode] (previously Failed)
test/language/eval-code/direct/var-env-global-lex-strict-source.js (previously Failed)
test/language/eval-code/direct/lex-env-distinct-const.js [strict mode] (previously Failed)
test/language/eval-code/direct/lex-env-distinct-const.js (previously Failed)
test/language/eval-code/direct/var-env-gloabl-lex-strict-caller.js [strict mode] (previously Failed)
test/language/eval-code/direct/lex-env-distinct-let.js [strict mode] (previously Failed)
test/language/eval-code/direct/lex-env-distinct-let.js (previously Failed)
test/language/statements/for-of/dstr/obj-prop-elem-target-yield-expr.js [strict mode] (previously Failed)
test/language/statements/for-of/dstr/obj-prop-elem-target-yield-expr.js (previously Failed)
test/language/statements/for-await-of/async-func-decl-dstr-obj-prop-elem-target-yield-ident-valid.js (previously Failed)
Broken tests (2): 
test/language/statements/for-await-of/async-gen-decl-dstr-obj-prop-elem-target-yield-expr.js [strict mode] (previously Passed)
test/language/statements/for-await-of/async-gen-decl-dstr-obj-prop-elem-target-yield-expr.js (previously Passed)

@codecov
Copy link

codecov bot commented Nov 5, 2022 •
edited
Loading

Codecov Report

Merging #2408 (12c9283) into main (aad7815) will decrease coverage by 0.42%.
The diff coverage is 25.95%.

@@            Coverage Diff             @@
##             main    #2408      +/-   ##
==========================================
- Coverage   38.83%   38.41%   -0.43%     
==========================================
  Files         313      313              
  Lines       23863    23974     +111     
==========================================
- Hits         9267     9209      -58     
- Misses      14596    14765     +169
Impacted Files Coverage Δ
boa_ast/src/declaration/mod.rs 44.00% <ø> (-8.95%) ⬇️
boa_ast/src/declaration/variable.rs 46.47% <ø> (+1.14%) ⬆️
boa_ast/src/expression/mod.rs 29.06% <ø> (-1.17%) ⬇️
boa_ast/src/function/parameters.rs 66.66% <ø> (ø)
boa_ast/src/pattern.rs 28.11% <0.00%> (+2.64%) ⬆️
boa_ast/src/statement/block.rs 35.71% <ø> (-1.79%) ⬇️
boa_ast/src/statement/iteration/for_loop.rs 28.57% <ø> (-2.20%) ⬇️
boa_ast/src/statement/iteration/for_of_loop.rs 33.33% <0.00%> (ø)
boa_ast/src/statement/iteration/mod.rs 9.09% <ø> (-2.91%) ⬇️
boa_ast/src/statement/mod.rs 34.78% <ø> (-12.53%) ⬇️
... and 84 more

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

@jedel1043 jedel1043 force-pushed the scope-analysis-visit branch from da20adc to e03e9dd Compare November 5, 2022 17:20
@jedel1043 jedel1043 mentioned this pull request Nov 5, 2022
Closed
@jedel1043
Copy link
Member Author

@Razican Applied your code suggestions here. Let me know if I missed something.

Razican
Razican approved these changes Nov 5, 2022
View reviewed changes
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.

LGTM :)

@Razican
Copy link
Member

Razican commented Nov 5, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 5, 2022
@jedel1043
Rewrite scope analysis operations using visitors (#2408)
8e14d76 
This PR rewrites all syntax-directed operations that find declared names and variables using visitors.

Hopefully, this should be the last step before finally being able to separate the parser from the engine.

I checked the failing [tests](https://github.com/tc39/test262/blob/85373b4ce12a908f8fc517093d95cf2ed2f5ee6a/test/language/statements/for-await-of/async-gen-decl-dstr-obj-prop-elem-target-yield-expr.js#L49) and they're apparently false positives, since they return `Promise { <rejected> ReferenceError: x is not initialized }` on the main branch.
@bors
Copy link

bors bot commented Nov 5, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Rewrite scope analysis operations using visitors [Merged by Bors] - Rewrite scope analysis operations using visitors Nov 5, 2022
@bors bors bot closed this Nov 5, 2022
@bors bors bot deleted the scope-analysis-visit branch November 5, 2022 22:03
bors bot pushed a commit that referenced this pull request Nov 6, 2022
@jedel1043
Extract the parser into a crate (#2409)
b88736a 
Depends on #2408.

This finally separates parsing from execution, which should make compilations a bit faster.
@RageKnify RageKnify added the Internal Category for changelog label Nov 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RageKnify RageKnify RageKnify approved these changes

@Razican Razican Razican approved these changes

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@HalidOdat HalidOdat Awaiting requested review from HalidOdat

@raskad raskad Awaiting requested review from raskad

Assignees
No one assigned
Labels
Internal Category for changelog parser Issues surrounding the parser technical debt
Projects
None yet
Milestone
v0.17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jedel1043 @Razican @RageKnify