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

Proposal: Follow program flow in constructor when strictPropertyInitialization = true #30462

Open
5 tasks done
SetTrend opened this issue Mar 18, 2019 · 11 comments
Open
5 tasks done
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@SetTrend
Copy link

Search Terms

constructor strictPropertyInitialization flow

Suggestion

TypeScript should follow unconditional function calls in constructor when checking for strict property initialization.

Use Cases

It is a common pattern to be able to reset objects after construction and, thus, to transfer object initialization to a separate routine:

class C
{
  private p: number;

  public constructor() { Initialize() }

  public Initialize() { p = 0; }
}

Currently, TypeScript doesn't consider functions being unconditionally called from within a constructor function when checking for strict property initialization. The above code yields a compiler error.

I propose TypeScript to be able to analyse code flow and recognize property initialization that's performed in functions unconditionally called by the constructor function.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@SetTrend SetTrend changed the title follow program flow in constructor when strictPropertyInitialization = true Proposal: Follow program flow in constructor when strictPropertyInitialization = true Mar 18, 2019
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 18, 2019
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Mar 18, 2019

I've heard C# is looking into this so it's worth discussing why we can't do the same.

@ahejlsberg
Copy link
Member

Beyond the complexity of doing inter-procedural control flow analysis, an issue here is that every method in JavaScript is virtual, so we really have no idea where a call to this.initialize() goes (a subclass may override it).

@SetTrend
Copy link
Author

SetTrend commented Mar 19, 2019

I see.

Yet, even in that case, wouldn't it be possible for the derived class to check whether it overrides a function that's called by a base class constructor and go through that path for validation?

image

Perhaps, in a first iteration, it would suffice to just check for functions immediately called by a constructor - not following the full execution path.

@SetTrend
Copy link
Author

I just stumbled over a similar issue, where the base class is abstract and generic:

export default abstract class A<T>
{
  protected _state: T;
}

... results in:

TS2564: Property '_state' has no initializer and is not definitely assigned in the constructor.

You may call this a design flaw as a constructor like:

protected constructor(state: T)
{
  super();
  this._state = state;
}

... may solve the problem. Though, basically, it would be plain correct to omit this redundant constructor.

This error should not occur in an abstract class. It should, however, occur in a non-abstract derived class if all base class constructors and the constructor of the first non-abstract derived class don't set the protected field.

@1valdis
Copy link

1valdis commented Jan 30, 2020

I just ran into that and I was surprised that method call wouldn't be considered an initialization. Seems like I will have to have a bit of duplicated code.
Edit: managed to workaround by putting ! after property name. Will do for now, but it probably would look better without that.

@beauxq
Copy link

beauxq commented Nov 8, 2020

This is not only for the false positive pointed out by the author.

This could also prevent a false negative on a null pointer exception (Property is used before being assigned. ts(2565)) that TypeScript is not currently catching.
See: #41446

@beauxq
Copy link

beauxq commented Nov 8, 2020

Beyond the complexity of doing inter-procedural control flow analysis

Here is an example with both the false positive and the false negative in the same example:

class A {}

class B {
    public foo() {}
}

class C {
    private a: A;  // false positive - `a` is definitely assigned in the constructor
    private b: B;
    constructor() {
        const x = 3;
        this.reset(x);
        this.b = new B();
    }

    private reset(y: number) {
        console.log(y);
        this.a = new A();
        console.log(this.a);
        this.b.foo();  // false negative - `b` used before being assigned
    }
}

Note that both problems (the false negative and the false positive) go away if you pretend that the code looks like this:

class A {}

class B {
    public foo() {}
}

class C {
    private a: A;  // false positive - `a` is definitely assigned in the constructor
    private b: B;
    constructor() {
        const x = 3;
        this.reset(x);
    ((y: number) => {  // copy parameters from the method to here
        // exact copy of the function body
        console.log(y);
        this.a = new A();
        console.log(this.a);
        this.b.foo();  // false negative - `b` used before being assigned
    })(x);  // copy the arguments from the function call to here
        this.b = new B();
    }

    private reset(y: number) {
        console.log(y);
        this.a = new A();
        console.log(this.a);
        this.b.foo();  // false negative - `b` used before being assigned
    }
}

Maybe Typescript could pretend that the code looks like that just for error checking.
For reporting the errors, it would just have to translate line numbers. (That's why I used the weird indentation.)

@fetis
Copy link

fetis commented Apr 23, 2021

Without that feature, the usage of strictPropertyInitialization is questionable for me. We've just enabled it in a quite large project and we had to move code from our private initialization methods back to the constructor because TS wasn't able to properly recognize initialization. This made code less readable, which was not my intention at all.

@paul-marechal
Copy link

Same over here, I was surprised to see that strictPropertyInitialization checked all branches of the constructor to make sure that the assignment happens, but didn't bother checking function calls. Putting aside complex recursive functions, something simple should work no?

@clshortfuse
Copy link

Oddly enough, TS emits more errors than just regular JS when using the same code. strictPropertyInitialization has no effect.

Javascript checking is actually pretty good with declaring types, even catching .prototype changes and constructor, where TS doesn't. Right now, I'm just trying to get it to read declarations in static initialization blocks. It's like it's doing it out of order. Those should be first. Tracing Web Component generation yields:

1 'Super Static Init'
2 'Super Static Field'
3 'Super post-declaration'
4 'Sub Static Init'
5 'Sub Static Field'
6 'Sub post-declaration'
6.5 'HTMLElement reads observedAttributes from subclass'
Element added to registry
7 'Subclass constructor: pre-super()'
8 'Superclass constructor: pre-super()'
9 'Superclass Instance Field'
10 'Superclass constructor: post-super()'
11 'Subclass Instance Field'
12 'Subclass constructor: post-super()'
Element created
13 'Attributes observed'
Attribute added

(See #23217 (comment)) You can't really rely on setting variables inside the constructor because that's pretty late in the Web Component generation. (eg: want to construct observedAttributes based on instance properties).

@theonlypwner
Copy link

theonlypwner commented Apr 17, 2023

@beauxq Apparently, the "false positive" and "false negative" are already known limitations in #20075 (comment), and the workaround for the false positive is to use ! to suppress error.

For false negatives, I hope we can detect something like this:

Playground: https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgJICFkG9kFsJgAWA9gCYAUAlAFzIBuxwpyAvgFBsIA2cAzr8gCCyYLgAOXCPnACM2NskXIARsgC8yEBADuydOSLBelBUvxEyVbMkO8AdMrvmSFSqzbtOPfnpHjJ0mCymFimigjEILxgUACuCGDEUOTKtBhuoUpZKk4ELlYA9AXICFB8hBACsSCSPhG4gRDMxLFgYe5ZzpYZJZG8xJJ2XMQA5uQADG7snlq6glS5Fq5sQA

interface IB { method(): void }

class A implements IB {
    b = new B(this)
    method() { this.b.method() } // delegation pattern
}

class B implements IB {
    constructor(b: IB) {
        b.method() // crashes unless commented out
    }
    method() { console.log(0) }
}

new A().method()

For false positives, I hope we can at least handle a basic case where the constructor calls some init function.

class A {
    a: number
    constructor () { this.reset() }
    reset() { this.a = 1 }
}

This avoids the duplication of code when there are many members to reset, which would look like this:

class A {
    a = 1
    b = 2
    // ...
    reset() {
        this.a = 1
        this.b = 1
        // ...
    }
}

But we'd also have to check when a subclass overrides the init function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

9 participants