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

No error when initializing a class field defined as a read-only getter in base class #34585

Closed
trusktr opened this issue Oct 18, 2019 · 10 comments · Fixed by #37894
Closed

No error when initializing a class field defined as a read-only getter in base class #34585

trusktr opened this issue Oct 18, 2019 · 10 comments · Fixed by #37894
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.

Comments

@trusktr
Copy link
Contributor

trusktr commented Oct 18, 2019

TypeScript Version: 3.6.4

Search Terms:

"Related Issues" feature of GitHub uses the title as search terms.

Code

class Base {
  get foo() {
    return 5
  }
}

class Child extends Base {
  foo = 10
}

new Child // runtime error!

Expected behavior:

Type error during compile

Actual behavior:

No error until runtime.

Playground Link:

playground

Related Issues:

Might be duplicate of #13347, but not sure.

@AviVahl
Copy link

AviVahl commented Oct 18, 2019

Checked this snippet of code directly in Chrome, and there's no runtime error when evaluating new Child. I get a new Child instance with a foo field === 10, and it is assignable (unlike instances of new Base).
do you mean you get a runtime error if you transpile this using typescript (to es5) and then run?

I believe this issue has to do with: #27644

@fatcerberus
Copy link

I don't know how TypeScript is supposed to handle this, but classfields in ES use [[Define]] (think Object.defineProperty) which bypasses the setter in the base class, so this is legal JS; that's probably why you're not getting a runtime error in Chrome @AviVahl.

@RyanCavanaugh RyanCavanaugh changed the title superclass getter incorrectly overridden with non-getter in child class. No error when initializing a class field defined as a read-only getter in base class Oct 24, 2019
@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Oct 24, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.8.0 milestone Oct 24, 2019
@trusktr
Copy link
Contributor Author

trusktr commented Dec 3, 2019

@AviVahl Yeah, exactly, if TS compiles to the form

constructor() {
  this.foo = 10
}

then it doesn't work. This can happen with either es5 target, or the new "useDefineForClassFields": false option (I haven't tested it with the latter).

@RyanCavanaugh RyanCavanaugh added Needs Investigation This issue needs a team member to investigate its status. and removed Bug A bug in TypeScript labels Feb 25, 2020
@sandersn
Copy link
Member

sandersn commented Apr 7, 2020

This is an error with useDefineForClassFields: true, which will be the default target for target: "esnext" when class fields reach stage 4. That's because it's quite confusing, even though it's legal [stage 3] JS. It could always be an error, but this is a problem that real code rarely encounters. When I looked at our user test suite, the only examples I found were in old, pre-readonly code, and I believe Google and Bloomberg found the same to be true on their internal code bases.

This would be a nice feature but not required, and fairly easy to implement since the error is already there, so I'm going to move this into the backlog and put the Help Wanted label on it. It's a good feature if you want to focus on identifying breaks in the ecosystem.

Edit: Well, I thought about it a bit more and decided to at least create the PR. There are lots of breaks in the test suite because we test exactly this weird case a lot, so I'll put it up tomorrow after verifying that they're correct.

@trusktr
Copy link
Contributor Author

trusktr commented Aug 4, 2020

@sandersn Thanks for the fix! Btw, I see the error message is

'foo' is defined as an accessor in class 'Base', but is overridden here in 'Child' as an instance property.

in 4-beta playground even if useDefineForClassFields is disabled and I have a setter. Isn't that code legal when useDefineForClassFields is false?

That is amazing that when useDefineForClassFields is set to true that there is a type error on accessor overrides! Very nice in helping people avoid problems.

@sandersn
Copy link
Member

sandersn commented Aug 5, 2020

We decided to disallow properties overriding accessors in 4.0 for everyone, so that when class fields finally do move to stage 4, any code that could get tripped up by the [[Set]] -> [[Define]] migration would have time to get fixed.

@ntucker
Copy link

ntucker commented Aug 12, 2020

what's wrong with this if the Child class marks it as readonly?

@sandersn
Copy link
Member

There's still some kind of initialisation, and whether that affects base accessor depends if you are compiling with [[Set]] (classic TS) or [[Define]] (ESNext) semantics. For example:

class Base {
  get foo() {
    return 5
  }
}

class Child extends Base {
  readonly foo = 10
}

Still crashes with [[Set]] semantics, because the emitted code has a this.foo = 10:

class Base {
    get foo() {
        return 5;
    }
}
class Child extends Base {
    constructor() {
        super(...arguments);
        this.foo = 10;
    }
}

@isral
Copy link

isral commented Jul 14, 2021

@sandersn
I have setter only property
But the code that get the property is not considered compile error (return undefined at runtime).
Is this related to this bug? Or should I issue a new bug?
Typescript 4.3.2 (VS Code) 4.3.5 (Command Prompt).

@sandersn
Copy link
Member

@isral Please do open a new bug. It's a lot easier with an example to look at.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants