Merge pull request microsoft#23954 from Kingwl/readonly-getter-support

add support for readonly modifier

Author: mhegazy

src/services/refactors/generateGetAccessorAndSetAccessor.ts

@@ -11,10 +11,12 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
     interface Info {
         container: ContainerDeclaration;
         isStatic: boolean;
+        isReadonly: boolean;
         type: TypeNode | undefined;
         declaration: AcceptedDeclaration;
         fieldName: AcceptedNameType;
         accessorName: AcceptedNameType;
+        originalName: AcceptedNameType;
     }
 
     function getAvailableActions(context: RefactorContext): ApplicableRefactorInfo[] | undefined {
@@ -41,21 +43,40 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
 
         const isJS = isSourceFileJavaScript(file);
         const changeTracker = textChanges.ChangeTracker.fromContext(context);
-        const { isStatic, fieldName, accessorName, type, container, declaration } = fieldInfo;
+        const { isStatic, isReadonly, fieldName, accessorName, originalName, type, container, declaration } = fieldInfo;
+
+        suppressLeadingAndTrailingTrivia(fieldName);
+        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
-            ? !declaration.modifiers || getModifierFlags(declaration) & ModifierFlags.Private ? getModifiers(isJS, isStatic, SyntaxKind.PublicKeyword) : declaration.modifiers
+            ? !modifierFlags || modifierFlags & ModifierFlags.Private
+                ? getModifiers(isJS, isStatic, SyntaxKind.PublicKeyword)
+                : createNodeArray(createModifiersFromModifierFlags(modifierFlags))
             : undefined;
         const fieldModifiers = isInClassLike ? getModifiers(isJS, isStatic, SyntaxKind.PrivateKeyword) : undefined;
 
         updateFieldDeclaration(changeTracker, file, declaration, fieldName, fieldModifiers);
 
         const getAccessor = generateGetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
-        const setAccessor = generateSetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
-
+        suppressLeadingAndTrailingTrivia(getAccessor);
         insertAccessor(changeTracker, file, getAccessor, declaration, container);
-        insertAccessor(changeTracker, file, setAccessor, declaration, container);
+
+        if (isReadonly) {
+            // readonly modifier only existed in classLikeDeclaration
+            const constructor = getFirstConstructorWithBody(<ClassLikeDeclaration>container);
+            if (constructor) {
+                updateReadonlyPropertyInitializerStatementConstructor(changeTracker, context, constructor, fieldName, originalName);
+            }
+        }
+        else {
+            const setAccessor = generateSetAccessor(fieldName, accessorName, type, accessorModifiers, isStatic, container);
+            suppressLeadingAndTrailingTrivia(setAccessor);
+            insertAccessor(changeTracker, file, setAccessor, declaration, container);
+        }
 
         const edits = changeTracker.getChanges();
         const renameFilename = file.fileName;
@@ -92,18 +113,18 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
     function getConvertibleFieldAtPosition(file: SourceFile, startPosition: number): Info | undefined {
         const node = getTokenAtPosition(file, startPosition, /*includeJsDocComment*/ false);
         const declaration = findAncestor(node.parent, isAcceptedDeclaration);
-        // make sure propertyDeclaration have AccessibilityModifier or Static Modifier
-        const meaning = ModifierFlags.AccessibilityModifier | ModifierFlags.Static;
+        // make sure declaration have AccessibilityModifier or Static Modifier or Readonly Modifier
+        const meaning = ModifierFlags.AccessibilityModifier | ModifierFlags.Static | ModifierFlags.Readonly;
         if (!declaration || !isConvertableName(declaration.name) || (getModifierFlags(declaration) | meaning) !== meaning) return undefined;
 
         const fieldName = createPropertyName(getUniqueName(`_${declaration.name.text}`, file.text), declaration.name);
         const accessorName = createPropertyName(declaration.name.text, declaration.name);
-        suppressLeadingAndTrailingTrivia(fieldName);
-        suppressLeadingAndTrailingTrivia(declaration);
         return {
             isStatic: hasStaticModifier(declaration),
+            isReadonly: hasReadonlyModifier(declaration),
             type: getTypeAnnotationNode(declaration),
             container: declaration.kind === SyntaxKind.Parameter ? declaration.parent.parent : declaration.parent,
+            originalName: <AcceptedNameType>declaration.name,
             declaration,
             fieldName,
             accessorName,
@@ -159,7 +180,6 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
             declaration.type,
             declaration.initializer
         );
-
         changeTracker.replaceNode(file, declaration, property);
     }
 
@@ -186,4 +206,24 @@ namespace ts.refactor.generateGetAccessorAndSetAccessor {
             ? changeTracker.insertNodeAtClassStart(file, <ClassLikeDeclaration>container, accessor)
             : changeTracker.insertNodeAfter(file, declaration, accessor);
     }
+
+    function updateReadonlyPropertyInitializerStatementConstructor(changeTracker: textChanges.ChangeTracker, context: RefactorContext, constructor: ConstructorDeclaration, fieldName: AcceptedNameType, originalName: AcceptedNameType) {
+        if (!constructor.body) return;
+        const { file, program, cancellationToken } = context;
+
+        const referenceEntries = mapDefined(FindAllReferences.getReferenceEntriesForNode(originalName.parent.pos, originalName, program, [file], cancellationToken), entry => (
+            (entry.type === "node" && rangeContainsRange(constructor, entry.node) && isIdentifier(entry.node) && isWriteAccess(entry.node)) ? entry.node : undefined
+        ));
+
+        forEach(referenceEntries, entry => {
+            const parent = entry.parent;
+            const accessorName = createIdentifier(fieldName.text);
+            const node = isBinaryExpression(parent)
+                ? updateBinary(parent, accessorName, parent.right, parent.operatorToken)
+                : isPropertyAccessExpression(parent)
+                    ? updatePropertyAccess(parent, parent.expression, accessorName)
+                    : Debug.fail("Unexpected write access token");
+            changeTracker.replaceNode(file, parent, node);
+        });
+    }
 }

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess14.ts

@@ -5,4 +5,15 @@
 //// }
 
 goTo.select("a", "b");
-verify.not.refactorAvailable("Generate 'get' and 'set' accessors");
+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";
+    public get a(): string {
+        return this._a;
+    }
+}`,
+});

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccess33.ts

@@ -0,0 +1,42 @@
+/// <reference path='fourslash.ts' />
+
+//// class A {
+////     public readonly /*a*/a/*b*/: number;
+////     public b: number;
+////     constructor () {
+////         this.a = 1; // convert
+////         this.a++; // convert
+////         ++this.a; // convert
+////         if (Math.random()) {
+////             this.a = 2; // convert
+////         }
+////         console.log(this.a); // preserve
+////         this.b = this.a; // preserve
+////     }
+////     foo () { this.a = 2; }
+//// }
+
+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: number;
+    public get a(): number {
+        return this._a;
+    }
+    public b: number;
+    constructor () {
+        this._a = 1; // convert
+        this._a++; // convert
+        ++this._a; // convert
+        if (Math.random()) {
+            this._a = 2; // convert
+        }
+        console.log(this.a); // preserve
+        this.b = this.a; // preserve
+    }
+    foo () { this.a = 2; }
+}`,
+});