Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Normative: End this TDZ before running initializers #92

Merged
merged 1 commit into from
Apr 11, 2018
Merged

Conversation

littledan
Copy link
Member

@littledan littledan commented Apr 6, 2018

Previously, the TDZ of this in a subclass constructor ended when
the super() call was entirely finished. However, this is visible
as not being in TDZ running initializers. For example:

{
  let f;
  class A extends (class {}) {
    x = (this, f());
    constructor() { f = () => this; super(); }
  };
  new A;
}

Previously, f() would lead to a ReferenceError in this case, while
this would evaluate to the new instance under construction.

This patch tweaks the TDZ to end a little bit sooner, so that this
is defined inside of the constructor's scope before the initializers
are run. In the above example, there would be no ReferenceError
under the semantics in this patch.

Thanks to Caitlin Potter, Kevin Gibbons and Jordan Harband for
contributing to the discussion that led to this change.

Previously, the TDZ of `this` in a subclass constructor ended when
the `super()` call was entirely finished. However, `this` is visible
as not being in TDZ running initializers. For example:

{
  let f;
  class A extends (class {}) {
    x = (this, f());
    constructor() { f = () => this; super(); }
  };
  new A;
}

Previously, `f()` would lead to a ReferenceError in this case, while
`this` would evaluate to the new instance under construction.

This patch tweaks the TDZ to end a little bit sooner, so that `this`
is defined inside of the constructor's scope *before* the initializers
are run. In the above example, there would be no ReferenceError
under the semantics in this patch.

Thanks to Caitlin Potter, Kevin Gibbons and Jordan Harband for
contributing to the discussion that led to this change.
@littledan
Copy link
Member Author

cc @ljharb @caitp @bakkot

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM!

@caitp
Copy link

caitp commented Apr 6, 2018

lgtm

@littledan
Copy link
Member Author

Wow, what a quick review! Thanks.

Would anyone be interested in writing test262 tests for this change?

@bakkot
Copy link
Contributor

bakkot commented Apr 6, 2018

@littledan Sure, give me a minute.

@gsathya
Copy link
Member

gsathya commented Apr 11, 2018

lgtm

@rwaldron
Copy link
Contributor

Tests merged tc39/test262#1507

I'm going to spend a little more time with the tests and these changes to verify coverage.

@littledan littledan merged commit e65bb38 into master Apr 11, 2018
kisg pushed a commit to paul99/v8mips that referenced this pull request Apr 17, 2018
Class fields needs to be initialized after `this` is bound, as per the
new spec change:
tc39/proposal-class-fields#92

This CL moves the initialization of `this` from parser desugaring to
the bytecode generator.

Bug: v8:7647
Change-Id: I20f749403e5a4d2f06a39726cf39012ceb541987
Reviewed-on: https://chromium-review.googlesource.com/1014383
Reviewed-by: Mythri Alle <[email protected]>
Commit-Queue: Sathya Gunasekaran <[email protected]>
Cr-Commit-Position: refs/heads/master@{#52646}
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants