add support of codefix for Strict Class Initialization by Kingwl · Pull Request #21528 · microsoft/TypeScript

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

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

@@ -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

+    }
+
+    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

+        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

+
+                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

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


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

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

+
+    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

+            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

+        }
+        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

+        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

+            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

+                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

        "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

+            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

+        },
+        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

+        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

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


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

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

+        "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

+            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

+////     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

+        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

+                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

+        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.

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