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] - Replace contains and friends with visitors #2403

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Replace contains and friends with visitors #2403

wants to merge 2 commits into from

Conversation

jedel1043
Copy link
Member

This Pull Request replaces contains, contains_arguments, has_direct_super and function_contains_super with visitors. (~1000 removed lines!)

Also, the new visitor implementation caught a bug where we weren't setting the home object of async functions, generators and async generators for methods of classes, which caused a stack overflow on super calls, and I think that's pretty cool!

Next is var_declared_names, lexically_declared_names and friends, which will be on another PR.

@jedel1043 jedel1043 added ast Issue surrounding the abstract syntax tree Internal Category for changelog labels Nov 3, 2022
@jedel1043 jedel1043 added this to the v0.17.0 milestone Nov 3, 2022
@github-actions
Copy link

github-actions bot commented Nov 3, 2022

Test262 conformance changes

Test result main count PR count difference
Total 93,789 93,789 0
Passed 69,316 69,328 +12
Ignored 18,352 18,352 0
Failed 6,121 6,109 -12
Panics 0 0 0
Conformance 73.91% 73.92% +0.01%
Fixed tests (12): 
test/language/statements/class/definition/methods-async-super-call-param.js [strict mode] (previously Failed)
test/language/statements/class/definition/methods-async-super-call-param.js (previously Failed)
test/language/statements/class/definition/methods-async-super-call-body.js [strict mode] (previously Failed)
test/language/statements/class/definition/methods-async-super-call-body.js (previously Failed)
test/language/expressions/object/method-definition/generator-super-prop-param.js [strict mode] (previously Failed)
test/language/expressions/object/method-definition/generator-super-prop-param.js (previously Failed)
test/language/expressions/object/method-definition/async-super-call-param.js [strict mode] (previously Failed)
test/language/expressions/object/method-definition/async-super-call-param.js (previously Failed)
test/language/expressions/object/method-definition/async-super-call-body.js [strict mode] (previously Failed)
test/language/expressions/object/method-definition/async-super-call-body.js (previously Failed)
test/language/expressions/object/method-definition/generator-super-prop-body.js [strict mode] (previously Failed)
test/language/expressions/object/method-definition/generator-super-prop-body.js (previously Failed)

@codecov
Copy link

codecov bot commented Nov 3, 2022 •
edited
Loading

Codecov Report

Merging #2403 (7e9c5a6) into main (b4da172) will increase coverage by 0.78%.
The diff coverage is 46.55%.

@@            Coverage Diff             @@
##             main    #2403      +/-   ##
==========================================
+ Coverage   38.29%   39.07%   +0.78%     
==========================================
  Files         310      311       +1     
  Lines       24290    23669     -621     
==========================================
- Hits         9301     9248      -53     
+ Misses      14989    14421     -568
Impacted Files Coverage Δ
boa_ast/src/declaration/mod.rs 52.94% <ø> (-2.17%) ⬇️
boa_ast/src/declaration/variable.rs 47.36% <ø> (+8.42%) ⬆️
boa_ast/src/expression/access.rs 26.92% <ø> (+9.27%) ⬆️
boa_ast/src/expression/await.rs 36.36% <ø> (+22.07%) ⬆️
boa_ast/src/expression/call.rs 36.11% <ø> (+14.15%) ⬆️
boa_ast/src/expression/literal/array.rs 19.44% <ø> (+2.54%) ⬆️
boa_ast/src/expression/literal/object.rs 19.41% <ø> (-1.31%) ⬇️
boa_ast/src/expression/literal/template.rs 9.67% <ø> (+4.27%) ⬆️
boa_ast/src/expression/mod.rs 30.12% <ø> (+11.63%) ⬆️
boa_ast/src/expression/new.rs 23.07% <ø> (+10.57%) ⬆️
... and 231 more

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

Razican
Razican reviewed Nov 3, 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.

Looks very good! I added some comments where needed. I also liked the fix with the leading_ones() function and the fix to the bug you found.

boa_ast/src/operations.rs Outdated Show resolved Hide resolved
boa_ast/src/operations.rs Show resolved Hide resolved
boa_ast/src/operations.rs Show resolved Hide resolved
boa_ast/src/operations.rs Outdated Show resolved Hide resolved
boa_ast/src/statement/mod.rs Outdated Show resolved Hide resolved
@jedel1043 jedel1043 requested a review from Razican November 3, 2022 21:06
Razican
Razican approved these changes Nov 3, 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.

Thanks! LGTM!

raskad
raskad approved these changes Nov 3, 2022
View reviewed changes
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 stuff. Having all of the logic in one place is really nice.

@raskad
Copy link
Member

raskad commented Nov 3, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 3, 2022
@jedel1043
Replace contains and friends with visitors (#2403)
49a5867 
This Pull Request replaces `contains`, `contains_arguments`, `has_direct_super` and `function_contains_super` with visitors. (~1000 removed lines!)

Also, the new visitor implementation caught a bug where we weren't setting the home object of async functions, generators and async generators for methods of classes, which caused a stack overflow on `super` calls, and I think that's pretty cool!

Next is `var_declared_names`, `lexically_declared_names` and friends, which will be on another PR.
@bors
Copy link

bors bot commented Nov 3, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Replace contains and friends with visitors [Merged by Bors] - Replace contains and friends with visitors Nov 3, 2022
@bors bors bot closed this Nov 3, 2022
@bors bors bot deleted the ast-operations branch November 3, 2022 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raskad raskad raskad approved these changes

@Razican Razican Razican approved these changes

@jasonwilliams jasonwilliams Awaiting requested review from jasonwilliams

@HalidOdat HalidOdat Awaiting requested review from HalidOdat

@RageKnify RageKnify Awaiting requested review from RageKnify

Assignees
No one assigned
Labels
ast Issue surrounding the abstract syntax tree Internal Category for changelog
Projects
None yet
Milestone
v0.17.0
Development

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