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

[ReferenceError] complete solution #488

Merged
merged 11 commits into from
Jun 16, 2020
Merged

Conversation

croraf
Copy link
Contributor

@croraf croraf commented Jun 12, 2020

@Razican Razican added bug Something isn't working execution Issues or PRs related to code execution labels Jun 13, 2020
@Razican Razican linked an issue Jun 13, 2020 that may be closed by this pull request
@croraf
Copy link
Contributor Author

croraf commented Jun 13, 2020

I planned to do some tests. But I don't know how to assert the error output from the script:

  • I tried try/catch but seems not to be working.

  • instanceof is also not implemented to test if ReferenceError is thrown.

  • for any error thrown we currently print the error object that is thrown, instead of error message

Also several tests currently fail, because they were written to expect wrong thing, for example:

        scenario = r#"
          {
            let bar = "bar";
          }
          bar == undefined;
        "#;

        assert_eq!(&exec(scenario), "true");

This is incorrect assertion. bar is not undefined but a ReferenceError should is thrown.

@croraf croraf marked this pull request as ready for review June 16, 2020 10:49
@Razican
Copy link
Member

Razican commented Jun 16, 2020

I tried try/catch but seems not to be working.

try/catch should be working. If it's not, then it's a bug. I think that the part of try/catch returning something is the part that is not 100% spec compliant, but exceptions can be thrown and catched, and finally blocks ran.

for any error thrown we currently print the error object that is thrown, instead of error message

This might be something to look at, since depending on which function you use to execute the code, this will happen. If we use exec() or forward(), then yes, we return the string representation, but if we use forward_val(), then we return a Result<Value, Value>, and here, we could check if it's an error, and if it is, we can get more information, such as it being an Object and probably its instance information.

Also several tests currently fail, because they were written to expect wrong thing, for example:

        scenario = r#"
          {
            let bar = "bar";
          }
          bar == undefined;
        "#;

        assert_eq!(&exec(scenario), "true");

This is incorrect assertion. bar is not undefined but a ReferenceError should is thrown.

For these cases, just fix the test or remove it, since it's not spec compliant.

@codecov
Copy link

codecov bot commented Jun 16, 2020

Codecov Report

Merging #488 into master will increase coverage by 0.01%.
The diff coverage is 76.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   67.62%   67.64%   +0.01%     
==========================================
  Files         160      162       +2     
  Lines        9836     9871      +35     
==========================================
+ Hits         6652     6677      +25     
- Misses       3184     3194      +10     
Impacted Files Coverage Δ
boa/src/builtins/error/mod.rs 28.00% <ø> (ø)
boa/src/builtins/error/reference.rs 56.00% <56.00%> (ø)
boa/src/exec/exception.rs 76.19% <80.00%> (+9.52%) ⬆️
boa/src/exec/identifier/mod.rs 88.88% <88.88%> (ø)
boa/src/builtins/mod.rs 100.00% <100.00%> (ø)
boa/src/environment/lexical_environment.rs 73.83% <100.00%> (ø)
boa/src/exec/mod.rs 64.82% <100.00%> (+0.42%) ⬆️
boa/src/exec/tests.rs 100.00% <100.00%> (ø)
boa/src/exec/declaration/mod.rs 72.46% <0.00%> (-1.45%) ⬇️
... and 3 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 64fca0c...b34da02. Read the comment docs.

@croraf croraf changed the title Fix identifier resolution [ReferenceError] complete solution Jun 16, 2020
@croraf
Copy link
Contributor Author

croraf commented Jun 16, 2020

Try/Catch is working. The issue was in something else. I thus managed to unignore and verify 4 additional test cases.

@Razican Razican linked an issue Jun 16, 2020 that may be closed by this pull request
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.

This looks perfect to me :)

@Razican Razican added this to the v0.9.0 milestone Jun 16, 2020
@HalidOdat HalidOdat merged commit 4ae939a into boa-dev:master Jun 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[try/catch] not returning correct value [executor] Use of undefined variable should throw an error
3 participants