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

Provide a quick fix for uninitalized class properties #21509

Closed
DanielRosenwasser opened this issue Jan 31, 2018 · 14 comments
Closed

Provide a quick fix for uninitalized class properties #21509

DanielRosenwasser opened this issue Jan 31, 2018 · 14 comments
Labels
Domain: Quick Fixes Editor-provided fixes, often called code actions. Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript

Comments

@DanielRosenwasser
Copy link
Member

class Point {
    z: string;
    constructor() {
    }
}

We should provide a quick fix here to provide a definite assignment assertion on z under --strictPropertyInitialization. Bonus points if we only provide the quick fix when the property has actually been written to!

@DanielRosenwasser DanielRosenwasser added Bug A bug in TypeScript Domain: Quick Fixes Editor-provided fixes, often called code actions. labels Jan 31, 2018
@mhegazy mhegazy added Suggestion An idea for TypeScript Help Wanted You can do this and removed Bug A bug in TypeScript labels Jan 31, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Jan 31, 2018

I am assuming we will provide 3 actions:

  • Add definite assignment operator (assuming it was written to at least once in the class)
  • Add an initialization in the constructor (assuming we can initialize it, e.g. string, number, or a type that has a constructor)
  • Add | undefined to the type declaration
@Kingwl
Copy link
Contributor

Kingwl commented Feb 1, 2018

I'd like to work on this 😃

@mhegazy
Copy link
Contributor

mhegazy commented Feb 1, 2018

Go for it!

@Kingwl
Copy link
Contributor

Kingwl commented Feb 1, 2018

Add definite assignment operator (assuming it was written to at least once in the class)
Add an initialization in the constructor (assuming we can initialize it, e.g. string, number, or a type that has a constructor)

How should we set their initializer expression?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 1, 2018

something like https://github.com/Microsoft/TypeScript/blob/master/src/services/codefixes/fixAddMissingMember.ts#L105

first there has to be a constructor. I would ignore the case where there is no constructor at the moment.

@Kingwl
Copy link
Contributor

Kingwl commented Feb 2, 2018

ping @mhegazy
I'm not sure I understand your meaning correctly

for example:
https://www.typescriptlang.org/play/index.html#src=class%20TT%20%7B%0D%0A%7D%0D%0A%0D%0Aclass%20T%20%7B%0D%0A%20%20%20%20c%3A%20number%3B%0D%0A%20%20%20%20b%3A%20TT%3B%0D%0A%20%20%20%20a%3A%20string%3B%0D%0A%7D%0D%0A

class TT {
}

class T {
    c: number;
    b: TT;
    a: string;
}

what should class T props do after codefix?

class T {
    c: number | undefined = undefined;
    b: TT | undefined = undefined;
    a: string | undefined = undefined;
}

or

class T {
    c: number = 0;
    b: TT = new TT();
    a: string = "";
}

or other way?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

you do not need the = undefined.

I think we should have 3 code actions:

  1. add | undefined
class T {
    c: number | undefined ;
    b: TT | undefined ;
    a: string | undefined;
}
  1. add an intializer
class T {
    c: number = 0 ;
    b: TT;
    a: string = "";
}

or 3. add a definite assignment operator

class T {
    c!: number;
    b!: TT;
    a!: string;
}
@Kingwl
Copy link
Contributor

Kingwl commented Feb 2, 2018

thanks for your response

and if the TT has the constructor

class TT {
    constructor () {}
}

then should i create a instance of TT in the prop b?


class T {
    c: number = 0 ;
    b: TT = new TT(); // this
    a: string = "";
}

@mhegazy
Copy link
Contributor

mhegazy commented Feb 2, 2018

then should i create a instance of TT in the prop b?

Sounds reasonable

@Kingwl
Copy link
Contributor

Kingwl commented Feb 2, 2018

i have finished codefix work 😄
review required

@ghost ghost closed this as completed in #21528 Feb 23, 2018
@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Feb 27, 2018
@mhegazy mhegazy added this to the TypeScript 2.8 milestone Feb 27, 2018
@oleg-codaio
Copy link

Nice! Instead of "Add 'undefined' type to property '{0}'", did you consider making the property optional via, e.g., prop?: string;?

@ghost
Copy link

ghost commented Mar 8, 2018

@vaskevich I think that would be a bad idea since those would be less compatible with javascript's own class fields (https://github.com/tc39/proposal-class-fields), which IIRC are always initialized to undefined, never missing.

@SlurpTheo
Copy link

We recently (probably with 2.7's --strictPropertyInitialization) discovered a problem where we thought we had declared an optional property:

  c: () => number | undefined;

But that's actually:

  c: () => (number | undefined);

And that's harder to "back-into" had we just used this syntax from the get-go:

  c?: () => number;
@DanielRosenwasser
Copy link
Member Author

@vaskevich @SlurpTheo I'd recommend filing a new issue; if a lot of users end up requesting that, we can consider it.

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Domain: Quick Fixes Editor-provided fixes, often called code actions. Fixed A PR has been merged for this issue Help Wanted You can do this Suggestion An idea for TypeScript
5 participants