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

Ambient public fields initialized in parent class are blocked at compile-time #35327

Closed
robpalme opened this issue Nov 25, 2019 · 6 comments · Fixed by #36112
Closed

Ambient public fields initialized in parent class are blocked at compile-time #35327

robpalme opened this issue Nov 25, 2019 · 6 comments · Fixed by #36112
Assignees
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue

Comments

@robpalme
Copy link

robpalme commented Nov 25, 2019

TypeScript Version: 3.8.0-dev.20191123

Search Terms: ambient declare public class fields parent initializer useDefineForClassFields

Code:

// @useDefineForClassFields: true
class Parent {
    a: any;
    constructor(arg: any) {
        this.a = arg;
    }
}
class Child extends Parent {
    declare a: number;
    constructor(arg: number) {
        super(arg);
        console.log(this.a);  // Property 'a' is used before being assigned. (2565)
    }
}

Expected behavior:

There should be no compile-time error, because this is legitimate ES(next) code. The intent of declare <field> is merely to specialize the type, not to impose additional initialization constraints.

Actual behavior:

The usage of this.a causes the compiler error Property 'a' is used before being assigned. (2565).

The emitted code is good. The only problem is the compile-time error.

Playground Link:

Related Issues: #34972

The main use-case for ambient public fields is to specialize parent class fields. It is common for parent class fields to only be initialized in the parent constructor, invoked via super(). Due to this bug, this pattern does not work today, forcing the use of a workaround such as using @ts-ignore or redundant re-initialization of the property in the child constructor to appease the compiler.

cc: @sandersn

@fatcerberus
Copy link

fatcerberus commented Nov 25, 2019

Interestingly, the error is correct, but only because the canonical (i.e. untransformed) emit is apparently wrong. With useDefineForClassFields: true and target: "esnext", the emitted code is:

"use strict";
// @useDefineForClassFields: true
// @target: esnext
class Parent {
    a;
    constructor(arg) {
        this.a = arg;
    }
}
class Child extends Parent {
    a;
    constructor(arg) {
        super(arg);
        console.log(this.a); // Property 'a' is used before being assigned. (2565)
    }
}
new Child(3); // prints undefined

Child#a is not actually ambient. With that emit, constructing a Child prints undefined, regardless of the value passed to the constructor. It really is used before being assigned! 😄

🏞 Playground Link

@fatcerberus
Copy link

It seems the emit was fixed post-3.7, as the ambient field isn't emitted after switching the Playground to Nightly and the code then works correctly. This does perhaps shed some light on how the error in question came to be, though.

@robpalme
Copy link
Author

You're right @fatcerberus that the original issue #34972 has now been fixed on master. Upon testing that fix I then discovered this compile-time error, so raised it here.

@fatcerberus
Copy link

It's just very interesting that the error message is correct for the original (incorrect) behavior.

@robpalme
Copy link
Author

Good point. I guess it's obvious in retrospect, because the fix only changed the transformer. There was no change to the checker.

@RyanCavanaugh RyanCavanaugh added the Bug A bug in TypeScript label Dec 9, 2019
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 3.8.0 milestone Dec 9, 2019
@robpalme
Copy link
Author

Thanks for scheduling this @RyanCavanaugh.

This feature (declare with class fields) was introduced in 3.7 and the showcase example code from the 3.7 release announcement still doesn't work due to this issue. It is currently scheduled for 3.8 - I wonder if this qualifies for patching 3.7?

@sandersn sandersn added this to Not started in Rolling Work Tracking Jan 7, 2020
@sandersn sandersn moved this from Not started to In Progress in Rolling Work Tracking Jan 10, 2020
sandersn added a commit that referenced this issue Jan 10, 2020
Previously these were incorrectly treated just like normal properties:

```ts
class Parent {
    a: any;
    constructor(arg: any) {
        this.a = arg;
    }
}
class Child extends Parent {
    declare a: number;
    constructor(arg: number) {
        super(arg);
        console.log(this.a);  // Property 'a' is used before being assigned. (2565)
    }
}
```

Fixes #35327
@sandersn sandersn added the Fix Available A PR has been opened for this issue label Jan 10, 2020
@sandersn sandersn moved this from In Progress to In Review in Rolling Work Tracking Jan 10, 2020
Rolling Work Tracking automation moved this from In Review to Done Jan 10, 2020
sandersn added a commit that referenced this issue Jan 10, 2020
Previously these were incorrectly treated just like normal properties:

```ts
class Parent {
    a: any;
    constructor(arg: any) {
        this.a = arg;
    }
}
class Child extends Parent {
    declare a: number;
    constructor(arg: number) {
        super(arg);
        console.log(this.a);  // Property 'a' is used before being assigned. (2565)
    }
}
```

Fixes #35327
Kingwl pushed a commit to Kingwl/TypeScript that referenced this issue Mar 4, 2020
Previously these were incorrectly treated just like normal properties:

```ts
class Parent {
    a: any;
    constructor(arg: any) {
        this.a = arg;
    }
}
class Child extends Parent {
    declare a: number;
    constructor(arg: number) {
        super(arg);
        console.log(this.a);  // Property 'a' is used before being assigned. (2565)
    }
}
```

Fixes microsoft#35327
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A bug in TypeScript Fix Available A PR has been opened for this issue
Projects
Development

Successfully merging a pull request may close this issue.

4 participants