Normative: Eliminate extra environment for eval in parameter initializers

[ajklein]

This environment was originally added to the spec as the fix for a bug reported by @anba
during the development of ES6. It first appeared in rev33, after a brief discussion
on es-discuss.

Yet as explained in my presentation at the November 2017 meeting,
it has not been uniformly implemented in engines, and its correct implementation in engines
that have attempted it has been a major source of bugs.

This PR implements "Option 4" of the linked slides, which coincides with the implementation
in JavaScriptCore, and thus is believed to be web-compatible.

[anba]

This PR implements "Option 4" of the linked slides, which coincides with the implementation
in JavaScriptCore, and thus is believed to be web-compatible.

A bit hand-wavy because I haven't checked the changes in detail, but I don't think it completely matches JSC (which shouldn't be a requirement).

function f0(a = eval("var b"), b) { }
// f0(); // No error

function f1(a = eval("var b = 0"), b) { }
// f1(); // ReferenceError, but no error in JSC

function f2(a = eval("function b(){}"), b) { }
// f2(); // ReferenceError, but SyntaxError in JSC

function f3(a = eval("{ function b(){} }"), b) { }
// f3(); // ReferenceError, but no error in JSC

function f4(b, a = eval("var b = 0")) { print(b) }
// f4(); // Prints "0"

function f5(b, a = eval("function b(){}")) { print(b) }
// f5(); // Prints "function b(){}", but SyntaxError in JSC

function f6(b, a = eval("{ function b(){} }")) { print(b) }
// f6(); // Prints "function b(){}", but "undefined" in JSC

[ajklein]

@anba Thanks for the examples!

function f0(a = eval("var b"), b) { }
// f0(); // No error

JSC seems to get this wrong in general: it doesn't detect conflicts between var declarations and lexical variables.

function f1(a = eval("var b = 0"), b) { }
// f1(); // ReferenceError, but no error in JSC

I think the PR actually makes this a SyntaxError during EvalDeclarationInstantiation. That's its intention, anyway; if your reading is different, that's definitely something I want to change.

function f2(a = eval("function b(){}"), b) { }
// f2(); // ReferenceError, but SyntaxError in JSC

Same as above, I think SyntaxError is correct.

function f3(a = eval("{ function b(){} }"), b) { }
// f3(); // ReferenceError, but no error in JSC

Why do you think this should be a ReferenceError? I would expect this to result in no error.

function f4(b, a = eval("var b = 0")) { print(b) }
// f4(); // Prints "0"

Yeah, this is the same JSC bug as f0; I think this should be a SyntaxError.

function f5(b, a = eval("function b(){}")) { print(b) }
// f5(); // Prints "function b(){}", but SyntaxError in JSC

SyntaxError is what I expect here too.

function f6(b, a = eval("{ function b(){} }")) { print(b) }
// f6(); // Prints "function b(){}", but "undefined" in JSC

"undefined" seems right to me, too. Are you expecting Annex B to cause the function to get assigned to b? I was thinking it wouldn't, since there's a conflicting binding in the outer scope.

[anba]

function f1(a = eval("var b = 0"), b) { }
// f1(); // ReferenceError, but no error in JSC

I think the PR actually makes this a SyntaxError during EvalDeclarationInstantiation. That's its intention, anyway; if your reading is different, that's definitely something I want to change.

The parameter bindings are installed in the current execution context's VariableEnvironment, so the redeclaration check in EvalDeclarationInstantiation, step 5.d won't report any SyntaxErrors for the variable declaration b. Later in EvalDeclarationInstantiation (step 16.b) a new binding for b is not created, because the VariableEnvironment's environment record already contains a binding for b. So when we then even later evaluate var b = 0, we'll call PutValue (13.3.2.4, step 6), PutValue then calls SetMutableBinding, and 8.1.1.1.5 SetMutableBinding, step 4 then throws a ReferenceError because the binding for b isn't yet initialized.

And because EvalDeclarationInstantiation won't throw any SyntaxErrors, Annex B semantics apply for the other cases with function b() {}.

[ajklein]

@anba Thanks, got it. The quickest fix I can think of would be, in sloppy mode, to introduce a LexicalEnvironment that sits just below the VariableEnvironment of the function, and declare parameters there. Do you think this would cause any additional problems?

[anba]

Do you mean the following environment set-up? (I haven't yet checked if this approach is feasible!)

Non-strict function with parameter expressions:

           Description                    Environment kind               Execution context state
+----------------------------------+     +----------------+     +-----------------------------------------+
|                                  |     |                |     |                                         |
| Var-bindings from eval in params +-----+  FunctionEnv   +-----+ VariableEnv during parameter evaluation |
|                                  |     |                |     |                                         |
+----------------------------------+     +----------------+     +-----------------------------------------+
                                                |
                                                |
        +--------------------------+     +------v---------+     +----------------------------------------+
        |                          |     |                |     |                                        |
        |  Parameter bindings      +-----+ DeclarativeEnv +-----+ LexicalEnv during parameter evaluation |
        |  "arguments" binding (?) |     |                |     |                                        |
        |                          |     +----------------+     +----------------------------------------+
        +--------------------------+            |
                                                |
   +--------------------------------+    +------v---------+     +----------------------------------------+
   |                                |    |                |     |                                        |
   | Var-bindings from body         +----+ DeclarativeEnv +-----+ VariableEnv after parameter evaluation |
   | Var-bindings from eval in body |    |                |     |                                        |
   |                                |    +----------------+     +----------------------------------------+
   +--------------------------------+           |
                                                |
   +--------------------------------+    +------v---------+     +---------------------------------------+
   |                                |    |                |     |                                       |
   | Lexical bindings from body     +----+ DeclarativeEnv +-----+ LexicalEnv after parameter evaluation |
   |                                |    |                |     |                                       |
   +--------------------------------+    +----------------+     +---------------------------------------+

Strict function with parameter expressions (no changes):

           Description              Environment kind               Execution context state
       +---------------------+     +----------------+     +-----------------------------------------+
       |                     |     |                |     |                                         |
       | Parameter bindings  +-----+  FunctionEnv   +-----+ VariableEnv during parameter evaluation |
       | "arguments" binding |     |                |     | LexicalEnv during parameter evaluation  |
       |                     |     +------+---------+     |                                         |
       +---------------------+            |               +-----------------------------------------+
                                          |
+----------------------------+     +------v---------+     +----------------------------------------+
|                            |     |                |     |                                        |
| Var-bindings from body     +-----+ DeclarativeEnv +-----+ VariableEnv after parameter evaluation |
| Lexical bindings from body |     |                |     | LexicalEnv after parameter evaluation  |
|                            |     +----------------+     |                                        |
+----------------------------+                            +----------------------------------------+

Non-strict function without parameter expressions (no changes):

           Description                    Environment kind               Execution context state
+----------------------------------+     +----------------+     +----------------------------------------+
|                                  |     |                |     |                                        |
|  Parameter bindings              +-----+  FunctionEnv   +-----+ VariableEnv                            |
|  "arguments" binding             |     |                |     | LexicalEnv during parameter evaluation |
|  Var-bindings from body          |     +------+---------+     |                                        |
|  Var-bindings from eval in body  |            |               +----------------------------------------+
|                                  |            |
+----------------------------------+            |
                                                |
  +--------------------------------+     +------v---------+     +---------------------------------------+
  |                                |     |                |     |                                       |
  | Lexical bindings from body     +-----+ DeclarativeEnv +-----+ LexicalEnv after parameter evaluation |
  |                                |     |                |     |                                       |
  +--------------------------------+     +----------------+     +---------------------------------------+

Strict function without parameter expressions (no changes):

           Description                    Environment kind       Execution context state
+----------------------------------+     +----------------+     +-----------------------+
|                                  |     |                |     |                       |
|  Parameter bindings              +-----+  FunctionEnv   +-----+ VariableEnv           |
|  "arguments" binding             |     |                |     | LexicalEnv            |
|  Var-bindings from body          |     +----------------+     |                       |
|  Lexical bindings from body      |                            +-----------------------+
|                                  |
+----------------------------------+

With:

• FunctionEnv = Lexical Environment with a function Environment Record
• DeclarativeEnv = Lexical Environment with a declarative Environment Record

[ajklein]

Yes, that's what I had in mind.

[ajklein]

@bterlson have you had a chance to look at this? (By "this" I guess I mean both the PR and @anba's lovely ASCII art above).

[syg]

@ajklein So based on @anba's diagrams, that means eval in parameter expression positions won't actually have access to any of the parameters, since the eval has the environment that encloses the parameter environment?

[ajklein]

@syg That's not my understanding. The eval should have access to the parameters; it's simply that any var bindings created in the eval will be "hoisted" above the parameters (or cause an error in case they conflict with the name of a parameter).

[syg]

Ah, right, the direct eval will have its own LexicalEnv which has the parameter environment as the enclosing environment, while its VariablesEnv will be the function environment. That seems okay.

[ajklein]

I've update the PR to match @anba's diagram. Another round of review would be greatly appreciated. I'm also not at all happy with my NOTEs, so any suggestions for how to add more clarity would be good.

[rwaldron]

@leobalter @spectranaut @nomadtechie we should review these changes for potential Test262 updating.

[bakkot]

LGTM, this is a nice simplification. (That is, in addition to implementing the change in question.)

Personally I wouldn't bother with the _strict_ branch, since the difference is unobservable; see the closely related #623. But it doesn't really matter, and could later be changed as part of addressing #623.

[ajklein]

Thanks for the review, @bakkot. I'd rather leave the strict branch as-is in this PR to leave the focus on the functional change, and leave it for #623 to replace this text with an informative note. If a fix for #623 gets merged before this, then of course I'd reflect that change here too.

[woess]

If implementation complexity was the rationale for this proposal, why was Option 3 "Make eval in parameters strict" dismissed? In comparison, that option seems the best: it's semantically sane and relatively easy to implement.

Admittedly, implicitly switching to strict mode might come as a bit of a surprise, but there are other precedents in the spec where new language features change legacy behavior.
And if that's a (compatibility) concern, there'd also be the option to leave the eval code in non-strict mode but with the strict mode env setup, but I don't think there's a necessity.

IMHO, Option 4 (this PR) is not a real improvement. It reintroduces the undesirable and error-prone behavior that eval var declarations leak into the caller. Of course, the current spec version is similarly flawed but, at least, the variable leaks are more confined.

[ajklein]

@woess It was indeed the concerns you lay out in your second paragraph I was worried about. We'd either have to decide that it was OK to change the strict-ness of the existing eval code, or create a new thing (strict eval with sloppy code inside).

I agree that the eval var declaration leakage is not great, but the amount of spec and implementation complexity (and security issues in the wild, in Chrome/V8's case) to "defend" against what I think is a very rare case doesn't seem worthwhile to me.

[ajklein]

@bterlson @ljharb @bmeck Would like to pick this up again, pinging editors for some formal review (I believe @syg looked at this too).

[bmeck]

@rwaldron did we ever figure out if there were any Test262 concerns?

[ajklein]

Re-pinging this thread for a new year. I continue to believe this is a nice simplification, which if landed and implemented in engines would increase interop while reducing both cognitive and implementation complexity.

@LeszekSwirski of V8 might be particularly interested in the "implementation complexity reduction bit"

[ljharb]

@ajklein would you be able to resummarize both a) what current browsers do, and b) what would change as a result of this patch? Thanks :-)

[bakkot]

@ljharb / @ajklein: to jostle my own memory, I wrote up my understanding / recollection of the current and proposed behavior with some tests and their results on current engines.

(Edit: the above gist is still accurate as of 2019-11-21.)

Basically: the spec currently says that each parameter is evaluated in its own unique var scope which is just inside of the parameter scope. After this change, there would be a var scope just outside of the parameter scope in which all parameters would be evaluated. V8 and SpiderMonkey correctly implement the current spec, Chakra and JSC do their own unique things.

[bakkot]

@erights There would be no observable effects for strict-only code, because strict evals already get their own VariableEnvironment which prevents hoisting declarations out of the eval. This change only affects code which has non-strict direct evals in parameter expressions.

[ajklein]

This isn't something I have bandwidth to push on right now, but I still think it's a good idea so would be happy for someone else to take it over.

[syg]

I will be taking over this.

[syg]

I rebased this as well.

[leobalter]

Tests have been merged