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] - Fix unreachable panics in compile_access #1861

Closed

Conversation

addisoncrump
Copy link
Contributor

This PR changes the following:

  • More elegantly handles illegal access statements in compile_access
  • Adds a slew of previously unhandled illegal access test cases

Caveats

It is very, very likely that you will want to simply restrict unary and assignment operations in the AST. However, this prevents crashes in the meantime with a error that is just slightly less detailed than if it were implemented in AST.

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #1861 (5916e2c) into main (3b53fec) will decrease coverage by 0.02%.
The diff coverage is 36.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1861      +/-   ##
==========================================
- Coverage   46.41%   46.39%   -0.03%     
==========================================
  Files         206      206              
  Lines       16790    16813      +23     
==========================================
+ Hits         7793     7800       +7     
- Misses       8997     9013      +16     
Impacted Files Coverage Δ
boa_engine/src/bytecompiler.rs 38.75% <36.11%> (-0.10%) ⬇️
boa_engine/src/environments/compile.rs 39.82% <0.00%> (-2.66%) ⬇️
boa_engine/src/object/internal_methods/array.rs 76.36% <0.00%> (-1.42%) ⬇️
boa_engine/src/builtins/date/mod.rs 77.50% <0.00%> (+0.17%) ⬆️
boa_engine/src/builtins/number/mod.rs 75.65% <0.00%> (+0.28%) ⬆️
boa_engine/src/syntax/parser/expression/update.rs 32.25% <0.00%> (+3.22%) ⬆️

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 3b53fec...5916e2c. Read the comment docs.

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 to me!

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Good job! Just some dbg leftovers

boa_engine/src/tests.rs Outdated Show resolved Hide resolved
boa_engine/src/tests.rs Outdated Show resolved Hide resolved
boa_engine/src/tests.rs Outdated Show resolved Hide resolved
Comment on lines 1343 to 1361
let mut context = Context::default();

let test_cases = [
"(()=>{})() -= 5",
"(()=>{})() *= 5",
"(()=>{})() /= 5",
"(()=>{})() %= 5",
"(()=>{})() &= 5",
"(()=>{})() ^= 5",
"(()=>{})() |= 5",
"(()=>{})() += 5",
"(()=>{})() = 5",
];

for case in &test_cases {
let string = forward(&mut context, case);

assert!(string.starts_with("Uncaught \"SyntaxError\": "));
}
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll change that over; I was following the pattern I observed with similar test cases.

@HalidOdat
Copy link
Member

Caveats

It is very, very likely that you will want to simply restrict unary and assignment operations in the AST. However, this prevents crashes in the meantime with a error that is just slightly less detailed than if it were implemented in AST.

Yup. These are early errors that should be checked during parsing, and the unreachable!() should be unreachable if we implement them.

@Razican Razican added bug Something isn't working vm Issues and PRs related to the Boa Virtual Machine. labels Feb 23, 2022
@Razican Razican added this to the v0.14.0 milestone Feb 23, 2022
addisoncrump and others added 2 commits March 5, 2022 14:57
Co-authored-by: jedel1043 <jedel0124@gmail.com>
@Razican Razican requested review from HalidOdat and jedel1043 March 5, 2022 22:00
boa_engine/src/bytecompiler.rs Outdated Show resolved Hide resolved
@addisoncrump addisoncrump requested a review from jedel1043 March 6, 2022 00:31
Copy link
Member

@jedel1043 jedel1043 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 to me!

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 6, 2022
This PR changes the following:

- More elegantly handles illegal access statements in compile_access
- Adds a slew of previously unhandled illegal access test cases

### Caveats

It is very, very likely that you will want to simply restrict unary and assignment operations in the AST. However, this prevents crashes in the meantime with a error that is just slightly less detailed than if it were implemented in AST.
@bors
Copy link

bors bot commented Mar 6, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fix unreachable panics in compile_access [Merged by Bors] - Fix unreachable panics in compile_access Mar 6, 2022
@bors bors bot closed this Mar 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants