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

Added undefined property to global scope #501

Merged
merged 22 commits into from
Jun 18, 2020
Merged

Conversation

croraf
Copy link
Contributor

@croraf croraf commented Jun 16, 2020

Please merge #488 first. Only the last commit is actually for this feature.

Should close #481 when completed

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #501 into master will increase coverage by 0.14%.
The diff coverage is 80.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #501      +/-   ##
==========================================
+ Coverage   67.48%   67.62%   +0.14%     
==========================================
  Files         162      165       +3     
  Lines        9869     9887      +18     
==========================================
+ Hits         6660     6686      +26     
+ Misses       3209     3201       -8     
Impacted Files Coverage Δ
boa/src/builtins/array/mod.rs 80.09% <0.00%> (-0.20%) ⬇️
boa/src/builtins/mod.rs 25.92% <0.00%> (-1.00%) ⬇️
boa/src/exec/mod.rs 64.70% <ø> (-0.13%) ⬇️
boa/src/syntax/parser/expression/primary/mod.rs 73.68% <ø> (-1.32%) ⬇️
boa/src/exec/identifier/mod.rs 66.66% <60.00%> (-22.23%) ⬇️
boa/src/builtins/undefined/mod.rs 66.66% <66.66%> (ø)
boa/src/builtins/undefined/tests.rs 100.00% <100.00%> (ø)
boa/src/environment/lexical_environment.rs 74.52% <100.00%> (+0.69%) ⬆️
boa/src/exec/operator/mod.rs 66.88% <100.00%> (+0.22%) ⬆️
boa/src/exec/operator/tests.rs 100.00% <100.00%> (ø)
... and 10 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 e674da4...50f1381. Read the comment docs.

@croraf
Copy link
Contributor Author

croraf commented Jun 16, 2020

I've made some changes on how get_binding_value works. I made it to return an Option instead of Value. This is important because we should not return Value::Undefined when the variable (that is node Identifier) is not found. Instead error should be explicitly signaled (for which I'm using Option(None)).

The actual JS error (ReferenceError) is thrown in the execution (run method) of the Identifier node.

There are some other places where get_binding_value is called for some reason. There I just added an unwrap as needed.

@croraf croraf marked this pull request as ready for review June 16, 2020 21:03
boa/src/exec/identifier/mod.rs Outdated Show resolved Hide resolved
boa/src/exec/operator/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Jun 17, 2020
@HalidOdat HalidOdat added this to the v0.9.0 milestone Jun 17, 2020
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 looking pretty good :) check the comments, it's almost ready!

boa/src/exec/identifier/mod.rs Outdated Show resolved Hide resolved
boa/src/exec/operator/mod.rs Outdated Show resolved Hide resolved
@croraf
Copy link
Contributor Author

croraf commented Jun 18, 2020

I've done the replacement. So you can merge if everything else is OK.

Not really sure why there exists the throw_reference_error" function at all? Should it be deleted?

@HalidOdat
Copy link
Member

Not really sure why there exists the throw_reference_error" function at all? Should it be deleted?

For a log time we only had the .throw_some_error which return a ResultValue which made throwing errors very easy in builtin functions, but a bit awkward for functions that return Result<NonValue, Value>, like to_number() which returns Result<f64, Value> we had to call unrechable! or use .expect("throw_type_error should always throw") which made this a bit convoluted.

So we added them in #419 to make it easier, we can use the .construct_some_error() in builtin functions, but we have to specify wether it we are throwing with Err() or returning an a Value with Ok(), so the .throw_some_error() are just a convenience functions.

Here we decided to add them: #419 (comment)

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 perfect to me :)

@HalidOdat HalidOdat changed the title [undefined] add undefined property to global scope Added undefined property to global scope Jun 18, 2020
@HalidOdat HalidOdat merged commit c19ef72 into boa-dev:master Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement global undefined property
3 participants