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

Class fields w/esnext+[[Define]]:no shadow error #36405

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jan 24, 2020

With useDefineForClassFields: true and ESNext target, initializer expressions for property declarations are evaluated in the scope of the class body and are permitted to reference parameters or local variables of the constructor. This is different from classic Typescript behaviour, with useDefineForClassFields: false. There, initialisers of property declarations are evaluated in the scope of the constructor body, so constructor-local symbols shadow outer symbols of the same name.

In other words:

var x = 1
class C {
  y = x
  constructor(x: string) { }
}

Is fine with ESNext emit because x refers unambiguously to var x = 1. In non-esnext emit, you'll get

var x = 1
class C {
  constructor(x) {
    y = x // oh no, wrong x!
  }
}

Note that when class fields are accepted in the ECMAScript standard, we'll need to replace ESNext with the current ES20xx.

The emit is already correct for ESNext + useDefineForClassFields, so this PR just removes the error in this case. It leaves the error for other cases, because fixing classic emit is non-trivial and low-value.

Fixes #35085

With useDefineForClassFields: true and ESNext target, initializer
expressions for property declarations are evaluated in the scope of
the class body and are permitted to reference parameters or local
variables of the constructor. This is different from classic
Typescript behaviour, with useDefineForClassFields: false. There,
initialisers of property declarations are evaluated in the scope of
the constructor body.

Note that when class fields are accepted in the ECMAScript
standard, the target will become that year's ES20xx
Copy link
Contributor

@elibarzilay elibarzilay left a comment

Choose a reason for hiding this comment

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

Two quick comments:

  1. It's confusing to figure out why it works to just not throw an error, so maybe a comment would be good there?

  2. I think that it's also good to include a test that shows that the local constructor scope is not used when there's no global (which I tried, and it looks like it's working fine...). E.g.,

    class C {
        b = z; // not ok
        constructor(z: string) { }
    }
    

@sandersn sandersn merged commit 36169b4 into master Jan 29, 2020
@sandersn sandersn deleted the no-shadowing-error-for-class-fields branch January 29, 2020 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Constructor parameters interfere with bindings in field initializers
3 participants