add support of codefix for Strict Class Initialization #21528

Kingwl · 2018-02-01T06:51:33Z

mhegazy · 2018-02-02T17:41:56Z

src/services/codefixes/fixAddMissingPropertyInitializer.ts

@@ -0,0 +1,112 @@
+/* @internal */
+namespace ts.codefix {


you do not need to split them into 3 fixes, one code fix can return multiple actions. for instance see https://github.com/Microsoft/TypeScript/blob/master/src/services/codefixes/fixUnusedIdentifier.ts#L11 we will add two actions possibly, one for _ prefix, and one for deleting the declaration, and the user gets to choose.

mhegazy · 2018-02-02T17:42:23Z

@Andy-MS can you please review

ghost

Could also use a test that isn't codeFixAll.

ghost · 2018-02-02T18:28:02Z

src/services/codefixes/fixAddMissingDefiniteAssignmentAssertions.ts

+    }
+
+    function addDefiniteAssignmentAssertion(changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): void {
+        const property = clone(propertyDeclaration);


This could use updateProperty (once #21577 is in)

ghost · 2018-02-02T18:51:13Z

src/services/codefixes/fixAddMissingDefiniteAssignmentAssertions.ts

+        return { token, propertyDeclaration };
+    }
+
+    function getActionsForAddMissingDefiniteAssignmentAssertion (context: CodeFixContext, token: Identifier, propertyDeclaration: PropertyDeclaration): CodeFixAction[] | undefined {


This function could be inlined.

thanks for your review
i have not inline this, because it looks clear 😃

ghost · 2018-02-02T18:52:42Z

src/services/codefixes/fixAddMissingDefiniteAssignmentAssertions.ts

+
+                const { token, propertyDeclaration } = info;
+                if (!addToSeen(seenNames, token.text)) {
+                    return;


Nit: No need for early return if there's only one line following it, then you could just put that line inside the if and negate the condition.

ghost · 2018-02-02T18:53:23Z

src/services/codefixes/fixAddMissingDefiniteAssignmentAssertions.ts

+        },
+    });
+
+    interface Info { token: Identifier; propertyDeclaration: PropertyDeclaration; }


Nit: Not much point returning token since that's just propertyDeclaration.name.

ghost · 2018-02-02T18:54:39Z

src/services/codefixes/fixAddMissingDefiniteAssignmentAssertions.ts

+
+    interface Info { token: Identifier; propertyDeclaration: PropertyDeclaration; }
+    function getInfo (sourceFile: SourceFile, pos: number): Info | undefined {
+        const token = getTokenAtPosition(sourceFile, pos, /*includeJsDocComment*/ false);


return isIdentifier(token) ? cast(token.parent, isPropertyDeclaration) : undefined;.

ghost · 2018-02-02T18:56:48Z

src/services/codefixes/fixAddMissingPropertyInitializer.ts

+            return createLiteral((<LiteralType>type).value);
+        }
+        else if (type.getFlags() & TypeFlags.Union) {
+            for (const t of (<UnionType>type).types) {


return firstDefined((<UnionType>type).types, t => getDefaultValueFromType(checker, t));

ghost · 2018-02-02T18:59:22Z

src/services/codefixes/fixAddMissingPropertyInitializer.ts

+        }
+        else if (type.getFlags() & TypeFlags.Object) {
+            const objectType = <ObjectType>type;
+            if (getObjectFlags(objectType) & ObjectFlags.Class) {


getObjectFlags will handle any type as input, so no need for if (type.getFlags() & TypeFlags.Object) check first. You don't need objectType since none of the uses of type require a more specific type.

ghost · 2018-02-02T19:00:07Z

src/services/codefixes/fixAddMissingPropertyInitializer.ts

+        else if (type.getFlags() & TypeFlags.Object) {
+            const objectType = <ObjectType>type;
+            if (getObjectFlags(objectType) & ObjectFlags.Class) {
+                const classDeclaration = <ClassLikeDeclaration>getClassLikeDeclarationOfSymbol(objectType.symbol);


I think the return type of getClassLikeDeclarationOfSymbol should be changed to ClassLikeDeclaration | undefined.

ghost · 2018-02-02T19:01:24Z

src/services/codefixes/fixAddMissingPropertyInitializer.ts

+            if (getObjectFlags(objectType) & ObjectFlags.Class) {
+                const classDeclaration = <ClassLikeDeclaration>getClassLikeDeclarationOfSymbol(objectType.symbol);
+                const constructorDeclaration = find<ClassElement, ConstructorDeclaration>(classDeclaration.members, (m): m is ConstructorDeclaration => isConstructorDeclaration(m) && !!m.body)!;
+                if (constructorDeclaration &&


You might also check that the class isn't abstract. Also, a class with no explicit constructor declaration has an implicit one with no parameters.

ghost · 2018-02-02T19:02:38Z

src/services/codefixes/fixAddMissingPropertyInitializer.ts

+                const classDeclaration = <ClassLikeDeclaration>getClassLikeDeclarationOfSymbol(objectType.symbol);
+                const constructorDeclaration = find<ClassElement, ConstructorDeclaration>(classDeclaration.members, (m): m is ConstructorDeclaration => isConstructorDeclaration(m) && !!m.body)!;
+                if (constructorDeclaration &&
+                    (!constructorDeclaration.parameters || !constructorDeclaration.parameters.length) &&


parameters should always be defined. Don't think you need to look at typeParameters since those are illegal on a constructor.

ghost · 2018-02-05T16:03:02Z

src/compiler/diagnosticMessages.json

        "category": "Message",
        "code": 95017
+    },
+    "add Undefined type to property '{0}'": {


Nit: Start with a capital letter, but don't capitalize other things (e.g. say "definite assignment assertion" not "Definite Assignment Assertion".) Also put 'undefined' in single quotes.

ghost · 2018-02-05T16:14:24Z

src/services/codefixes/fixStrictClassInitialization.ts

+            if (!propertyDeclaration) return undefined;
+
+            const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options);
+            return [


getActionsForAddMissingInitializer is the only one that can be undefined. Maybe you could use ...optionToArray(getActionsForMissingInitializer(...)) (#21628) instead of filter.

ghost · 2018-02-05T16:16:51Z

src/services/codefixes/fixStrictClassInitialization.ts

+        },
+        fixIds: [fixIdAddDefiniteAssignmentAssertions, fixIdAddUndefinedType, fixIdAddInitializer],
+        getAllCodeActions: context => {
+            const seenNames = createMap<true>();


Are you sure we need this? I don't think we would have two of these errors for the same property. But we might get this error for two separate properties that happen to share a name, which would result in only one fix here.

ghost · 2018-02-05T16:19:20Z

src/services/codefixes/fixStrictClassInitialization.ts

+        else if (type.flags & TypeFlags.Union) {
+            return firstDefined((<UnionType>type).types, t => getDefaultValueFromType(checker, t));
+        }
+        else if (type.flags & TypeFlags.Object) {


Earlier comment marked as out of date, so copying here: getObjectFlags will handle any type as input, so no need for if (type.getFlags() & TypeFlags.Object) check first.

ghost · 2018-02-05T16:20:13Z

tests/cases/fourslash/codeFixClassPropertyInitializationWithDefiniteAssignmentAssertions.ts

+////     foo: T;
+//// }
+
+verify.codeFixAll({


Also add tests using verify.codeFixAvailable instead of verify.codeFixAll.

ghost · 2018-02-05T16:21:58Z

src/compiler/diagnosticMessages.json

+        "category": "Message",
+        "code": 95018
+    },
+    "add default Initializer to property '{0}'": {


Nit: I think this can just say "initializer", not "default initializer".

Kingwl · 2018-02-18T10:54:26Z

ping @Andy-MS
hello... still need review😅

ghost · 2018-02-20T21:17:16Z

src/services/codefixes/fixStrictClassInitialization.ts

+            return firstDefined((<UnionType>type).types, t => getDefaultValueFromType(checker, t));
+        }
+        else {
+            if (getObjectFlags(type) & ObjectFlags.Class) {


Nit: Use else if

ghost · 2018-02-20T21:18:56Z

tests/cases/fourslash/codeFixClassPropertyInitializationWithDefiniteAssignmentAssertions.ts

+////     foo: T;
+//// }
+
+verify.codeFixAvailable();


Also test the content of the code fix -- use verify.codeFix and verify.codeFixAll (in separate tests).

ghost · 2018-02-20T21:19:52Z

src/services/codefixes/fixStrictClassInitialization.ts

+        errorCodes,
+        getCodeActions: (context) => {
+            const propertyDeclaration = getPropertyDeclaration(context.sourceFile, context.span.start);
+            if (!propertyDeclaration) return undefined;


Nit: Just return; not return undefined; since this is a void function.

ghost · 2018-02-20T21:20:23Z

src/services/codefixes/fixStrictClassInitialization.ts

+                getActionsForAddMissingDefiniteAssignmentAssertion(context, propertyDeclaration, newLineCharacter)
+            ];
+            let action: CodeFixAction | undefined;
+            if ((action = getActionsForAddMissingInitializer(context, propertyDeclaration, newLineCharacter))) result.push(action);


append(result, getActionsForAddMissingInitializer(context, propertyDeclaration, newLineCharacter)))

ghost · 2018-02-20T21:21:02Z

src/services/codefixes/fixStrictClassInitialization.ts

+        return isIdentifier(token) ? cast(token.parent, isPropertyDeclaration) : undefined;
+    }
+
+    function getActionsForAddMissingDefiniteAssignmentAssertion (context: CodeFixContext, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): CodeFixAction {


Name the function getActionFor not getActionsFor if it doesn't return an array

… codefix-with-strictPropertyInitialization

ghost · 2018-02-22T15:14:01Z

Could you merge from master again and fix the lint failure in fixStrictClassInitialization.ts?

ghost · 2018-02-23T15:25:47Z

Thanks!

Kingwl changed the title ~~[WIP] add support of add undefined type to propertyDeclaration~~ add support of add undefined type to propertyDeclaration Feb 2, 2018

Kingwl changed the title ~~add support of add undefined type to propertyDeclaration~~ add support of codefix for Strict Class Initialization Feb 2, 2018

mhegazy reviewed Feb 2, 2018

View reviewed changes

mhegazy requested a review from a user February 2, 2018 17:42

ghost reviewed Feb 2, 2018

View reviewed changes

王文璐 added 5 commits February 5, 2018 10:14

add support of add undefined type to propertyDeclaration

50abfec

add support of add Definite Assignment Assertions to propertyDeclaration

a5de740

add support of add Initializer to propertyDeclaration

da787d2

remove useless parameter

6ab8373

fix PropertyDeclaration emit missing exclamationToken

5583bad

Kingwl force-pushed the codefix-with-strictPropertyInitialization branch from 57b1033 to 8dd5037 Compare February 5, 2018 04:14

ghost reviewed Feb 5, 2018

View reviewed changes

Kingwl force-pushed the codefix-with-strictPropertyInitialization branch 2 times, most recently from 984c2c8 to 4b1e444 Compare February 6, 2018 02:49

ghost reviewed Feb 20, 2018

View reviewed changes

merge fixes and fix

5d1bf55

Kingwl force-pushed the codefix-with-strictPropertyInitialization branch from 4b1e444 to 5d1bf55 Compare February 22, 2018 09:18

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

602c81d

… codefix-with-strictPropertyInitialization

ghost approved these changes Feb 22, 2018

View reviewed changes

Kingwl added 2 commits February 23, 2018 09:40

Merge branch 'master' into codefix-with-strictPropertyInitialization

9db9a99

fix unnecessary type assert

531e27b

ghost merged commit 30a96ba into microsoft:master Feb 23, 2018

Kingwl deleted the codefix-with-strictPropertyInitialization branch February 24, 2018 01:57

microsoft locked and limited conversation to collaborators Jul 3, 2018

This pull request was closed.

add support of codefix for Strict Class Initialization #21528

add support of codefix for Strict Class Initialization #21528

Uh oh!

Conversation

Kingwl commented Feb 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Feb 2, 2018

Uh oh!

ghost left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kingwl commented Feb 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Feb 22, 2018

Uh oh!

ghost commented Feb 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants