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

Reduce the number of Array-related panics #919

Merged
merged 15 commits into from
Dec 15, 2020

Conversation

jakubfijalkowski
Copy link
Contributor

@jakubfijalkowski jakubfijalkowski commented Oct 28, 2020

Hello 👋 !

This Pull Request addresses some of the panics in Test262 suite #817, most notably the ones in Array module.

They (most of the time) boil down to changing get_field("length").as_number().unwrap() to get_field("length").to_length(context). I needed to add context: &mut Context to construct_array and add_to_array_object because of that. I've also added a little bit of +/-Inf, NaN and out-of-range checks to the code.

Basically, this reduced the number of panics in Array part of the test suite to 8 (and these are related to display).

The main problem that I see with the changes are the semi-random usize/isize/i32/f64 casts, but I really don't know what would be best to do with them and would appreciate a little bit of guidance here.

@jakubfijalkowski jakubfijalkowski changed the title Fix array length panics Reduce the number of Array-related panics Oct 28, 2020
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #919 (bbecaf3) into master (6eac058) will increase coverage by 0.14%.
The diff coverage is 79.67%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
+ Coverage   59.21%   59.36%   +0.14%     
==========================================
  Files         166      166              
  Lines       10570    10719     +149     
==========================================
+ Hits         6259     6363     +104     
- Misses       4311     4356      +45     
Impacted Files Coverage Δ
boa/src/builtins/map/map_iterator.rs 70.68% <0.00%> (-1.25%) ⬇️
boa/src/builtins/array/mod.rs 73.37% <78.50%> (-0.15%) ⬇️
boa/src/value/mod.rs 74.33% <92.30%> (+0.63%) ⬆️
boa/src/builtins/function/mod.rs 73.73% <100.00%> (ø)
boa/src/syntax/ast/node/array/mod.rs 74.07% <100.00%> (ø)
boa/src/syntax/parser/statement/block/mod.rs 78.94% <0.00%> (-4.39%) ⬇️
boa/src/syntax/parser/function/mod.rs 64.17% <0.00%> (-3.52%) ⬇️
boa/src/syntax/lexer/template.rs 61.11% <0.00%> (-3.18%) ⬇️
boa/src/syntax/lexer/mod.rs 64.86% <0.00%> (-2.79%) ⬇️
boa/src/syntax/lexer/regex.rs 39.36% <0.00%> (-1.67%) ⬇️
... and 14 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 6eac058...bbecaf3. Read the comment docs.

@Razican
Copy link
Member

Razican commented Oct 29, 2020

Test262 conformance changes:

Test result master count PR count difference
Total 78,460 78,460 0
Passed 18,964 19,380 +416
Ignored 15,559 15,559 0
Failed 43,937 43,521 -416
Panics 1,127 465 -662
Conformance 24.17 24.70 +0.53%

@RageKnify
Copy link
Member

I think following the advice in this SO post we should avoid conversions with as isize or as usize and use From or TryFrom instead with for example: usize::from(x) that makes it clearer that it is a safe conversion.
@HalidOdat @Razican what's your opinion? Since integer conversion is the sort of thing that can easily go wrong I'd rather be explicit about it.

@Razican
Copy link
Member

Razican commented Nov 2, 2020

I think following the advice in this SO post we should avoid conversions with as isize or as usize and use From or TryFrom instead with for example: usize::from(x) that makes it clearer that it is a safe conversion.
@HalidOdat @Razican what's your opinion? Since integer conversion is the sort of thing that can easily go wrong I'd rather be explicit about it.

Yes, we should not use as. But I'm happy to merge this as-is, since we use it widely through the codebase and that would require a different PR, I think.

@RageKnify
Copy link
Member

Yes, we should not use as. But I'm happy to merge this as-is, since we use it widely through the codebase and that would require a different PR, I think.

I figured since we're changing this part of the code we could try to clean up this part of the code and as other parts are changed in PRs we can ask that those PRs remove those uses.
I preffer having safe conversions as we go since I can trust the code more if it uses safe covnersions.

@Razican
Copy link
Member

Razican commented Nov 2, 2020

Yes, we should not use as. But I'm happy to merge this as-is, since we use it widely through the codebase and that would require a different PR, I think.

I figured since we're changing this part of the code we could try to clean up this part of the code and as other parts are changed in PRs we can ask that those PRs remove those uses.
I preffer having safe conversions as we go since I can trust the code more if it uses safe covnersions.

OK, let's do that :)

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 very good, and I'm loving the new panic count :D
As we mentioned in the comments, it's best if we use a try_from() when casting, or from() if possible, to avoid casting issues. Moreover, I see we use wrapping_add() in many places. I'm not familiar with the spec, but are we sure we should be using a wrapping addition?

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved
@jakubfijalkowski
Copy link
Contributor Author

jakubfijalkowski commented Nov 2, 2020

Thanks for the reviews and comments!

Moreover, I see we use wrapping_add() in many places. I'm not familiar with the spec, but are we sure we should be using a wrapping addition?

When reading the ECMA specs I had a feeling that they assume that overflow can't happen, since everywhere they operate on JS Number and not on concrete types. Moreover, since this is JS, most of the time they assumed it is floating point number and handled the Infinity cases explicitly, which I also added in some places.

I think that the whole length/index mgmt needs to be thought over - the new ECMA draft (30.10) uses ToIntegerOrInfinity to handle the Infinity/NaN case (esp. "Infinity" interpreted as number which is quite common in the suite :) ) and I think the current code deliberately ignores that (which makes some tests to fail) or is based on some older draft (since it uses ToInteger that currently is not defined in the spec).

I will try to write a little bit longer summary of my findings and dig a little bit deeper in the standard to find out what they assume should be done in this case.

@Razican
Copy link
Member

Razican commented Nov 2, 2020

This might also have to do with the fact that we use integers just as an optimization for numeric types. Working with integers is faster than working with floats, but, of course, they are not spec compliant, so sometimes we have to transform them back to floats.

@jakubfijalkowski
Copy link
Contributor Author

jakubfijalkowski commented Nov 2, 2020

I went through the file and changed all the casts to try_from/try_into. There are, unfortunately, some rough edges.

Since now the conversions can fail, we need to map errors. I put the impl From<std::num::TryFromIntError> for Value in value/conversions.rs. I'm not sure whether this really is the correct place nor whether the trait should be implemented at all. Explicit conversions for every call to try_from seem redundant and clutter the code, but having the conversion available everywhere might be error prone.

Not all casts are possible. There is no f64 -> isize nor f64 -> usize conversions available OOTB. num-traits implements the safe casts, but returns Option<T> and not Result<T, E>. I wrapped these in helper functions but I'm not sure whether that's really necessary (because the as cast is saturating and most of the time, this is what we expect).

I think most of the above errors will go away if a new type will be introduced that reflects the result of ToIntegerOrInfinity, i.e. something like:

enum IntegerOrInfinity {
    Integer(isize),
    NegativeInfinity,
    PositiveInfinity,
}

That way we would not need the f64 -> isize casts and we would be able to use integers everywhere.

Regarding the wrapping_* topic - I think that, most of the time, these should not be used. I can see two cases when these are used:

  1. offsetting the index that we ensured is safe (i.e. we won't overflow) - in this case, normal operations would be more appropriate IMO,
  2. offsetting the index, but then comparing it with length or 0, e.g. max(len.wrapping_add(start), 0) if start < 0, when selecting array slice. In this case, saturating_add would be correct as this would ignore the fact that we operate on finite types. If we saturate, we would correctly get 0 as the start index (per standard) since the len.saturating_add(start) will be isize::MIN. With wrapping, we end up with isize::MAX - (start + isize::MAX) = -start. If we do the same for end, we can construct a range that points to a random memory region. :)

@jakubfijalkowski
Copy link
Contributor Author

For tests, I implemented the ToIntegerOrInfinity idea in a86ea85. I was able (hopefully) to make it safe and eliminate most of the casts. I haven't used it everywhere, but you can find examples in Array::fill and Array::slice.

Tell me what do you think and I can either revert this or go and change rest of the methods. :)

@Razican Razican requested review from HalidOdat and removed request for HalidOdat and RageKnify December 4, 2020 08:29
@Razican Razican added bug Something isn't working builtins PRs and Issues related to builtins/intrinsics labels Dec 4, 2020
@Razican Razican added this to the v0.11.0 milestone Dec 4, 2020
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 good!

Check my comments on how to improve this :)

boa/src/value/to_integer_or_infinity.rs Outdated Show resolved Hide resolved
boa/src/value/to_integer_or_infinity.rs Outdated Show resolved Hide resolved
boa/src/value/to_integer_or_infinity.rs Outdated Show resolved Hide resolved
boa/src/value/mod.rs Outdated Show resolved Hide resolved
boa/src/value/conversions.rs Outdated Show resolved Hide resolved
@jakubfijalkowski
Copy link
Contributor Author

Thanks for the comments! They really did help me get back on track. Now the changes are (I think) more targeted and don't pollute unrelated code.

To sum up the changes:

  1. IntegerOrInfinity doesn't have the Undefined case to conform to standard better,
  2. The as_relative_* methods are now in array module (this is the only place where these are used),
  3. I removed the From<TryFromIntError> implementation from Value.

@Razican
Copy link
Member

Razican commented Dec 15, 2020

Let's merge this. I think all concerns were properly treated :)

@Razican Razican merged commit 8f388d5 into boa-dev:master Dec 15, 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 builtins PRs and Issues related to builtins/intrinsics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants