Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Verify if Binding is preserved for obj's SetMutableBinding #2094

Merged
merged 1 commit into from
Jul 30, 2020

Conversation

leobalter
Copy link
Member

@leobalter leobalter commented Jul 14, 2020

Verify binding existence in the object's SetMutableBinding on Strict Mode.

Closes #2093
Ref #467
Ref tc39/test262#427

@bakkot
Copy link
Contributor

bakkot commented Jul 14, 2020

Note that the HasBinding call is observable. For example, consider

let scope = { x: 1 };
let handler = {
  has(o, p) {
    if (p === 'x') {
      console.log('hit', Reflect.has(o, p));
    }
    return Reflect.has(o, p);
  },
};
with (new Proxy(scope, handler)) {
  (function () {
    'use strict';
    x = (delete scope.x, 2);
  })();
}

What should this print / do? In Chrome it prints hit, false and throws; in Firefox it prints hit, true and does not throw; in Safari it prints hit, true; hit, false and throws.

For a trickier example, consider

let scope = { x: 1 };
let first = true;
let handler = {
  has(o, p) {
    if (p === 'scope') {
      return false;
    }
    console.log('hit');
    if (first) {
      first = false;
      return true;
    }
    return false;
  },
};
with (new Proxy({ x: 1 }, handler)) {
  (function () {
    'use strict';
    x = 2;
  })();
}

where here the call to HasBinding is the thing which causes the property to not exist!

@anba
Copy link
Contributor

anba commented Jul 14, 2020

Can't we perform this check as part of 8.1.1.2.5 SetMutableBinding as proposed in the es-discuss thread? That way it nicely lines up with other strict mode checks in environment records, like for example 8.1.1.2.6 GetBindingValue, step 4 or 8.1.1.1.5 SetMutableBinding, step 2.

@leobalter
Copy link
Member Author

@anba @bakkot how does it look now?

We are still caught by Proxies [[HasProperty]] that can falsely report as an existing property. IMO, it sounds like not a problem for the specs but user land.

@leobalter leobalter marked this pull request as ready for review July 14, 2020 17:54
@leobalter leobalter changed the title Normative: Verify if Binding is preserved for PutValue Normative: Verify if Binding is preserved for obj's SetMutableBinding Jul 14, 2020
@ljharb ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Jul 14, 2020
@anba
Copy link
Contributor

anba commented Jul 16, 2020

LGTM!

FWIW the proxy issue actually also applies to 8.1.1.2.6 GetBindingValue.

@ljharb ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jul 21, 2020
@leobalter
Copy link
Member Author

This has reached consensus today. We need to update the current tests - starting with those pointed out at tc39/test262#427 - and also verify observable implications with Proxies.

Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm editorially

@ljharb ljharb requested review from michaelficarra, bakkot and a team July 29, 2020 22:30
@ljharb ljharb self-assigned this Jul 29, 2020
spec.html Outdated Show resolved Hide resolved
@leobalter

This comment has been minimized.

@ljharb ljharb force-pushed the 2093/putvalue-strict-check branch from ea73966 to 019d498 Compare July 30, 2020 19:55
…tc39#2094)

Verify binding existence in the object's SetMutableBinding on Strict Mode.

Closes tc39#2093
Ref tc39#467
Ref tc39/test262#427
@ljharb ljharb merged commit 019d498 into tc39:master Jul 30, 2020
@leobalter leobalter deleted the 2093/putvalue-strict-check branch September 15, 2020 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assignment of global variables deleted by their RHS in strict mode is permitted
7 participants