TypeScript: Throw errors for invalid modifiers (#13877)

Fixes https://github.com/prettier/prettier/issues/9760

src/language-js/parse/babel.js

@@ -207,8 +207,6 @@ const allowedMessageCodes = new Set([
   "InvalidTupleMemberLabel",
 
   "NonClassMethodPropertyHasAbstractModifer",
-  "ClassMethodHasDeclare",
-  "ClassMethodHasReadonly",
   "DuplicateAccessibilityModifier",
   "IndexSignatureHasDeclare",
 

src/language-js/parse/postprocess/ts-syntax-kind.evaluate.js

@@ -1,11 +0,0 @@
-import ts from "typescript";
-
-export const {
-  AbstractKeyword,
-  SourceFile,
-  DeclareKeyword,
-  PropertyDeclaration,
-  InKeyword,
-  OutKeyword,
-  MethodSignature,
-} = ts.SyntaxKind;

src/language-js/parse/postprocess/typescript.js

@@ -1,10 +1,11 @@
 import isNonEmptyArray from "../../../utils/is-non-empty-array.js";
 import visitNode from "./visit-node.js";
 import throwTsSyntaxError from "./throw-ts-syntax-error.js";
-import * as SyntaxKind from "./ts-syntax-kind.evaluate.js";
+
+let ts;
 
 function getTsNodeLocation(nodeOrToken) {
-  const sourceFile = getSourceFileOfNode(nodeOrToken);
+  const sourceFile = ts.getSourceFileOfNode(nodeOrToken);
   const [start, end] = [nodeOrToken.pos, nodeOrToken.end].map((position) => {
     const { line, character: column } =
       sourceFile.getLineAndCharacterOfPosition(position);
@@ -14,12 +15,8 @@ function getTsNodeLocation(nodeOrToken) {
   return { start, end };
 }
 
-// Copied from https://unpkg.com/typescript@4.8.2/lib/typescript.js
-function getSourceFileOfNode(node) {
-  while (node && node.kind !== SyntaxKind.SourceFile) {
-    node = node.parent;
-  }
-  return node;
+function throwErrorOnTsNode(node, message) {
+  throwTsSyntaxError({ loc: getTsNodeLocation(node) }, message);
 }
 
 // Invalid decorators are removed since `@typescript-eslint/typescript-estree` v4
@@ -33,10 +30,7 @@ function throwErrorForInvalidDecorator(tsNode) {
 
   const [{ expression }] = illegalDecorators;
 
-  throwTsSyntaxError(
-    { loc: getTsNodeLocation(expression) },
-    "Decorators are not valid here."
-  );
+  throwErrorOnTsNode(expression, "Decorators are not valid here.");
 }
 
 // Values of abstract property is removed since `@typescript-eslint/typescript-estree` v5
@@ -44,11 +38,11 @@ function throwErrorForInvalidDecorator(tsNode) {
 function throwErrorForInvalidAbstractProperty(tsNode, esTreeNode) {
   if (
     !(
-      tsNode.kind === SyntaxKind.PropertyDeclaration &&
+      tsNode.kind === ts.SyntaxKind.PropertyDeclaration &&
       tsNode.initializer &&
       esTreeNode.value === null &&
       tsNode.modifiers?.some(
-        (modifier) => modifier.kind === SyntaxKind.AbstractKeyword
+        (modifier) => modifier.kind === ts.SyntaxKind.AbstractKeyword
       )
     )
   ) {
@@ -61,55 +55,86 @@ function throwErrorForInvalidAbstractProperty(tsNode, esTreeNode) {
   );
 }
 
-function throwErrorForInvalidDeclare(tsNode, esTreeNode) {
-  if (
-    !(
-      esTreeNode.type === "MethodDefinition" &&
-      (esTreeNode.kind === "get" || esTreeNode.kind === "set")
-    )
-  ) {
+// Based on `checkGrammarModifiers` function in `typescript`
+function throwErrorForInvalidModifier(node) {
+  const { modifiers } = node;
+  if (!isNonEmptyArray(modifiers)) {
     return;
   }
 
-  const declareKeyword = tsNode.modifiers?.find(
-    (modifier) => modifier.kind === SyntaxKind.DeclareKeyword
-  );
+  const { SyntaxKind } = ts;
 
-  if (!declareKeyword) {
-    return;
+  for (const modifier of modifiers) {
+    if (ts.isDecorator(modifier)) {
+      continue;
+    }
+
+    if (modifier.kind !== SyntaxKind.ReadonlyKeyword) {
+      if (
+        node.kind === SyntaxKind.PropertySignature ||
+        node.kind === SyntaxKind.MethodSignature
+      ) {
+        throwErrorOnTsNode(
+          modifier,
+          `'${ts.tokenToString(
+            modifier.kind
+          )}' modifier cannot appear on a type member`
+        );
+      }
+
+      if (
+        node.kind === SyntaxKind.IndexSignature &&
+        (modifier.kind !== SyntaxKind.StaticKeyword ||
+          !ts.isClassLike(node.parent))
+      ) {
+        throwErrorOnTsNode(
+          modifier,
+          `'${ts.tokenToString(
+            modifier.kind
+          )}' modifier cannot appear on an index signature`
+        );
+      }
+    }
+
+    if (
+      modifier.kind !== SyntaxKind.InKeyword &&
+      modifier.kind !== SyntaxKind.OutKeyword &&
+      node.kind === SyntaxKind.TypeParameter
+    ) {
+      throwErrorOnTsNode(
+        modifier,
+        `'${ts.tokenToString(
+          modifier.kind
+        )}' modifier cannot appear on a type parameter`
+      );
+    }
+
+    if (
+      modifier.kind === SyntaxKind.ReadonlyKeyword &&
+      node.kind !== SyntaxKind.PropertyDeclaration &&
+      node.kind !== SyntaxKind.PropertySignature &&
+      node.kind !== SyntaxKind.IndexSignature &&
+      node.kind !== SyntaxKind.Parameter
+    ) {
+      throwErrorOnTsNode(
+        modifier,
+        "'readonly' modifier can only appear on a property declaration or index signature."
+      );
+    }
+
+    if (
+      modifier.kind === SyntaxKind.DeclareKeyword &&
+      ts.isClassLike(node.parent) &&
+      !ts.isPropertyDeclaration(node)
+    ) {
+      throwErrorOnTsNode(
+        modifier,
+        `'${ts.tokenToString(
+          modifier.kind
+        )}' modifier cannot appear on class elements of this kind.`
+      );
+    }
   }
-
-  throwTsSyntaxError(
-    { loc: getTsNodeLocation(declareKeyword) },
-    /* cspell:disable-next-line */
-    `'declare' is not allowed in ${esTreeNode.kind}ters.`
-  );
-}
-
-function throwErrorForInvalidModifierOnTypeParameter(tsNode, esTreeNode) {
-  if (
-    esTreeNode.type !== "TSMethodSignature" ||
-    tsNode.kind !== SyntaxKind.MethodSignature
-  ) {
-    return;
-  }
-
-  const invalidModifier = tsNode.modifiers?.find(
-    (modifier) =>
-      !(
-        modifier.kind === SyntaxKind.InKeyword ||
-        modifier.kind === SyntaxKind.OutKeyword
-      )
-  );
-
-  if (!invalidModifier) {
-    return;
-  }
-
-  throwTsSyntaxError(
-    { loc: getTsNodeLocation(invalidModifier) },
-    "This modifier cannot appear on a type member"
-  );
 }
 
 function getTsNode(node, tsParseResult) {
@@ -127,27 +152,46 @@ function getTsNode(node, tsParseResult) {
   return tsNode;
 }
 
-function throwErrorForInvalidNodes(tsParseResult, options) {
+async function throwErrorForInvalidNodes(tsParseResult, options) {
   if (
-    // decorators
-    // abstract properties
-    // declare in accessor
-    // modifiers on type parameter
-    !/@|abstract|declare|interface/.test(options.originalText)
+    // decorators and modifiers
+    !new RegExp(
+      [
+        "@",
+        "abstract",
+        "accessor",
+        "async",
+        "const",
+        "declare",
+        "default",
+        "export",
+        "static",
+        "in",
+        "out",
+        "override",
+        "public",
+        "private",
+        "protected",
+        "readonly",
+      ].join("|")
+    ).test(options.originalText)
   ) {
     return;
   }
 
+  if (!ts) {
+    ({ default: ts } = await import("typescript"));
+  }
+
   visitNode(tsParseResult.ast, (esTreeNode) => {
     const tsNode = getTsNode(esTreeNode, tsParseResult);
     if (!tsNode) {
       return;
     }
 
-    throwErrorForInvalidDeclare(tsNode, esTreeNode);
     throwErrorForInvalidDecorator(tsNode);
     throwErrorForInvalidAbstractProperty(tsNode, esTreeNode);
-    throwErrorForInvalidModifierOnTypeParameter(tsNode, esTreeNode);
+    throwErrorForInvalidModifier(tsNode);
   });
 }
 

src/language-js/parse/typescript.js

@@ -57,7 +57,7 @@ async function parse(text, options = {}) {
 
   options.originalText = text;
 
-  throwErrorForInvalidNodes(result, options);
+  await throwErrorForInvalidNodes(result, options);
 
   return postprocess(result.ast, options);
 }

src/language-js/print/class.js

@@ -165,14 +165,6 @@ function printClassMethod(path, options, print) {
   if (node.accessibility) {
     parts.push(node.accessibility + " ");
   }
-  // "readonly" and "declare" are supported by only "babel-ts"
-  // https://github.com/prettier/prettier/issues/9760
-  if (node.readonly) {
-    parts.push("readonly ");
-  }
-  if (node.declare) {
-    parts.push("declare ");
-  }
 
   if (node.static) {
     parts.push("static ");

tests/format/misc/errors/typescript/__snapshots__/jsfmt.spec.js.snap

@@ -1,7 +1,7 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`declare-getter.ts [typescript] format 1`] = `
-"'declare' is not allowed in getters. (1:12)
+"'declare' modifier cannot appear on class elements of this kind. (1:12)
 > 1 | class Foo {
     |            ^
 > 2 |   declare get foo()
@@ -11,7 +11,7 @@ exports[`declare-getter.ts [typescript] format 1`] = `
 `;
 
 exports[`declare-setter.ts [typescript] format 1`] = `
-"'declare' is not allowed in setters. (1:12)
+"'declare' modifier cannot appear on class elements of this kind. (1:12)
 > 1 | class Foo {
     |            ^
 > 2 |   declare set foo(v)
@@ -94,78 +94,6 @@ exports[`snippet: #5 [typescript] format 1`] = `
     |               ^"
 `;
 
-exports[`snippet: #6 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   abstract e();
-    | ^^^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #7 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   declare e();
-    | ^^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #8 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   export e();
-    | ^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #9 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   static e();
-    | ^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #10 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   private e();
-    | ^^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #11 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   protected e();
-    | ^^^^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #12 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   public e();
-    | ^^^^^^^^^
-  3 | }"
-`;
-
-exports[`snippet: #13 [typescript] format 1`] = `
-"This modifier cannot appear on a type member (1:16)
-> 1 | interface Foo {
-    |                ^
-> 2 |   readonly e();
-    | ^^^^^^^^^^^
-  3 | }"
-`;
-
 exports[`value-of-abstract-property.ts [typescript] format 1`] = `
 "Abstract property cannot have an initializer (2:3)
   1 | abstract class Foo {

tests/format/misc/errors/typescript/babel-ts/__snapshots__/jsfmt.spec.js.snap

@@ -43,67 +43,3 @@ exports[`snippet: #2 [babel-ts] format 1`] = `
 > 1 | <string>foo = '100';
     |         ^"
 `;
-
-exports[`snippet: #3 [babel-ts] format 1`] = `
-"'abstract' modifier cannot appear on a type member. (2:3)
-  1 | interface Foo {
-> 2 |   abstract e();
-    |   ^
-  3 | }"
-`;
-
-exports[`snippet: #4 [babel-ts] format 1`] = `
-"'declare' modifier cannot appear on a type member. (2:3)
-  1 | interface Foo {
-> 2 |   declare e();
-    |   ^
-  3 | }"
-`;
-
-exports[`snippet: #5 [babel-ts] format 1`] = `
-"Unexpected token, expected ";" (2:10)
-  1 | interface Foo {
-> 2 |   export e();
-    |          ^
-  3 | }"
-`;
-
-exports[`snippet: #6 [babel-ts] format 1`] = `
-"'static' modifier cannot appear on a type member. (2:3)
-  1 | interface Foo {
-> 2 |   static e();
-    |   ^
-  3 | }"
-`;
-
-exports[`snippet: #7 [babel-ts] format 1`] = `
-"'private' modifier cannot appear on a type member. (2:3)
-  1 | interface Foo {
-> 2 |   private e();
-    |   ^
-  3 | }"
-`;
-
-exports[`snippet: #8 [babel-ts] format 1`] = `
-"'protected' modifier cannot appear on a type member. (2:3)
-  1 | interface Foo {
-> 2 |   protected e();
-    |   ^
-  3 | }"
-`;
-
-exports[`snippet: #9 [babel-ts] format 1`] = `
-"'public' modifier cannot appear on a type member. (2:3)
-  1 | interface Foo {
-> 2 |   public e();
-    |   ^
-  3 | }"
-`;
-
-exports[`snippet: #10 [babel-ts] format 1`] = `
-"'readonly' modifier can only appear on a property declaration or index signature. (2:3)
-  1 | interface Foo {
-> 2 |   readonly e();
-    |   ^
-  3 | }"
-`;

tests/format/misc/errors/typescript/babel-ts/jsfmt.spec.js

@@ -1,5 +1,3 @@
-import { outdent } from "outdent";
-
 run_spec(
   {
     importMeta: import.meta,
@@ -7,22 +5,6 @@ run_spec(
       "foo as any = 10;",
       "({ a: b as any = 2000 } = x);",
       "<string>foo = '100';",
-      ...[
-        "abstract",
-        "declare",
-        "export",
-        "static",
-        "private",
-        "protected",
-        "public",
-        "readonly",
-      ].map(
-        (modifier) => outdent`
-          interface Foo {
-            ${modifier} e();
-          }
-        `
-      ),
     ],
   },
   ["babel-ts"]

tests/format/misc/errors/typescript/jsfmt.spec.js

@@ -1,5 +1,3 @@
-import { outdent } from "outdent";
-
 run_spec(
   {
     importMeta: import.meta,
@@ -10,22 +8,6 @@ run_spec(
       'let x4 = <div>{"foo"}></div>;',
       'let x5 = <div>}{"foo"}</div>;',
       'let x6 = <div>>{"foo"}</div>;',
-      ...[
-        "abstract",
-        "declare",
-        "export",
-        "static",
-        "private",
-        "protected",
-        "public",
-        "readonly",
-      ].map(
-        (modifier) => outdent`
-          interface Foo {
-            ${modifier} e();
-          }
-        `
-      ),
     ],
   },
   ["typescript"]

tests/format/misc/errors/typescript/modifiers/jsfmt.spec.js

@@ -0,0 +1,93 @@
+import { outdent } from "outdent";
+
+const POSSIBLE_MODIFIERS = [
+  "abstract",
+  "accessor",
+  "async",
+  "const",
+  "declare",
+  "default",
+  "export",
+  "static",
+  "in",
+  "out",
+  "override",
+  "public",
+  "private",
+  "protected",
+  "readonly",
+];
+
+run_spec(
+  {
+    importMeta: import.meta,
+    snippets: [
+      // Only `readonly` allowed in some places
+      ...POSSIBLE_MODIFIERS.filter(
+        (modifier) => modifier !== "readonly"
+      ).flatMap((modifier) => [
+        outdent`
+          interface Foo {
+            ${modifier} method();
+          }
+        `,
+        outdent`
+          interface Foo {
+            ${modifier} property;
+          }
+        `,
+        // index signature
+        outdent`
+          interface Foo {
+            ${modifier} [index: string]: number
+          }
+        `,
+        outdent`
+          const foo: {
+            ${modifier} [index: string] : string
+          } = {};
+        `,
+      ]),
+      outdent`
+        interface Foo {
+          readonly method();
+        }
+      `,
+
+      // TODO[@fisker]: Fix these tests
+      // ...["abstract", "static", "private", "protected", "public"].map(
+      //   (modifier) =>
+      //     outdent`
+      //       module Foo {
+      //         ${modifier} module Bar {}
+      //       }
+      //     `
+      // ),
+
+      // Only `in` and `out` allowed in type parameter
+      ...POSSIBLE_MODIFIERS.filter(
+        (modifier) => modifier !== "in" && modifier !== "out"
+      ).map((modifier) => `interface Foo<${modifier} T> {}`),
+
+      ...["declare", "readonly"].map(
+        (modifier) =>
+          outdent`
+            class Foo {
+              ${modifier} method() {}
+            }
+          `
+      ),
+      outdent`
+        class Foo {
+          declare get getter() {}
+        }
+      `,
+      outdent`
+        class Foo {
+          declare set setter(v) {}
+        }
+      `,
+    ],
+  },
+  ["babel-ts", "typescript"]
+);

tests/format/misc/typescript-babel-only/__snapshots__/jsfmt.spec.js.snap

@@ -20,26 +20,6 @@ class C {
 ================================================================================
 `;
 
-exports[`invalid-modifiers.ts format 1`] = `
-====================================options=====================================
-parsers: ["babel-ts"]
-printWidth: 80
-                                                                                | printWidth
-=====================================input======================================
-class Bar {
-  declare e() {};
-  readonly g() {};
-}
-
-=====================================output=====================================
-class Bar {
-  declare e() {}
-  readonly g() {}
-}
-
-================================================================================
-`;
-
 exports[`parenthesized-decorators-call-expression.ts format 1`] = `
 ====================================options=====================================
 parsers: ["babel-ts"]

tests/format/misc/typescript-babel-only/invalid-modifiers.ts

@@ -1,4 +0,0 @@
-class Bar {
-  declare e() {};
-  readonly g() {};
-}

tests/format/misc/typescript-only/__snapshots__/jsfmt.spec.js.snap

@@ -51,28 +51,6 @@ await 0;
 ================================================================================
 `;
 
-exports[`invalid-modifiers.ts format 1`] = `
-====================================options=====================================
-parsers: ["typescript"]
-printWidth: 80
-                                                                                | printWidth
-=====================================input======================================
-class Bar {
-  declare e() {};
-  abstract f() {};
-  readonly g() {};
-}
-
-=====================================output=====================================
-class Bar {
-  e() {}
-  abstract f() {}
-  g() {}
-}
-
-================================================================================
-`;
-
 exports[`parenthesized-decorators-call-expression.ts format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]

tests/format/misc/typescript-only/invalid-modifiers.ts

@@ -1,5 +0,0 @@
-class Bar {
-  declare e() {};
-  abstract f() {};
-  readonly g() {};
-}

tests/format/typescript/keywords/__snapshots__/jsfmt.spec.js.snap

@@ -1,14 +1,14 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
 exports[`keywords.ts [babel-ts] format 1`] = `
-"Unexpected token, expected "{" (9:12)
-   7 |
-   8 |     // Apparently this parses :P
->  9 |     export private public protected static readonly abstract async enum X { }
-     |            ^
-  10 |
-  11 |     interface x {
-  12 |         export private static readonly [x: any]: any;"
+"Missing semicolon. (4:11)
+  2 |
+  3 | module Y4 {
+> 4 |     public enum Color { Blue, Red }
+    |           ^
+  5 | }
+  6 |
+  7 | module YY3 {"
 `;
 
 exports[`keywords.ts format 1`] = `
@@ -19,19 +19,6 @@ printWidth: 80
 =====================================input======================================
 // All of these should be an error
 
-module Y3 {
-    public module Module {
-        class A { s: string }
-    }
-
-    // Apparently this parses :P
-    export private public protected static readonly abstract async enum X { }
-
-    interface x {
-        export private static readonly [x: any]: any;
-    }
-}
-
 module Y4 {
     public enum Color { Blue, Red }
 }
@@ -59,21 +46,6 @@ module YYY4 {
 =====================================output=====================================
 // All of these should be an error
 
-module Y3 {
-  public module Module {
-    class A {
-      s: string;
-    }
-  }
-
-  // Apparently this parses :P
-  export private public protected static readonly abstract async enum X {}
-
-  interface x {
-    export private static readonly [x: any]: any;
-  }
-}
-
 module Y4 {
   public enum Color {
     Blue,
@@ -195,17 +167,6 @@ class G {
 ================================================================================
 `;
 
-exports[`module.ts [babel-ts] format 1`] = `
-"Unexpected token, expected "{" (7:10)
-   5 |
-   6 |   // Apparently this parses :P
->  7 |   export private public protected static readonly abstract async enum X { }
-     |          ^
-   8 |
-   9 |   interface x {
-  10 |       export private static readonly [x: any]: any;"
-`;
-
 exports[`module.ts format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
@@ -213,31 +174,29 @@ printWidth: 80
                                                                                 | printWidth
 =====================================input======================================
 module Y3 {
-  public module Module {
+  module Module {
       class A { s: string }
   }
 
-  // Apparently this parses :P
-  export private public protected static readonly abstract async enum X { }
+  export enum X { }
 
   interface x {
-      export private static readonly [x: any]: any;
+      readonly [x: any]: any;
   }
 }
 
 =====================================output=====================================
 module Y3 {
-  public module Module {
+  module Module {
     class A {
       s: string;
     }
   }
 
-  // Apparently this parses :P
-  export private public protected static readonly abstract async enum X {}
+  export enum X {}
 
   interface x {
-    export private static readonly [x: any]: any;
+    readonly [x: any]: any;
   }
 }
 

tests/format/typescript/keywords/jsfmt.spec.js

@@ -1,3 +1,3 @@
 run_spec(import.meta, ["typescript"], {
-  errors: { "babel-ts": ["module.ts", "keywords.ts"] },
+  errors: { "babel-ts": ["keywords.ts"] },
 });

tests/format/typescript/keywords/keywords.ts

@@ -1,18 +1,5 @@
 // All of these should be an error
 
-module Y3 {
-    public module Module {
-        class A { s: string }
-    }
-
-    // Apparently this parses :P
-    export private public protected static readonly abstract async enum X { }
-
-    interface x {
-        export private static readonly [x: any]: any;
-    }
-}
-
 module Y4 {
     public enum Color { Blue, Red }
 }

tests/format/typescript/keywords/module.ts

@@ -1,12 +1,11 @@
 module Y3 {
-  public module Module {
+  module Module {
       class A { s: string }
   }
 
-  // Apparently this parses :P
-  export private public protected static readonly abstract async enum X { }
+  export enum X { }
 
   interface x {
-      export private static readonly [x: any]: any;
+      readonly [x: any]: any;
   }
 }