Reduce the number of Array-related panics

[jakubfijalkowski]

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.

[codecov]

Codecov Report

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

@@            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]

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]

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]

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]

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]

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 :)

[Razican]

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?

[jakubfijalkowski]

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]

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]

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]

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. :)

[HalidOdat]

Looks good!

Check my comments on how to improve this :)

[jakubfijalkowski]

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]

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