fix refactor

microsoft · Feb 26, 2018 · e39a889 · e39a889
1 parent 5133b68
commit e39a889
Show file tree

Hide file tree

Showing 12 changed files with 149 additions and 31 deletions.
diff --git a/src/services/codefixes/fixStrictClassInitialization.ts b/src/services/codefixes/fixStrictClassInitialization.ts
@@ -80,9 +80,8 @@ namespace ts.codefix {
     }
 
     function addUndefinedType(changeTracker: textChanges.ChangeTracker, propertyDeclarationSourceFile: SourceFile, propertyDeclaration: PropertyDeclaration): void {
-        const undefinedTypeNode = createKeywordTypeNode(SyntaxKind.UndefinedKeyword);
-        const types = isUnionTypeNode(propertyDeclaration.type) ? propertyDeclaration.type.types.concat(undefinedTypeNode) : [propertyDeclaration.type, undefinedTypeNode];
-        changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration.type, createUnionTypeNode(types));
+        const type = mergeTypeNodeToUnion(propertyDeclaration.type, createKeywordTypeNode(SyntaxKind.UndefinedKeyword));
+        changeTracker.replaceNode(propertyDeclarationSourceFile, propertyDeclaration.type, type);
     }
 
     function getActionForAddMissingInitializer (context: CodeFixContext, propertyDeclaration: PropertyDeclaration, newLineCharacter: string): CodeFixAction | undefined {

diff --git a/...ices/refactors/GenerateGetterAndSetter.ts → ...tors/generateGetAccessorAndSetAccessor.ts b/...ices/refactors/GenerateGetterAndSetter.ts → ...tors/generateGetAccessorAndSetAccessor.ts
@@ -1,10 +1,20 @@
 /* @internal */
-namespace ts.refactor.GenerateGetterAndSetter {
+namespace ts.refactor.generateGetAccessorAndSetAccessor {
     const actionName = "Generate 'get' and 'set' accessors";
     const actionDescription = Diagnostics.Generate_get_and_set_accessors.message;
-
     registerRefactor(actionName, { getEditsForAction, getAvailableActions });
 
+    interface Info {
+        originalName: string;
+        fieldName: string;
+        accessorName: string;
+        accessorType: TypeNode;
+        propertyDeclaration: PropertyDeclaration;
+        needUpdateName: boolean;
+        hasModifiers: boolean;
+        needUpdateModifiers: boolean;
+    }
+
     function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined {
         const { file, startPosition } = context;
 
@@ -34,15 +44,17 @@ namespace ts.refactor.GenerateGetterAndSetter {
         const changeTracker = textChanges.ChangeTracker.fromContext(context);
         const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options);
 
-        const { fieldName, accessorName, propertyDeclaration, needUpdateName, hasModifiers, needUpdateModifiers } = fieldInfo;
-        const accessorModifiers = hasModifiers ? createNodeArray([createToken(SyntaxKind.PublicKeyword)]) : undefined;
+        const { fieldName, accessorName, accessorType, propertyDeclaration, needUpdateName, hasModifiers, needUpdateModifiers } = fieldInfo;
+        const accessorModifiers = hasModifiers ? (
+            (getModifierFlags(propertyDeclaration) & ModifierFlags.Private || !propertyDeclaration.modifiers) ? createNodeArray([createToken(SyntaxKind.PublicKeyword)]) : propertyDeclaration.modifiers
+        ) : undefined;
 
-        const getAccessor = generateGetAccessor(propertyDeclaration, fieldName, accessorName, accessorModifiers);
-        const setAccessor = generateSetAccessor(propertyDeclaration, fieldName, accessorName, accessorModifiers);
+        const getAccessor = generateGetAccessor(fieldName, accessorName, accessorType, accessorModifiers);
+        const setAccessor = generateSetAccessor(fieldName, accessorName, accessorType, accessorModifiers);
 
-        const modifiers = needUpdateModifiers ? createNodeArray([createToken(SyntaxKind.PrivateKeyword)]) : propertyDeclaration.modifiers;
+        const modifiers = hasModifiers ? createNodeArray([createToken(SyntaxKind.PrivateKeyword)]) : undefined;
         if (needUpdateName || needUpdateModifiers) {
-            changeTracker.replaceNode(file, propertyDeclaration, updateOriginPropertyDeclaration(propertyDeclaration, fieldName, modifiers), {
+            changeTracker.replaceNode(file, propertyDeclaration, updateoriginalPropertyDeclaration(propertyDeclaration, fieldName, modifiers), {
                 suffix: newLineCharacter
             });
         }
@@ -57,13 +69,13 @@ namespace ts.refactor.GenerateGetterAndSetter {
         };
     }
 
-    interface Info { originName: string; fieldName: string; accessorName: string; propertyDeclaration: PropertyDeclaration; needUpdateName: boolean; hasModifiers: boolean; needUpdateModifiers: boolean; }
     function getConvertibleFieldAtPosition(file: SourceFile, startPosition: number): Info | undefined {
         const node = getTokenAtPosition(file, startPosition, /*includeJsDocComment*/ false);
         const propertyDeclaration = findAncestor(node.parent, isPropertyDeclaration);
 
-        if (!(propertyDeclaration && propertyDeclaration.name.kind === SyntaxKind.Identifier &&
-            (getModifierFlags(propertyDeclaration) | ModifierFlags.AccessibilityModifier) === ModifierFlags.AccessibilityModifier)) return undefined;
+        if (!propertyDeclaration || propertyDeclaration.name.kind !== SyntaxKind.Identifier) return undefined;
+        // make sure propertyDeclaration have only AccessibilityModifier
+        if ((getModifierFlags(propertyDeclaration) | ModifierFlags.AccessibilityModifier) !== ModifierFlags.AccessibilityModifier) return undefined;
 
         const containerClass = getContainingClass(propertyDeclaration);
         if (!containerClass) return undefined;
@@ -79,26 +91,28 @@ namespace ts.refactor.GenerateGetterAndSetter {
         if (find(members, member => needUpdateName ? member.name.getText() === fieldName : member.name.getText() === accessorName)) return undefined;
 
         const hasModifiers = !!find(members, member => !!member.modifiers);
-        const needUpdateModifiers = hasModifiers && (!propertyDeclaration.modifiers || hasModifier(propertyDeclaration, ModifierFlags.Public));
+        const needUpdateModifiers = hasModifiers && (!propertyDeclaration.modifiers || !hasModifier(propertyDeclaration, ModifierFlags.Private));
+        const accessorType = propertyDeclaration.questionToken ? mergeTypeNodeToUnion(propertyDeclaration.type, createKeywordTypeNode(SyntaxKind.UndefinedKeyword)) : propertyDeclaration.type;
 
         return {
-            originName: propertyDeclaration.name.text,
+            originalName: propertyDeclaration.name.text,
             fieldName,
             accessorName,
+            accessorType,
             propertyDeclaration,
             needUpdateName,
             hasModifiers,
             needUpdateModifiers
         };
     }
 
-    function generateGetAccessor (propertyDeclaration: PropertyDeclaration, fieldName: string, name: string, modifiers: ModifiersArray) {
+    function generateGetAccessor (fieldName: string, name: string, type: TypeNode, modifiers: ModifiersArray) {
         return createGetAccessor(
             /*decorators*/ undefined,
             modifiers,
             name,
             /*parameters*/ undefined,
-            propertyDeclaration.type,
+            type,
             createBlock([
                 createReturn(
                     createPropertyAccess(
@@ -110,7 +124,7 @@ namespace ts.refactor.GenerateGetterAndSetter {
         );
     }
 
-    function generateSetAccessor (propertyDeclaration: PropertyDeclaration, fieldName: string, name: string, modifiers: ModifiersArray) {
+    function generateSetAccessor (fieldName: string, name: string, type: TypeNode, modifiers: ModifiersArray) {
         return createSetAccessor(
             /*decorators*/ undefined,
             modifiers,
@@ -121,7 +135,7 @@ namespace ts.refactor.GenerateGetterAndSetter {
                 /*dotDotDotToken*/ undefined,
                 createIdentifier("value"),
                 /*questionToken*/ undefined,
-                propertyDeclaration.type
+                type
             )],
             createBlock([
                 createStatement(
@@ -137,15 +151,15 @@ namespace ts.refactor.GenerateGetterAndSetter {
         );
     }
 
-    function updateOriginPropertyDeclaration (propertyDeclaration: PropertyDeclaration, fieldName: string, modifiers: ModifiersArray) {
+    function updateoriginalPropertyDeclaration (propertyDeclaration: PropertyDeclaration, fieldName: string, modifiers: ModifiersArray) {
         return updateProperty(
             propertyDeclaration,
-            /*decorators*/ undefined,
+            propertyDeclaration.decorators,
             modifiers,
             fieldName,
-            /*questionOrExclamationToken*/ undefined,
+            propertyDeclaration.questionToken || propertyDeclaration.exclamationToken,
             propertyDeclaration.type,
-            propertyDeclaration.initializer,
+            propertyDeclaration.initializer
         );
     }
 }
diff --git a/src/services/refactors/refactors.ts b/src/services/refactors/refactors.ts
@@ -4,4 +4,4 @@
 /// <reference path="extractSymbol.ts" />
 /// <reference path="installTypesForPackage.ts" />
 /// <reference path="useDefaultImport.ts" />
-/// <reference path="GenerateGetterAndSetter.ts" />
+/// <reference path="generateGetAccessorAndSetAccessor.ts" />
diff --git a/src/services/utilities.ts b/src/services/utilities.ts
@@ -269,6 +269,10 @@ namespace ts {
             getExternalModuleImportEqualsDeclarationExpression(node.parent.parent) === node;
     }
 
+    export function mergeTypeNodeToUnion(target: TypeNode, ...types: TypeNode[]) {
+        return createUnionTypeNode(isUnionTypeNode(target) ? target.types.concat(types) : [target].concat(types));
+    }
+
     export function getContainerNode(node: Node): Declaration {
         if (node.kind === SyntaxKind.JSDocTypedefTag) {
             // This doesn't just apply to the node immediately under the comment, but to everything in its parent's scope.

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess12.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess12.ts
@@ -0,0 +1,23 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     @foo
+////     /*a*/public a: string = "foo";/*b*/
+//// }
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Generate 'get' and 'set' accessors",
+    actionName: "Generate 'get' and 'set' accessors",
+    actionDescription: "Generate 'get' and 'set' accessors",
+    newContent: `class A {
+    @foo
+    private _a: string = "foo";
+    public get a(): string {
+        return this._a;
+    }
+    public set a(value: string) {
+        this._a = value;
+    }
+}`,
+});
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess13.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess13.ts
@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     /*a*/public a?: string = "foo";/*b*/
+//// }
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Generate 'get' and 'set' accessors",
+    actionName: "Generate 'get' and 'set' accessors",
+    actionDescription: "Generate 'get' and 'set' accessors",
+    newContent: `class A {
+    private _a?: string = "foo";
+    public get a(): string | undefined {
+        return this._a;
+    }
+    public set a(value: string | undefined) {
+        this._a = value;
+    }
+}`,
+});
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess14.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess14.ts
@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     /*a*/public a!: string = "foo";/*b*/
+//// }
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Generate 'get' and 'set' accessors",
+    actionName: "Generate 'get' and 'set' accessors",
+    actionDescription: "Generate 'get' and 'set' accessors",
+    newContent: `class A {
+    private _a!: string = "foo";
+    public get a(): string {
+        return this._a;
+    }
+    public set a(value: string) {
+        this._a = value;
+    }
+}`,
+});
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess15.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess15.ts
@@ -0,0 +1,16 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     /*a*/public "a": string = "foo";/*b*/
+////     /*c*/public get b/*d*/ () { return 1; }
+////     /*e*/public set b/*f*/ (v) { }
+//// }
+
+goTo.select("a", "b");
+verify.not.refactorAvailable("Generate 'get' and 'set' accessors");
+
+goTo.select("c", "d");
+verify.not.refactorAvailable("Generate 'get' and 'set' accessors");
+
+goTo.select("e", "f");
+verify.not.refactorAvailable("Generate 'get' and 'set' accessors");
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess16.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess16.ts
@@ -0,0 +1,9 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     /*a*/public readonly a: string = "foo";/*b*/
+////     /*c*/public static a: string = "foo";/*d*/
+//// }
+
+goTo.select("a", "b");
+verify.not.refactorAvailable("Generate 'get' and 'set' accessors");
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess17.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess17.ts
@@ -0,0 +1,11 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     public _a: number = 1;
+////     /*a*/public a: string = "foo";/*b*/
+////     public b: number = 2;
+////     /*c*/public _b: string = "foo";/*d*/
+//// }
+
+goTo.select("a", "b");
+verify.not.refactorAvailable("Generate 'get' and 'set' accessors");
diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess2.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess2.ts
@@ -10,11 +10,11 @@ edit.applyRefactor({
     actionName: "Generate 'get' and 'set' accessors",
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
-    protected _a: string;
-    public get a(): string {
+    private _a: string;
+    protected get a(): string {
         return this._a;
     }
-    public set a(value: string) {
+    protected set a(value: string) {
         this._a = value;
     }
 }`,

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess5.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess5.ts
@@ -10,11 +10,11 @@ edit.applyRefactor({
     actionName: "Generate 'get' and 'set' accessors",
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
-    protected _a: string;
-    public get a(): string {
+    private _a: string;
+    protected get a(): string {
         return this._a;
     }
-    public set a(value: string) {
+    protected set a(value: string) {
         this._a = value;
     }
 }`,