Error handling in environment

[54k1]

This PR introduces error handling in environment.
This Pull Request fixes/closes #622.

• Environment methods (which are prone to errors) now return a Result<X, ErrorKind>, where X is either () or Value.
• ErrorKind enum gives a hint to the other modules about the Error. According to the spec, there only are ReferenceError and TypeError generated from environment. And so there only are two fields in the ErrorKind enum.
• Since ErrorKind is returned, any code using environment must map data of type ErrorKind to type Error. This is achieved using to_error method. For instance, env.get_this_binding().map_err(|e| e.to_error(ctx))?;
• panic!s and assert!s that are present in some environment methods correspond to assert statements in the spec. Some things have to be guaranteed (such as prior existence of a binding) and if they fail, they may lead to panic!.

[codecov]

Codecov Report

Merging #659 (704f942) into master (015c11e) will decrease coverage by 0.16%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master     #659      +/-   ##
==========================================
- Coverage   59.97%   59.80%   -0.17%     
==========================================
  Files         166      167       +1     
  Lines       10851    10982     +131     
==========================================
+ Hits         6508     6568      +60     
- Misses       4343     4414      +71     

Impacted Files	Coverage Δ
boa/src/environment/global_environment_record.rs	22.80% <14.70%> (-5.15%)	⬇️
boa/src/environment/object_environment_record.rs	21.05% <25.00%> (-2.03%)	⬇️
boa/src/syntax/ast/node/mod.rs	46.92% <25.00%> (-1.11%)	⬇️
boa/src/environment/function_environment_record.rs	33.33% <32.69%> (-1.78%)	⬇️
.../src/environment/declarative_environment_record.rs	39.32% <37.83%> (+0.65%)	⬆️
boa/src/environment/mod.rs	50.00% <50.00%> (ø)
boa/src/value/operations.rs	48.58% <50.00%> (ø)
...a/src/syntax/ast/node/iteration/for_of_loop/mod.rs	58.97% <63.15%> (+5.36%)	⬆️
boa/src/syntax/ast/node/operator/bin_op/mod.rs	72.41% <66.66%> (-1.28%)	⬇️
boa/src/environment/lexical_environment.rs	69.23% <68.18%> (-3.11%)	⬇️
... and 13 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 015c11e...704f942. Read the comment docs.

[HalidOdat]

This looks good! we should add some test cases where a panic was being caused.

[54k1]

@HalidOdat I think it would be better to add tests after there's support for strict mode added (After that, some implementation in exec(Assignment Operator) needs to be fixed).

• The Assignment operator currently creates a new binding in the current scope if no binding was found in the environment stack.
  This is erroneous since in functions, any assignment without a prior declaration must result in addition of new entry to the global scope (in non-strict mode). In strict mode however, it must throw a ReferenceError.

Current behaviour: Assignments without prior declarations are added to the function scope.

>> function f () {
x = 12;
}
>> f()
>> x // Must be visible in global
Uncaught: "ReferenceError": "x is not defined"

Correct behaviour:

>> function f () {
    x = 12;
}
>> f()
undefined
>> x
12
// Ok

>> function f () {
    'use strict';
    x = 12;
}
>> f() // Can't do this in strict mode
Uncaught ReferenceError: assignment to undeclared variable x

• There's also another issue which is that the following must throw a SyntaxError.

let x = 12;
const x = 12;

What do you think?

[54k1]

I'm taking a re-look at this.

[Razican]

Hi! It seems this requires a rebase :) what is the state of the PR?

[Razican]

This is looking very good. The CI error is unerlated, and seems to be related to the new Test262 conformance comments.

It should be writing this comment, but it's failing:

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,493	78,493	0
Passed	19,596	19,600	+4
Ignored	15,585	15,585	0
Failed	43,312	43,308	-4
Panics	447	413	-34
Conformance	24.97	24.97	+0.01%

[Razican]

I think we could use a Box<str> here to reduce a bit the memory footprint.

[54k1]

The Error messages are however dynamic (depend on variable names, etc.) which is why we had used a String earlier. Could you please elaborate a bit more?

[Razican]

Yep, a String contains a pointer, a length and a capacity. The capacity indicates how many bytes are allocated for the string, while the length indicates how many are in use. This is useful to grow the string without needing to allocate, but once we know that the String will not change anymore, it's better to drop the excess capacity and have just a pointer with length, a Box<str>.

This can be constructed using String::into_boxed_str() you can find more information on the standard library documentation :).

[54k1]

@Razican Finally, when we are ready to construct the actual Error object, the API currently expects a template which can convert to a String as an argument (for the message).

pub fn construct_reference_error<M> (&mut self, msg: M) -> Value
where
    M: Into<String> {
...
}

Would a Box<str> be useful in this case?

[Razican]

Most (if not all) of the things that can be converted to a String can be converted to a Box<str>, so changing it to Into<Box<str>> could make the trick.

[HalidOdat]

Looks good to me! :)

[Razican]

I would spect this to be able to remove some of the panics listed in #817. Namely:

Identifier x has already been declared #659, count: 58

Example:

thread 'main' panicked at 'Identifier x has already been declared', boa/src/environment/function_environment_record.rs:94:13

and

Binding already exists! #659, count: 30

Example:

thread 'main' panicked at 'Binding already exists!', boa/src/environment/global_environment_record.rs:107:13

and

TypeError: Cannot mutate an immutable binding x #659, count: 2

Example:

thread 'main' panicked at 'TypeError: Cannot mutate an immutable binding x', boa/src/environment/declarative_environment_record.rs:117:13

and

thread 'main' panicked at 'TypeError: Cannot mutate an immutable binding i', boa/src/environment/declarative_environment_record.rs:117:13

But I think they still exist in this branch, right?

[Razican]

Hi @54k1, did you have the chance to review why are those environment panics still happening?

[Razican]

This looks good to me. Only needs a cargo fmt.