Modify environment binding behaviour of function

[54k1]

This Pull Request fixes/closes #669.

If a binding for the function name already exists, we should simply override it (set_mutable_binding), otherwise we need to create a new binding. For this to be accurate, we need to also fix #858, since now we are simply overriding the binding (potentially let or const).

This seems to fix it, however, I haven't found this in the documentation. The implementation of var also seems to follow this. Could anybody provide a link to the part where this is described in the documentation?

[codecov]

Codecov Report

Merging #1010 (da0f878) into master (ceca316) will increase coverage by 0.01%.
The diff coverage is 69.23%.

@@            Coverage Diff             @@
##           master    #1010      +/-   ##
==========================================
+ Coverage   58.61%   58.62%   +0.01%     
==========================================
  Files         172      172              
  Lines       11763    11766       +3     
==========================================
+ Hits         6895     6898       +3     
  Misses       4868     4868              

Impacted Files	Coverage Δ
...c/syntax/ast/node/declaration/function_decl/mod.rs	70.73% <69.23%> (+2.31%)	⬆️

---

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 ceca316...f1dd836. Read the comment docs.

[RageKnify]

Test262 conformance changes:

Test result	master count	PR count	difference
Total	78,493	78,493	0
Passed	24,005	24,037	+32
Ignored	15,585	15,585	0
Failed	38,903	38,871	-32
Panics	90	59	-31
Conformance	30.58	30.62	+0.04%

The numbers look good.

I think adding some tests would be nice to make the desired behaviour clearer.

[jasonwilliams]

Thanks for looking at this @54k1
I think currently we don't check for duplicates at parse-time nor is there a mechanism for doing so. So we don't have good early error support for that.

One place to handle that would be here (however this only accounts for when bindings are created as a list.
https://github.com/jasonwilliams/boa/blob/master/boa/src/syntax/parser/statement/declaration/lexical.rs#L145-L208

This could need a new issue, but im open to hear what the others think

This seems to fix it, however, I haven't found this in the documentation. The implementation of var also seems to follow this. Could anybody provide a link to the part where this is described in the documentation?

Could you go into more detail here? What documentation are you referring to? The specification?

I think adding some tests would be nice to make the desired behaviour clearer.

Yep adding some tests here would be good enough as a fix for the "environment side"

[Razican]

This fix looks good to me. We still need to fix #858, but I believe that is a different issue. We need to implement a pre-parsing strategy I think.

[54k1]

@jasonwilliams Yes, I meant the specification, sorry about the confusion
@Razican Yes, it's not directly related to #858, but assumes that SyntaxError is thrown for those cases mentioned in #858.
@RageKnify , I've added just a basic test. What other things can we test?

[RageKnify]

I've found CreateGlobalFunctionBinding in the spec, which seems to be the approppriate section of the spec that describes the behaviour we were looking for.

The "call-trace" seems to be ScriptEvaluation -> GlobalDeclarationInstantiation -> env.CreateGlobalFunctionBinding

[RageKnify]

Seems like run for FunctionDecl should be using GlobalEnvironmentRecord::create_global_function_binding, but instead the bindings are created in the LexicalEnvironment. I'm not really familiar with these aspects, maybe @jasonwilliams or other Members have a better understanding of these concepts.

[jasonwilliams]

@RageKnify that's interesting, the way functions are bound right now is we just use the containing scope. I don't know if we're supposed to be using create_global_function_binding for all of our functions maybe that's some research we need to do. That would in theory make runtime slower though, as every function call would need to traverse the scope chain to the top.

@54k1 you're right in that in theory we would catch this during parsing (as an early error), but we don't have functionality to check duplicates as parsing time just yet, but I see you've created an issue for that anyway.

Looks good to me

[Razican]

Does this help with #989?

[RageKnify]

Does this help with #989?

No, just tested it, ran the exemple code from #989 in this branch and the bug remains.