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

wrap the regress error for now #1027

Merged
merged 1 commit into from
Jan 2, 2021
Merged

wrap the regress error for now #1027

merged 1 commit into from
Jan 2, 2021

Conversation

jasonwilliams
Copy link
Member

@jasonwilliams jasonwilliams commented Jan 2, 2021

Regress panics on "Invalid character escape"s.
However on V8 and other JS engines these seem to be acceptable.

const a = /,\;/;

was causing issues in Boa, due to regress panicing and us not catching.
This PR now changes it to a SyntaxError, however this is a temporary fix, ideally it should be accepted.

Raised upstream:
ridiculousfish/regress#26

@codecov
Copy link

codecov bot commented Jan 2, 2021

Codecov Report

Merging #1027 (66135bc) into master (90a8587) will decrease coverage by 1.53%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1027      +/-   ##
==========================================
- Coverage   60.27%   58.73%   -1.54%     
==========================================
  Files         169      172       +3     
  Lines       11415    11732     +317     
==========================================
+ Hits         6880     6891      +11     
- Misses       4535     4841     +306     
Impacted Files Coverage Δ
boa/src/builtins/regexp/mod.rs 70.88% <100.00%> (+0.75%) ⬆️
boa/src/syntax/ast/node/operator/bin_op/mod.rs 52.32% <0.00%> (-14.35%) ⬇️
boa/src/syntax/ast/node/operator/unary_op/mod.rs 56.41% <0.00%> (-11.93%) ⬇️
boa/src/syntax/ast/node/statement_list/mod.rs 44.18% <0.00%> (-4.54%) ⬇️
boa/src/context.rs 61.56% <0.00%> (-4.13%) ⬇️
boa/src/lib.rs 88.46% <0.00%> (ø)
boa/src/profiler.rs 0.00% <0.00%> (ø)
boa/src/vm/mod.rs 0.00% <0.00%> (ø)
boa/src/vm/compilation.rs 0.00% <0.00%> (ø)
boa/src/vm/instructions.rs 0.00% <0.00%> (ø)
... and 1 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 90a8587...66135bc. Read the comment docs.

@Razican Razican added this to the v0.11.0 milestone Jan 2, 2021
@Razican Razican added the builtins PRs and Issues related to builtins/intrinsics label Jan 2, 2021
@Razican
Copy link
Member

Razican commented Jan 2, 2021

Looking good!

Test262 conformance changes:

Test result master count PR count difference
Total 78,493 78,493 0
Passed 23,805 23,944 +139
Ignored 15,585 15,585 0
Failed 39,103 38,964 -139
Panics 362 147 -215
Conformance 30.33 30.50 +0.18%

@jasonwilliams jasonwilliams changed the title wrap the regress panic for now wrap the regress error for now Jan 2, 2021
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 good, I would just remove the check for a Syntax error in the test, in order to avoid changing it when we fix regress.

fn no_panic_on_invalid_character_escape() {
let mut context = Context::new();

assert_eq!(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just try to run it, without checking that it creates a Syntax error, since that will change in the future once we fix regress. If it panics, the test will fail, if it doesn't, it will pass.

@Razican
Copy link
Member

Razican commented Jan 2, 2021

This is linked to #817.

boa/src/builtins/regexp/tests.rs Outdated Show resolved Hide resolved
@RageKnify RageKnify merged commit 98945c8 into boa-dev:master Jan 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants