Fix crash in extract type and generate get/set refactors

Author: rbuckton

src/compiler/checker.ts

@@ -46927,7 +46927,6 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
             case SyntaxKind.PropertyAssignment:
             case SyntaxKind.ShorthandPropertyAssignment:
             case SyntaxKind.NamespaceExportDeclaration:
-            case SyntaxKind.FunctionType:
             case SyntaxKind.MissingDeclaration:
                 return find(node.modifiers, isModifier);
             default:

src/compiler/factory/utilities.ts

@@ -1151,7 +1151,6 @@ export function canHaveIllegalModifiers(node: Node): node is HasIllegalModifiers
     return kind === SyntaxKind.ClassStaticBlockDeclaration
         || kind === SyntaxKind.PropertyAssignment
         || kind === SyntaxKind.ShorthandPropertyAssignment
-        || kind === SyntaxKind.FunctionType
         || kind === SyntaxKind.MissingDeclaration
         || kind === SyntaxKind.NamespaceExportDeclaration;
 }

src/compiler/parser.ts

@@ -4359,13 +4359,13 @@ namespace Parser {
         const hasJSDoc = hasPrecedingJSDocComment();
         const modifiers = parseModifiersForConstructorType();
         const isConstructorType = parseOptional(SyntaxKind.NewKeyword);
+        Debug.assert(!modifiers || isConstructorType, "Per isStartOfFunctionOrConstructorType, a function type cannot have modifiers.");
         const typeParameters = parseTypeParameters();
         const parameters = parseParameters(SignatureFlags.Type);
         const type = parseReturnType(SyntaxKind.EqualsGreaterThanToken, /*isType*/ false);
         const node = isConstructorType
             ? factory.createConstructorTypeNode(modifiers, typeParameters, parameters, type)
             : factory.createFunctionTypeNode(typeParameters, parameters, type);
-        if (!isConstructorType) (node as Mutable<FunctionTypeNode>).modifiers = modifiers;
         return withJSDoc(finishNode(node, pos), hasJSDoc);
     }
 

src/compiler/types.ts

@@ -1362,7 +1362,6 @@ export type HasIllegalModifiers =
     | PropertyAssignment
     | ShorthandPropertyAssignment
     | MissingDeclaration
-    | FunctionTypeNode
     | NamespaceExportDeclaration
     ;
 
@@ -1801,7 +1800,7 @@ export interface TypeParameterDeclaration extends NamedDeclaration, JSDocContain
     readonly constraint?: TypeNode;
     readonly default?: TypeNode;
 
-    // For error recovery purposes.
+    // For error recovery purposes (see `isGrammarError` in utilities.ts).
     expression?: Expression;
 }
 
@@ -1888,7 +1887,7 @@ export interface PropertySignature extends TypeElement, JSDocContainer {
     readonly questionToken?: QuestionToken;      // Present on optional property
     readonly type?: TypeNode;                    // Optional type annotation
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly initializer?: Expression | undefined; // A property signature cannot have an initializer
 }
 
@@ -1897,7 +1896,7 @@ export interface PropertyDeclaration extends ClassElement, JSDocContainer {
     readonly parent: ClassLikeDeclaration;
     readonly modifiers?: NodeArray<ModifierLike>;
     readonly name: PropertyName;
-    readonly questionToken?: QuestionToken;      // Present for use with reporting a grammar error
+    readonly questionToken?: QuestionToken;      // Present for use with reporting a grammar error for auto-accessors (see `isGrammarError` in utilities.ts)
     readonly exclamationToken?: ExclamationToken;
     readonly type?: TypeNode;
     readonly initializer?: Expression;           // Optional initializer
@@ -1960,7 +1959,7 @@ export interface PropertyAssignment extends ObjectLiteralElement, JSDocContainer
     readonly name: PropertyName;
     readonly initializer: Expression;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly modifiers?: NodeArray<ModifierLike> | undefined; // property assignment cannot have decorators or modifiers
     /** @internal */ readonly questionToken?: QuestionToken | undefined; // property assignment cannot have a question token
     /** @internal */ readonly exclamationToken?: ExclamationToken | undefined; // property assignment cannot have an exclamation token
@@ -1971,11 +1970,11 @@ export interface ShorthandPropertyAssignment extends ObjectLiteralElement, JSDoc
     readonly parent: ObjectLiteralExpression;
     readonly name: Identifier;
     // used when ObjectLiteralExpression is used in ObjectAssignmentPattern
-    // it is a grammar error to appear in actual object initializer:
+    // it is a grammar error to appear in actual object initializer (see `isGrammarError` in utilities.ts):
     readonly equalsToken?: EqualsToken;
     readonly objectAssignmentInitializer?: Expression;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly modifiers?: NodeArray<ModifierLike> | undefined; // shorthand property assignment cannot have decorators or modifiers
     /** @internal */ readonly questionToken?: QuestionToken | undefined; // shorthand property assignment cannot have a question token
     /** @internal */ readonly exclamationToken?: ExclamationToken | undefined; // shorthand property assignment cannot have an exclamation token
@@ -2076,7 +2075,7 @@ export interface MethodDeclaration extends FunctionLikeDeclarationBase, ClassEle
     readonly name: PropertyName;
     readonly body?: FunctionBody | undefined;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly exclamationToken?: ExclamationToken | undefined; // A method cannot have an exclamation token
 }
 
@@ -2086,7 +2085,7 @@ export interface ConstructorDeclaration extends FunctionLikeDeclarationBase, Cla
     readonly modifiers?: NodeArray<ModifierLike> | undefined;
     readonly body?: FunctionBody | undefined;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly typeParameters?: NodeArray<TypeParameterDeclaration>; // A constructor cannot have type parameters
     /** @internal */ readonly type?: TypeNode; // A constructor cannot have a return type annotation
 }
@@ -2106,7 +2105,7 @@ export interface GetAccessorDeclaration extends FunctionLikeDeclarationBase, Cla
     readonly name: PropertyName;
     readonly body?: FunctionBody;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly typeParameters?: NodeArray<TypeParameterDeclaration> | undefined; // A get accessor cannot have type parameters
 }
 
@@ -2119,7 +2118,7 @@ export interface SetAccessorDeclaration extends FunctionLikeDeclarationBase, Cla
     readonly name: PropertyName;
     readonly body?: FunctionBody;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly typeParameters?: NodeArray<TypeParameterDeclaration> | undefined; // A set accessor cannot have type parameters
     /** @internal */ readonly type?: TypeNode | undefined; // A set accessor cannot have a return type
 }
@@ -2141,7 +2140,7 @@ export interface ClassStaticBlockDeclaration extends ClassElement, JSDocContaine
     /** @internal */ endFlowNode?: FlowNode;
     /** @internal */ returnFlowNode?: FlowNode;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly modifiers?: NodeArray<ModifierLike> | undefined;
 }
 
@@ -2190,8 +2189,8 @@ export interface FunctionOrConstructorTypeNodeBase extends TypeNode, SignatureDe
 export interface FunctionTypeNode extends FunctionOrConstructorTypeNodeBase, LocalsContainer {
     readonly kind: SyntaxKind.FunctionType;
 
-    // The following properties are used only to report grammar errors
-    /** @internal */ readonly modifiers?: NodeArray<Modifier> | undefined;
+    // A function type cannot have modifiers
+    /** @internal */ readonly modifiers?: undefined;
 }
 
 export interface ConstructorTypeNode extends FunctionOrConstructorTypeNodeBase, LocalsContainer {
@@ -3723,7 +3722,7 @@ export interface NamespaceExportDeclaration extends DeclarationStatement, JSDocC
     readonly kind: SyntaxKind.NamespaceExportDeclaration;
     readonly name: Identifier;
 
-    // The following properties are used only to report grammar errors
+    // The following properties are used only to report grammar errors (see `isGrammarError` in utilities.ts)
     /** @internal */ readonly modifiers?: NodeArray<ModifierLike> | undefined;
 }
 

src/compiler/utilities.ts

@@ -234,6 +234,7 @@ import {
     isArray,
     isArrayLiteralExpression,
     isArrowFunction,
+    isAutoAccessorPropertyDeclaration,
     isBigIntLiteral,
     isBinaryExpression,
     isBindingElement,
@@ -265,6 +266,7 @@ import {
     isFunctionLike,
     isFunctionLikeDeclaration,
     isFunctionLikeOrClassStaticBlockDeclaration,
+    isFunctionTypeNode,
     isGetAccessorDeclaration,
     isHeritageClause,
     isIdentifier,
@@ -300,6 +302,7 @@ import {
     isMetaProperty,
     isMethodDeclaration,
     isMethodOrAccessor,
+    isModifierLike,
     isModuleDeclaration,
     isNamedDeclaration,
     isNamespaceExport,
@@ -333,6 +336,7 @@ import {
     isTypeElement,
     isTypeLiteralNode,
     isTypeNode,
+    isTypeParameterDeclaration,
     isTypeReferenceNode,
     isVariableDeclaration,
     isVariableStatement,
@@ -964,6 +968,31 @@ export function nodeIsPresent(node: Node | undefined): boolean {
     return !nodeIsMissing(node);
 }
 
+/**
+ * Tests whether `child` is a grammar error on `parent`.
+ * @internal
+ */
+export function isGrammarError(parent: Node, child: Node | NodeArray<Node>) {
+    if (isTypeParameterDeclaration(parent)) return child === parent.expression;
+    if (isClassStaticBlockDeclaration(parent)) return child === parent.modifiers;
+    if (isPropertySignature(parent)) return child === parent.initializer;
+    if (isPropertyDeclaration(parent)) return child === parent.questionToken && isAutoAccessorPropertyDeclaration(parent);
+    if (isPropertyAssignment(parent)) return child === parent.modifiers || child === parent.questionToken || child === parent.exclamationToken || isGrammarErrorElement(parent.modifiers, child, isModifierLike);
+    if (isShorthandPropertyAssignment(parent)) return child === parent.equalsToken || child === parent.modifiers || child === parent.questionToken || child === parent.exclamationToken || isGrammarErrorElement(parent.modifiers, child, isModifierLike);
+    if (isMethodDeclaration(parent)) return child === parent.exclamationToken;
+    if (isConstructorDeclaration(parent)) return child === parent.typeParameters || child === parent.type || isGrammarErrorElement(parent.typeParameters, child, isTypeParameterDeclaration);
+    if (isGetAccessorDeclaration(parent)) return child === parent.typeParameters || isGrammarErrorElement(parent.typeParameters, child, isTypeParameterDeclaration);
+    if (isSetAccessorDeclaration(parent)) return child === parent.typeParameters || child === parent.type || isGrammarErrorElement(parent.typeParameters, child, isTypeParameterDeclaration);
+    if (isFunctionTypeNode(parent)) return child === parent.modifiers || isGrammarErrorElement(parent.modifiers, child, isModifierLike);
+    if (isNamespaceExportDeclaration(parent)) return child === parent.modifiers || isGrammarErrorElement(parent.modifiers, child, isModifierLike);
+    return false;
+}
+
+function isGrammarErrorElement<T extends Node>(nodeArray: NodeArray<T> | undefined, child: Node | NodeArray<Node>, isElement: (node: Node) => node is T) {
+    if (!nodeArray || isArray(child) || !isElement(child)) return false;
+    return contains(nodeArray, child);
+}
+
 function insertStatementsAfterPrologue<T extends Statement>(to: T[], from: readonly T[] | undefined, isPrologueDirective: (node: Node) => boolean): T[] {
     if (from === undefined || from.length === 0) return to;
     let statementIndex = 0;

src/services/codefixes/generateAccessors.ts

@@ -37,6 +37,7 @@ import {
     isWriteAccess,
     ModifierFlags,
     ModifierLike,
+    Mutable,
     Node,
     nodeOverlapsWithStartEnd,
     ObjectLiteralExpression,
@@ -258,7 +259,14 @@ function updatePropertyDeclaration(changeTracker: textChanges.ChangeTracker, fil
 }
 
 function updatePropertyAssignmentDeclaration(changeTracker: textChanges.ChangeTracker, file: SourceFile, declaration: PropertyAssignment, fieldName: AcceptedNameType) {
-    const assignment = factory.updatePropertyAssignment(declaration, fieldName, declaration.initializer);
+    let assignment = factory.updatePropertyAssignment(declaration, fieldName, declaration.initializer);
+    // Remove grammar errors from assignment
+    if (assignment.modifiers || assignment.questionToken || assignment.exclamationToken) {
+        if (assignment === declaration) assignment = factory.cloneNode(assignment);
+        (assignment as Mutable<PropertyAssignment>).modifiers = undefined;
+        (assignment as Mutable<PropertyAssignment>).questionToken = undefined;
+        (assignment as Mutable<PropertyAssignment>).exclamationToken = undefined;
+    }
     changeTracker.replacePropertyAssignment(file, declaration, assignment);
 }
 

src/services/formatting/formatting.ts

@@ -34,6 +34,7 @@ import {
     InterfaceDeclaration,
     isComment,
     isDecorator,
+    isGrammarError,
     isJSDoc,
     isLineBreak,
     isModifier,
@@ -863,7 +864,7 @@ function formatSpanWorker(
                 // if child node is a token, it does not impact indentation, proceed it using parent indentation scope rules
                 const tokenInfo = formattingScanner.readTokenInfo(child);
                 // JSX text shouldn't affect indenting
-                if (child.kind !== SyntaxKind.JsxText) {
+                if (child.kind !== SyntaxKind.JsxText && !isGrammarError(parent, child)) {
                     Debug.assert(tokenInfo.token.end === child.end, "Token end is child end");
                     consumeTokenAndAdvanceScanner(tokenInfo, node, parentDynamicIndentation, child);
                     return inheritedIndentation;

tests/cases/fourslash/refactorConvertToGetAccessAndSetAccessRemoveGrammarErrors1.ts

@@ -0,0 +1,21 @@
+/// <reference path='fourslash.ts' />
+
+//// const foo = {
+////     /*a*/async a: 1/*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: `const foo = {
+    /*RENAME*/_a: 1,
+    get a() {
+        return this._a;
+    },
+    set a(value) {
+        this._a = value;
+    },
+}`,
+});

tests/cases/fourslash/refactorExtractTypeRemoveGrammarError1.ts

@@ -0,0 +1,17 @@
+/// <reference path='fourslash.ts' />
+
+// @Filename: a.ts
+////type Foo = /*a*/{ x: string = a }/*b*/
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract type",
+    actionName: "Extract to type alias",
+    actionDescription: "Extract to type alias",
+    newContent:
+`type /*RENAME*/NewType = {
+    x: string;
+};
+
+type Foo = NewType`,
+});

tests/cases/fourslash/refactorExtractTypeRemoveGrammarError2.ts

@@ -0,0 +1,17 @@
+/// <reference path='fourslash.ts' />
+
+// @Filename: a.ts
+////type Foo<T extends /*a*/{ x: string = a }/*b*/> = T
+
+goTo.select("a", "b");
+edit.applyRefactor({
+    refactorName: "Extract type",
+    actionName: "Extract to type alias",
+    actionDescription: "Extract to type alias",
+    newContent:
+`type /*RENAME*/NewType = {
+    x: string;
+    };
+
+type Foo<T extends NewType> = T`,
+});