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] - Give the arrow function its proper name #1832

Closed
wants to merge 1 commit into from

Conversation

rumpl
Copy link
Contributor

@rumpl rumpl commented Feb 11, 2022

With this change an arrow function name is correctly set to the name of the variable:

const myFunction = () => {};
console.log(myFunction.name); // Prints "myFunction"

Note: I'm still getting familiar with the codebase and am pretty new to Rust so I won't be offended if this isn't merged. I am actually surprised I had to make so many changes to give the right code the name it needed. Maybe there is a better way? I'm all ears :)

An arrow function name should be the name of the variable:

const myFunction = () => {};
console.log(myFunction.name); // Prints "myFunction"
@rumpl
Copy link
Contributor Author

rumpl commented Feb 11, 2022

I force-pushed changes to the syntax/parser/function/tests.rs to make them pass

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #1832 (792873d) into main (7779c1a) will decrease coverage by 0.10%.
The diff coverage is 37.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1832      +/-   ##
==========================================
- Coverage   55.71%   55.61%   -0.11%     
==========================================
  Files         201      201              
  Lines       17326    17374      +48     
==========================================
+ Hits         9654     9663       +9     
- Misses       7672     7711      +39     
Impacted Files Coverage Δ
...syntax/parser/expression/assignment/conditional.rs 54.16% <0.00%> (-20.84%) ⬇️
boa/src/syntax/parser/function/mod.rs 45.28% <0.00%> (ø)
boa/src/syntax/parser/statement/mod.rs 36.16% <0.00%> (-1.01%) ⬇️
...src/syntax/parser/statement/declaration/lexical.rs 50.00% <7.69%> (-3.85%) ⬇️
boa/src/syntax/parser/statement/variable/mod.rs 58.90% <23.07%> (-7.25%) ⬇️
...ax/ast/node/declaration/arrow_function_decl/mod.rs 56.52% <33.33%> (-10.15%) ⬇️
boa/src/syntax/parser/expression/assignment/mod.rs 43.26% <44.44%> (+4.09%) ⬆️
...parser/expression/primary/array_initializer/mod.rs 69.56% <50.00%> (-3.17%) ⬇️
boa/src/syntax/parser/statement/expression/mod.rs 32.14% <50.00%> (ø)
boa/src/syntax/parser/statement/if_stm/mod.rs 57.77% <50.00%> (ø)
... and 18 more

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 7779c1a...792873d. Read the comment docs.

@jedel1043
Copy link
Member

Test262 conformance changes

VM implementation

Test result main count PR count difference
Total 87,912 87,912 0
Passed 40,892 40,898 +6
Ignored 19,929 19,929 0
Failed 27,091 27,085 -6
Panics 0 0 0
Conformance 46.51% 46.52% +0.01%
Fixed tests (6):
test/language/statements/variable/fn-name-arrow.js [strict mode] (previously Failed)
test/language/statements/variable/fn-name-arrow.js (previously Failed)
test/language/statements/let/fn-name-arrow.js [strict mode] (previously Failed)
test/language/statements/let/fn-name-arrow.js (previously Failed)
test/language/statements/const/fn-name-arrow.js [strict mode] (previously Failed)
test/language/statements/const/fn-name-arrow.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 very much for the contribution! Indeed, it's a lot of changes, I guess mostly because the name retrieval is done at parsing time instead of execution time.

I think this is good, it should be faster this way, and we have other examples where this is done this way too.

Thank you for the contribution! It looks pretty good to me!

@raskad raskad added ast Issue surrounding the abstract syntax tree enhancement New feature or request parser Issues surrounding the parser labels Feb 14, 2022
@raskad raskad added this to the v0.14.0 milestone Feb 14, 2022
@raskad
Copy link
Member

raskad commented Feb 14, 2022

bors r+

bors bot pushed a commit that referenced this pull request Feb 14, 2022
With this change an arrow function name is correctly set to the name of the variable:

```javascript
const myFunction = () => {};
console.log(myFunction.name); // Prints "myFunction"
```

_Note:_ I'm still getting familiar with the codebase and am pretty new to Rust so I won't be offended if this isn't merged. I am actually surprised I had to make so many changes to give the right code the name it needed. Maybe there is a better way? I'm all ears :)
@bors
Copy link

bors bot commented Feb 14, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Give the arrow function its proper name [Merged by Bors] - Give the arrow function its proper name Feb 14, 2022
@bors bors bot closed this Feb 14, 2022
@rumpl rumpl deleted the arrow-function-name branch February 15, 2022 08:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast Issue surrounding the abstract syntax tree enhancement New feature or request parser Issues surrounding the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants