Report grammar errors for invalid decorator grammar (#57749)

microsoft · Mar 13, 2024 · 5e8f900 · 5e8f900
1 parent c1f0f7c
commit 5e8f900
Show file tree

Hide file tree

Showing 34 changed files with 1,550 additions and 47 deletions.
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -651,6 +651,7 @@ import {
     isNewExpression,
     isNodeDescendantOf,
     isNonNullAccess,
+    isNonNullExpression,
     isNullishCoalesce,
     isNumericLiteral,
     isNumericLiteralName,
@@ -41572,8 +41573,82 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
         }
     }
 
+    function checkGrammarDecorator(decorator: Decorator): boolean {
+        const sourceFile = getSourceFileOfNode(decorator);
+        if (!hasParseDiagnostics(sourceFile)) {
+            let node: Expression = decorator.expression;
+
+            // DecoratorParenthesizedExpression :
+            //   `(` Expression `)`
+
+            if (isParenthesizedExpression(node)) {
+                return false;
+            }
+
+            let canHaveCallExpression = true;
+            let errorNode: Node | undefined;
+            while (true) {
+                // Allow TS syntax such as non-null assertions and instantiation expressions
+                if (isExpressionWithTypeArguments(node) || isNonNullExpression(node)) {
+                    node = node.expression;
+                    continue;
+                }
+
+                // DecoratorCallExpression :
+                //   DecoratorMemberExpression Arguments
+
+                if (isCallExpression(node)) {
+                    if (!canHaveCallExpression) {
+                        errorNode = node;
+                    }
+                    if (node.questionDotToken) {
+                        // Even if we already have an error node, error at the `?.` token since it appears earlier.
+                        errorNode = node.questionDotToken;
+                    }
+                    node = node.expression;
+                    canHaveCallExpression = false;
+                    continue;
+                }
+
+                // DecoratorMemberExpression :
+                //   IdentifierReference
+                //   DecoratorMemberExpression `.` IdentifierName
+                //   DecoratorMemberExpression `.` PrivateIdentifier
+
+                if (isPropertyAccessExpression(node)) {
+                    if (node.questionDotToken) {
+                        // Even if we already have an error node, error at the `?.` token since it appears earlier.
+                        errorNode = node.questionDotToken;
+                    }
+                    node = node.expression;
+                    canHaveCallExpression = false;
+                    continue;
+                }
+
+                if (!isIdentifier(node)) {
+                    // Even if we already have an error node, error at this node since it appears earlier.
+                    errorNode = node;
+                }
+
+                break;
+            }
+
+            if (errorNode) {
+                addRelatedInfo(
+                    error(decorator.expression, Diagnostics.Expression_must_be_enclosed_in_parentheses_to_be_used_as_a_decorator),
+                    createDiagnosticForNode(errorNode, Diagnostics.Invalid_syntax_in_decorator),
+                );
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /** Check a decorator */
     function checkDecorator(node: Decorator): void {
+        checkGrammarDecorator(node);
+
         const signature = getResolvedSignature(node);
         checkDeprecatedSignature(signature, node);
         const returnType = getReturnTypeOfSignature(signature);

diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -1637,6 +1637,14 @@
         "category": "Error",
         "code": 1496
     },
+    "Expression must be enclosed in parentheses to be used as a decorator.": {
+        "category": "Error",
+        "code": 1497
+    },
+    "Invalid syntax in decorator.": {
+        "category": "Error",
+        "code": 1498
+    },
 
     "The types of '{0}' are incompatible between these types.": {
         "category": "Error",
@@ -7792,6 +7800,14 @@
         "category": "Message",
         "code": 95193
     },
+    "Wrap in parentheses": {
+        "category": "Message",
+        "code": 95194
+    },
+    "Wrap all invalid decorator expressions in parentheses": {
+        "category": "Message",
+        "code": 95195
+    },
 
     "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
         "category": "Error",

diff --git a/src/services/_namespaces/ts.codefix.ts b/src/services/_namespaces/ts.codefix.ts
@@ -64,6 +64,7 @@ export * from "../codefixes/useDefaultImport";
 export * from "../codefixes/useBigintLiteral";
 export * from "../codefixes/fixAddModuleReferTypeMissingTypeof";
 export * from "../codefixes/wrapJsxInFragment";
+export * from "../codefixes/wrapDecoratorInParentheses";
 export * from "../codefixes/convertToMappedObjectType";
 export * from "../codefixes/removeAccidentalCallParentheses";
 export * from "../codefixes/removeUnnecessaryAwait";

diff --git a/src/services/codefixes/wrapDecoratorInParentheses.ts b/src/services/codefixes/wrapDecoratorInParentheses.ts
@@ -0,0 +1,35 @@
+import {
+    Debug,
+    Diagnostics,
+    factory,
+    findAncestor,
+    getTokenAtPosition,
+    isDecorator,
+    SourceFile,
+    textChanges,
+} from "../_namespaces/ts";
+import {
+    codeFixAll,
+    createCodeFixAction,
+    registerCodeFix,
+} from "../_namespaces/ts.codefix";
+
+const fixId = "wrapDecoratorInParentheses";
+const errorCodes = [Diagnostics.Expression_must_be_enclosed_in_parentheses_to_be_used_as_a_decorator.code];
+registerCodeFix({
+    errorCodes,
+    getCodeActions: function getCodeActionsToWrapDecoratorExpressionInParentheses(context) {
+        const changes = textChanges.ChangeTracker.with(context, t => makeChange(t, context.sourceFile, context.span.start));
+        return [createCodeFixAction(fixId, changes, Diagnostics.Wrap_in_parentheses, fixId, Diagnostics.Wrap_all_invalid_decorator_expressions_in_parentheses)];
+    },
+    fixIds: [fixId],
+    getAllCodeActions: context => codeFixAll(context, errorCodes, (changes, diag) => makeChange(changes, diag.file, diag.start)),
+});
+
+function makeChange(changeTracker: textChanges.ChangeTracker, sourceFile: SourceFile, pos: number) {
+    const token = getTokenAtPosition(sourceFile, pos);
+    const decorator = findAncestor(token, isDecorator)!;
+    Debug.assert(!!decorator, "Expected position to be owned by a decorator.");
+    const replacement = factory.createParenthesizedExpression(decorator.expression);
+    changeTracker.replaceNode(sourceFile, decorator.expression, replacement);
+}
diff --git a/src/testRunner/compilerRunner.ts b/src/testRunner/compilerRunner.ts
@@ -128,42 +128,43 @@ export class CompilerBaselineRunner extends RunnerBase {
 
 class CompilerTest {
     private static varyBy: readonly string[] = [
-        "module",
-        "moduleResolution",
-        "moduleDetection",
+        "allowArbitraryExtensions",
         "allowImportingTsExtensions",
-        "target",
-        "jsx",
-        "noEmit",
-        "removeComments",
+        "allowSyntheticDefaultImports",
+        "alwaysStrict",
+        "downlevelIteration",
+        "experimentalDecorators",
+        "emitDecoratorMetadata",
+        "esModuleInterop",
+        "exactOptionalPropertyTypes",
         "importHelpers",
         "importHelpers",
-        "downlevelIteration",
         "isolatedModules",
-        "verbatimModuleSyntax",
-        "strict",
+        "jsx",
+        "module",
+        "moduleDetection",
+        "moduleResolution",
+        "noEmit",
         "noImplicitAny",
-        "strictNullChecks",
-        "strictFunctionTypes",
-        "strictBindCallApply",
-        "strictPropertyInitialization",
         "noImplicitThis",
-        "alwaysStrict",
-        "allowSyntheticDefaultImports",
-        "esModuleInterop",
-        "emitDecoratorMetadata",
-        "skipDefaultLibCheck",
+        "noPropertyAccessFromIndexSignature",
+        "noUncheckedIndexedAccess",
         "preserveConstEnums",
+        "removeComments",
+        "resolveJsonModule",
+        "resolvePackageJsonExports",
+        "resolvePackageJsonImports",
+        "skipDefaultLibCheck",
         "skipLibCheck",
-        "exactOptionalPropertyTypes",
+        "strict",
+        "strictBindCallApply",
+        "strictFunctionTypes",
+        "strictNullChecks",
+        "strictPropertyInitialization",
+        "target",
         "useDefineForClassFields",
         "useUnknownInCatchVariables",
-        "noUncheckedIndexedAccess",
-        "noPropertyAccessFromIndexSignature",
-        "resolvePackageJsonExports",
-        "resolvePackageJsonImports",
-        "resolveJsonModule",
-        "allowArbitraryExtensions",
+        "verbatimModuleSyntax",
     ];
     private fileName: string;
     private justName: string;

diff --git a/tests/baselines/reference/decoratorOnClassMethod11.errors.txt b/tests/baselines/reference/decoratorOnClassMethod11.errors.txt
@@ -1,13 +1,13 @@
-decoratorOnClassMethod11.ts(5,10): error TS2331: 'this' cannot be referenced in a module or namespace body.
+decoratorOnClassMethod11.ts(5,11): error TS2331: 'this' cannot be referenced in a module or namespace body.
 
 
 ==== decoratorOnClassMethod11.ts (1 errors) ====
     module M {
         class C {
             decorator(target: Object, key: string): void { }
 
-            @this.decorator
-             ~~~~
+            @(this.decorator)
+              ~~~~
 !!! error TS2331: 'this' cannot be referenced in a module or namespace body.
             method() { }
         }

diff --git a/tests/baselines/reference/decoratorOnClassMethod11.js b/tests/baselines/reference/decoratorOnClassMethod11.js
@@ -5,7 +5,7 @@ module M {
     class C {
         decorator(target: Object, key: string): void { }
 
-        @this.decorator
+        @(this.decorator)
         method() { }
     }
 }
@@ -25,7 +25,7 @@ var M;
         C.prototype.decorator = function (target, key) { };
         C.prototype.method = function () { };
         __decorate([
-            this.decorator
+            (this.decorator)
         ], C.prototype, "method", null);
         return C;
     }());

diff --git a/tests/baselines/reference/decoratorOnClassMethod11.symbols b/tests/baselines/reference/decoratorOnClassMethod11.symbols
@@ -13,7 +13,7 @@ module M {
 >Object : Symbol(Object, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --))
 >key : Symbol(key, Decl(decoratorOnClassMethod11.ts, 2, 33))
 
-        @this.decorator
+        @(this.decorator)
         method() { }
 >method : Symbol(C.method, Decl(decoratorOnClassMethod11.ts, 2, 56))
     }

diff --git a/tests/baselines/reference/decoratorOnClassMethod11.types b/tests/baselines/reference/decoratorOnClassMethod11.types
@@ -12,7 +12,8 @@ module M {
 >target : Object
 >key : string
 
-        @this.decorator
+        @(this.decorator)
+>(this.decorator) : any
 >this.decorator : any
 >this : any
 >decorator : any

diff --git a/tests/baselines/reference/decoratorOnClassMethod12.errors.txt b/tests/baselines/reference/decoratorOnClassMethod12.errors.txt
@@ -1,4 +1,4 @@
-decoratorOnClassMethod12.ts(6,10): error TS2660: 'super' can only be referenced in members of derived classes or object literal expressions.
+decoratorOnClassMethod12.ts(6,11): error TS2660: 'super' can only be referenced in members of derived classes or object literal expressions.
 
 
 ==== decoratorOnClassMethod12.ts (1 errors) ====
@@ -7,8 +7,8 @@ decoratorOnClassMethod12.ts(6,10): error TS2660: 'super' can only be referenced
             decorator(target: Object, key: string): void { }
         }
         class C extends S {
-            @super.decorator
-             ~~~~~
+            @(super.decorator)
+              ~~~~~
 !!! error TS2660: 'super' can only be referenced in members of derived classes or object literal expressions.
             method() { }
         }

diff --git a/tests/baselines/reference/decoratorOnClassMethod12.js b/tests/baselines/reference/decoratorOnClassMethod12.js
@@ -6,7 +6,7 @@ module M {
         decorator(target: Object, key: string): void { }
     }
     class C extends S {
-        @super.decorator
+        @(super.decorator)
         method() { }
     }
 }
@@ -48,7 +48,7 @@ var M;
         }
         C.prototype.method = function () { };
         __decorate([
-            _super.decorator
+            (_super.decorator)
         ], C.prototype, "method", null);
         return C;
     }(S));

diff --git a/tests/baselines/reference/decoratorOnClassMethod12.symbols b/tests/baselines/reference/decoratorOnClassMethod12.symbols
@@ -17,7 +17,7 @@ module M {
 >C : Symbol(C, Decl(decoratorOnClassMethod12.ts, 3, 5))
 >S : Symbol(S, Decl(decoratorOnClassMethod12.ts, 0, 10))
 
-        @super.decorator
+        @(super.decorator)
         method() { }
 >method : Symbol(C.method, Decl(decoratorOnClassMethod12.ts, 4, 23))
     }

diff --git a/tests/baselines/reference/decoratorOnClassMethod12.types b/tests/baselines/reference/decoratorOnClassMethod12.types
@@ -16,7 +16,8 @@ module M {
 >C : C
 >S : S
 
-        @super.decorator
+        @(super.decorator)
+>(super.decorator) : any
 >super.decorator : any
 >super : any
 >decorator : any