Writing to deleted global variable references has been classified as a spec bug

bakkot commented 9 years ago

See this es-discuss thread.

This invalidates at least the following tests:

leobalter commented 7 years ago

I need to investigate these cases, currently assigning this to myself.

leobalter commented 4 years ago

This definitely needs follow up. I shouldn't hold this one for too long.

leobalter commented 4 years ago

I'm having trouble with this piece of code:


Object.defineProperty(this, "x", {
  configurable: true,
  get: function() {
    delete this.x;
    return 2;
  }
});

(function() {
  "use strict";
  ++x;
})();

Here comes the evaluation:

  Runtime Semantics: Evaluation

  UpdateExpression : ++ UnaryExpression

    Let expr be the result of evaluating UnaryExpression.
    Let oldValue be ? ToNumeric(? GetValue(expr)).
    Let newValue be ! Type(oldValue)::add(oldValue, Type(oldValue)::unit).
    Perform ? PutValue(expr, newValue).
    Return newValue.

  PutValue ( V, W )

    ...
    Let base be GetBase(V).
    If IsUnresolvableReference(V) is true, then
    ...

  IsUnresolvableReference ( V )

    Assert: Type(V) is Reference.
    If the base value component of V is undefined, return true; otherwise return false.

expr is the Reference for x in the base value of the Global Env Record, right?.

IIUC, the base value of expr is the Global Env Record.

The delete happens in the GetValue(expr), PutValue is then called with the expr Reference, and here comes a trick: IsUnresovableReference returns true becase the base value component of V is still the Global Env Record. Is that correct so far?

back to PutValue

  PutValue ( V, W )

    ...
    Let base be GetBase(V).
    If IsUnresolvableReference(V) is true, then
    ...
    Else if IsPropertyReference(V) is true, then
    ...
    Else,
      Assert: base is an Environment Record.
      Return ? base.SetMutableBinding(GetReferencedName(V), W, IsStrictReference(V)) (see 8.1).

So we reach the SetMutableBinding for global Env Records. (8.1.1.4.5)

SetMutableBinding ( N, V, S )

The concrete Environment Record method SetMutableBinding for global Environment Records attempts to change the bound value of the current binding of the identifier whose name is the value of the argument N to the value of argument V. If the binding is an immutable binding, a TypeError is thrown if S is true. A property named N normally already exists but if it does not or is not currently writable, error handling is determined by the value of the Boolean argument S.

    Let envRec be the global Environment Record for which the method was invoked.
    Let DclRec be envRec.[[DeclarativeRecord]].
    If DclRec.HasBinding(N) is true, then
        Return DclRec.SetMutableBinding(N, V, S).
    Let ObjRec be envRec.[[ObjectRecord]].
    Return ? ObjRec.SetMutableBinding(N, V, S).

Now the DeclarativeRecord wont have a binding X. So we would roll to ObjRec.SetMutableBinding.

SetMutableBinding ( N, V, S )

The concrete Environment Record method SetMutableBinding for object Environment Records attempts to set the value of the Environment Record's associated binding object's property whose name is the value of the argument N to the value of argument V. A property named N normally already exists but if it does not or is not currently writable, error handling is determined by the value of the Boolean argument S.

    Let envRec be the object Environment Record for which the method was invoked.
    Let bindings be the binding object for envRec.
    Return ? Set(bindings, N, V, S).

Set ( O, P, V, Throw )

    ...
    Let success be ? O.[[Set]](P, V, O).
    If success is false and Throw is true, throw a TypeError exception.
    Return success.

This would give me no path to get a ReferenceError for the ++x and x being deleted mid process.

Although, the engines seem to not agree with anything:

➜  eshost -tsx 'Object.defineProperty(this, "x", {
  configurable: true,
  get: function() {
    delete this.x;
    return 2;
  }
});

(function() {
  "use strict";
  ++x;
})();
'
┌────────────────┬─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ChakraCore     │                                                                                                                             │
│                │ ReferenceError: Variable undefined in strict mode                                                                           │
├────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ engine262      │                                                                                                                             │
│ Moddable XS    │                                                                                                                             │
├────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ Hermes         │                                                                                                                             │
│                │ ReferenceError: Property 'x' doesn't exist                                                                                  │
├────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ JavaScriptCore │ /var/folders/_s/v0bdf8sd237bfp_kw4cyx8100000gp/T/9nVbA2j8YmPietLJklnz/f-1594694938814-48385-yr5nrk.8osm.js:11:5             │
│                │ global code@/var/folders/_s/v0bdf8sd237bfp_kw4cyx8100000gp/T/9nVbA2j8YmPietLJklnz/f-1594694938814-48385-yr5nrk.8osm.js:12:3 │
│                │ ReferenceError: Can't find variable: x                                                                                      │
├────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ SpiderMonkey   │                                                                                                                             │
│                │ ReferenceError: assignment to undeclared variable x                                                                         │
├────────────────┼─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
│ V8             │                                                                                                                             │
│                │ ReferenceError: x is not defined                                                                                            │
└────────────────┴─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

I'm not sure where is ReferenceError... let me try this again...

leobalter commented 4 years ago

@allenwb @anba, can you help me here, please?

leobalter commented 4 years ago

I wonder if the IsUnresolvableReference should be explicit about the base value component of V being dynamic as in: the current base value component of V.

The engine results throwing a RefError make me think this is the case.

In my brain, the base value component is not changed within the current spec text. Somehow it persists when we fetched that Reference (before the delete).

cc @tc39/ecma262-editors.

bakkot commented 4 years ago

@leobalter I believe your reading of the spec is correct: that is, in the current spec, there is no reference error. Per that esdiscuss thread, this is a bug in the specification. I (apparently) opened a bugzilla issue to track this, and it never got fixed. But engines still don't pass the tests (e.g. v8 xfails them) and we should fix it upstream. Opened https://github.com/tc39/ecma262/issues/2093 to track.

leobalter commented 4 years ago

I have a tentative PR Draft in https://github.com/tc39/ecma262/pull/2094 but I need to follow up investigating usage of PutValue to confirm it works.

devsnek commented 4 years ago

More invalid tests:

language/expressions/compound-assignment/S11.13.2_A5.1_T4.js language/expressions/compound-assignment/S11.13.2_A5.4_T4.js language/expressions/compound-assignment/S11.13.2_A5.7_T4.js language/expressions/compound-assignment/S11.13.2_A5.10_T4.js language/expressions/compound-assignment/S11.13.2_A5.11_T4.js language/expressions/compound-assignment/S11.13.2_A5.2_T4.js language/expressions/compound-assignment/S11.13.2_A5.3_T4.js language/expressions/compound-assignment/S11.13.2_A5.5_T4.js language/expressions/compound-assignment/S11.13.2_A5.6_T4.js language/expressions/compound-assignment/S11.13.2_A5.9_T4.js language/expressions/compound-assignment/S11.13.2_A5.8_T4.js language/expressions/assignment/S11.13.1_A5_T4.js language/expressions/prefix-increment/S11.4.4_A5_T4.js language/expressions/postfix-increment/S11.3.1_A5_T4.js language/expressions/prefix-decrement/S11.4.5_A5_T4.js language/expressions/postfix-decrement/S11.3.2_A5_T4.js

tc39 / test262

Writing to deleted global variable references has been classified as a spec bug #427