fix(33836): allow readonly modifier for a field with only get accessor

microsoft · Apr 10, 2020 · b88a0b1 · b88a0b1
1 parent 52dc9f2
commit b88a0b1
Show file tree

Hide file tree

Showing 3 changed files with 36 additions and 19 deletions.
diff --git a/src/services/refactors/generateGetAccessorAndSetAccessor.ts b/src/services/refactors/generateGetAccessorAndSetAccessor.ts
@@ -41,7 +41,6 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
         const fieldInfo = getConvertibleFieldAtPosition(context);
         if (!fieldInfo) return undefined;
 
-        const isJS = isSourceFileJS(file);
         const changeTracker = textChanges.ChangeTracker.fromContext(context);
         const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration, renameAccessor } = fieldInfo;
 
@@ -50,15 +49,20 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
         suppressLeadingAndTrailingTrivia(declaration);
         suppressLeadingAndTrailingTrivia(container);
 
-        const isInClassLike = isClassLike(container);
-        // avoid Readonly modifier because it will convert to get accessor
-        const modifierFlags = getModifierFlags(declaration) & ~ModifierFlags.Readonly;
-        const accessorModifiers = isInClassLike
-            ? !modifierFlags || modifierFlags & ModifierFlags.Private
-                ? getModifiers(isJS, isStatic, SyntaxKind.PublicKeyword)
-                : createNodeArray(createModifiersFromModifierFlags(modifierFlags))
-            : undefined;
-        const fieldModifiers = isInClassLike ? getModifiers(isJS, isStatic, SyntaxKind.PrivateKeyword) : undefined;
+        let accessorModifiers: ModifiersArray | undefined;
+        let fieldModifiers: ModifiersArray | undefined;
+        if (isClassLike(container)) {
+            const modifierFlags = getModifierFlags(declaration);
+            if (isSourceFileJS(file)) {
+                const modifiers = createModifiers(modifierFlags);
+                accessorModifiers = modifiers;
+                fieldModifiers = modifiers;
+            }
+            else {
+                accessorModifiers = createModifiers(prepareModifierFlagsForAccessor(modifierFlags));
+                fieldModifiers = createModifiers(prepareModifierFlagsForField(modifierFlags));
+            }
+        }
 
         updateFieldDeclaration(changeTracker, file, declaration, fieldName, fieldModifiers);
 
@@ -105,12 +109,26 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
         return isIdentifier(fieldName) ? createPropertyAccess(leftHead, fieldName) : createElementAccess(leftHead, createLiteral(fieldName));
     }
 
-    function getModifiers(isJS: boolean, isStatic: boolean, accessModifier: SyntaxKind.PublicKeyword | SyntaxKind.PrivateKeyword): NodeArray<Modifier> | undefined {
-        const modifiers = append<Modifier>(
-            !isJS ? [createToken(accessModifier) as Token<SyntaxKind.PublicKeyword> | Token<SyntaxKind.PrivateKeyword>] : undefined,
-            isStatic ? createToken(SyntaxKind.StaticKeyword) : undefined
-        );
-        return modifiers && createNodeArray(modifiers);
+    function createModifiers(modifierFlags: ModifierFlags): ModifiersArray | undefined {
+        return modifierFlags ? createNodeArray(createModifiersFromModifierFlags(modifierFlags)) : undefined;
+    }
+
+    function prepareModifierFlagsForAccessor(modifierFlags: ModifierFlags): ModifierFlags {
+        modifierFlags &= ~ModifierFlags.Readonly; // avoid Readonly modifier because it will convert to get accessor
+        modifierFlags &= ~ModifierFlags.Private;
+
+        if (!(modifierFlags & ModifierFlags.Protected)) {
+            modifierFlags |= ModifierFlags.Public;
+        }
+
+        return modifierFlags;
+    }
+
+    function prepareModifierFlagsForField(modifierFlags: ModifierFlags): ModifierFlags {
+        modifierFlags &= ~ModifierFlags.Public;
+        modifierFlags &= ~ModifierFlags.Protected;
+        modifierFlags |= ModifierFlags.Private;
+        return modifierFlags;
     }
 
     function getConvertibleFieldAtPosition(context: RefactorContext): Info | undefined {

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess14.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess14.ts
@@ -4,14 +4,13 @@
 ////     /*a*/public readonly a: string = "foo";/*b*/
 //// }
 
-goTo.select("a", "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 /*RENAME*/_a: string = "foo";
+    private readonly /*RENAME*/_a: string = "foo";
     public get a(): string {
         return this._a;
     }

diff --git a/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess33.ts b/tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess33.ts
@@ -22,7 +22,7 @@ edit.applyRefactor({
     actionName: "Generate 'get' and 'set' accessors",
     actionDescription: "Generate 'get' and 'set' accessors",
     newContent: `class A {
-    private /*RENAME*/_a: number;
+    private readonly /*RENAME*/_a: number;
     public get a(): number {
         return this._a;
     }