Added @ts-expect-error to @ts-ignore directives (#36014)

* Added @ts-expect-error to @ts-ignore directives Similar to `// @ts-ignore`, but will itself cause a new error diagnostic if it does not cause an existing diagnostic to be ignored. Technical summary: 1. The scanner will now keep track of `CommentDirective`s it comes across: both `@ts-expect-error` and `@ts-ignore` 2. During type checking, the program will turn those directives into a map keying them by line number 3. For each diagnostic, if it's preceded by a directive, that directive is marked as "used" 4. All `@ts-expect-error` directives not marked as used generate a new diagnostic error * Renamed to getDiagnosticsWithPrecedingDirectives per suggestion * Added JSDoc comment I thought I did already Co-authored-by: Orta <orta.therox+github@gmail.com>
microsoft · Mar 5, 2020 · ffde923 · ffde923
1 parent 3c0c01c
commit ffde923
Show file tree

Hide file tree

Showing 17 changed files with 415 additions and 41 deletions.
diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json
@@ -2245,6 +2245,10 @@
         "category": "Error",
         "code": 2577
     },
+    "Unused '@ts-expect-error' directive.": {
+        "category": "Error",
+        "code": 2578
+    },
     "Cannot find name '{0}'. Do you need to install type definitions for node? Try `npm i @types/node`.": {
         "category": "Error",
         "code": 2580

diff --git a/src/compiler/parser.ts b/src/compiler/parser.ts
@@ -915,6 +915,7 @@ namespace ts {
 
         function clearState() {
             // Clear out the text the scanner is pointing at, so it doesn't keep anything alive unnecessarily.
+            scanner.clearCommentDirectives();
             scanner.setText("");
             scanner.setOnError(undefined);
 
@@ -948,6 +949,7 @@ namespace ts {
 
             setExternalModuleIndicator(sourceFile);
 
+            sourceFile.commentDirectives = scanner.getCommentDirectives();
             sourceFile.nodeCount = nodeCount;
             sourceFile.identifierCount = identifierCount;
             sourceFile.identifiers = identifiers;

diff --git a/src/compiler/program.ts b/src/compiler/program.ts
@@ -1,6 +1,4 @@
 namespace ts {
-    const ignoreDiagnosticCommentRegEx = /(^\s*$)|(^\s*\/\/\/?\s*(@ts-ignore)?)/;
-
     export function findConfigFile(searchPath: string, fileExists: (fileName: string) => boolean, configName = "tsconfig.json"): string | undefined {
         return forEachAncestorDirectory(searchPath, ancestor => {
             const fileName = combinePaths(ancestor, configName);
@@ -1661,17 +1659,16 @@ namespace ts {
             const fileProcessingDiagnosticsInFile = fileProcessingDiagnostics.getDiagnostics(sourceFile.fileName);
             const programDiagnosticsInFile = programDiagnostics.getDiagnostics(sourceFile.fileName);
 
-            let diagnostics: Diagnostic[] | undefined;
-            for (const diags of [fileProcessingDiagnosticsInFile, programDiagnosticsInFile]) {
-                if (diags) {
-                    for (const diag of diags) {
-                        if (shouldReportDiagnostic(diag)) {
-                            diagnostics = append(diagnostics, diag);
-                        }
-                    }
-                }
+            return getMergedProgramDiagnostics(sourceFile, fileProcessingDiagnosticsInFile, programDiagnosticsInFile);
+        }
+
+        function getMergedProgramDiagnostics(sourceFile: SourceFile, ...allDiagnostics: (readonly Diagnostic[] | undefined)[]) {
+            const flatDiagnostics = flatten(allDiagnostics);
+            if (!sourceFile.commentDirectives?.length) {
+                return flatDiagnostics;
             }
-            return diagnostics || emptyArray;
+
+            return getDiagnosticsWithPrecedingDirectives(sourceFile, sourceFile.commentDirectives, flatDiagnostics).diagnostics;
         }
 
         function getDeclarationDiagnostics(sourceFile?: SourceFile, cancellationToken?: CancellationToken): readonly DiagnosticWithLocation[] {
@@ -1749,49 +1746,72 @@ namespace ts {
                 const bindDiagnostics: readonly Diagnostic[] = includeBindAndCheckDiagnostics ? sourceFile.bindDiagnostics : emptyArray;
                 const checkDiagnostics = includeBindAndCheckDiagnostics ? typeChecker.getDiagnostics(sourceFile, cancellationToken) : emptyArray;
 
-                let diagnostics: Diagnostic[] | undefined;
-                for (const diags of [bindDiagnostics, checkDiagnostics, isCheckJs ? sourceFile.jsDocDiagnostics : undefined]) {
-                    if (diags) {
-                        for (const diag of diags) {
-                            if (shouldReportDiagnostic(diag)) {
-                                diagnostics = append(diagnostics, diag);
-                            }
-                        }
-                    }
-                }
-                return diagnostics || emptyArray;
+                return getMergedBindAndCheckDiagnostics(sourceFile, bindDiagnostics, checkDiagnostics, isCheckJs ? sourceFile.jsDocDiagnostics : undefined);
             });
         }
 
+        function getMergedBindAndCheckDiagnostics(sourceFile: SourceFile, ...allDiagnostics: (readonly Diagnostic[] | undefined)[]) {
+            const flatDiagnostics = flatten(allDiagnostics);
+            if (!sourceFile.commentDirectives?.length) {
+                return flatDiagnostics;
+            }
+
+            const { diagnostics, directives } = getDiagnosticsWithPrecedingDirectives(sourceFile, sourceFile.commentDirectives, flatDiagnostics);
+
+            for (const errorExpectation of directives.getUnusedExpectations()) {
+                diagnostics.push(createDiagnosticForRange(sourceFile, errorExpectation.range, Diagnostics.Unused_ts_expect_error_directive));
+            }
+
+            return diagnostics;
+        }
+
+        /**
+         * Creates a map of comment directives along with the diagnostics immediately preceded by one of them.
+         * Comments that match to any of those diagnostics are marked as used.
+         */
+        function getDiagnosticsWithPrecedingDirectives(sourceFile: SourceFile, commentDirectives: CommentDirective[], flatDiagnostics: Diagnostic[]) {
+            // Diagnostics are only reported if there is no comment directive preceding them
+            // This will modify the directives map by marking "used" ones with a corresponding diagnostic
+            const directives = createCommentDirectivesMap(sourceFile, commentDirectives);
+            const diagnostics = flatDiagnostics.filter(diagnostic => markPrecedingCommentDirectiveLine(diagnostic, directives) === -1);
+
+            return { diagnostics, directives };
+        }
+
         function getSuggestionDiagnostics(sourceFile: SourceFile, cancellationToken: CancellationToken): readonly DiagnosticWithLocation[] {
             return runWithCancellationToken(() => {
                 return getDiagnosticsProducingTypeChecker().getSuggestionDiagnostics(sourceFile, cancellationToken);
             });
         }
 
         /**
-         * Skip errors if previous line start with '// @ts-ignore' comment, not counting non-empty non-comment lines
+         * @returns The line index marked as preceding the diagnostic, or -1 if none was.
          */
-        function shouldReportDiagnostic(diagnostic: Diagnostic) {
+        function markPrecedingCommentDirectiveLine(diagnostic: Diagnostic, directives: CommentDirectivesMap) {
             const { file, start } = diagnostic;
-            if (file) {
-                const lineStarts = getLineStarts(file);
-                let { line } = computeLineAndCharacterOfPosition(lineStarts, start!); // TODO: GH#18217
-                while (line > 0) {
-                    const previousLineText = file.text.slice(lineStarts[line - 1], lineStarts[line]);
-                    const result = ignoreDiagnosticCommentRegEx.exec(previousLineText);
-                    if (!result) {
-                        // non-empty line
-                        return true;
-                    }
-                    if (result[3]) {
-                        // @ts-ignore
-                        return false;
-                    }
-                    line--;
+            if (!file) {
+                return -1;
+            }
+
+            // Start out with the line just before the text
+            const lineStarts = getLineStarts(file);
+            let line = computeLineAndCharacterOfPosition(lineStarts, start!).line - 1; // TODO: GH#18217
+            while (line >= 0) {
+                // As soon as that line is known to have a comment directive, use that
+                if (directives.markUsed(line)) {
+                    return line;
                 }
+
+                // Stop searching if the line is not empty and not a comment
+                const lineText = file.text.slice(lineStarts[line - 1], lineStarts[line]).trim();
+                if (lineText !== "" && !/^(\s*)\/\/(.*)$/.test(lineText)) {
+                    return -1;
+                }
+
+                line--;
             }
-            return true;
+
+            return -1;
         }
 
         function getJSSyntacticDiagnosticsForFile(sourceFile: SourceFile): DiagnosticWithLocation[] {

diff --git a/src/compiler/scanner.ts b/src/compiler/scanner.ts
diff --git a/src/compiler/types.ts b/src/compiler/types.ts
@@ -2990,6 +2990,8 @@ namespace ts {
         // This field should never be used directly to obtain line map, use getLineMap function instead.
         /* @internal */ lineMap: readonly number[];
         /* @internal */ classifiableNames?: ReadonlyUnderscoreEscapedMap<true>;
+        // Comments containing @ts-* directives, in order.
+        /* @internal */ commentDirectives?: CommentDirective[];
         // Stores a mapping 'external module reference text' -> 'resolved file name' | undefined
         // It is used to resolve module names in the checker.
         // Content of this field should never be used directly - use getResolvedModuleFileName/setResolvedModuleFileName functions instead
@@ -3009,6 +3011,18 @@ namespace ts {
         /*@internal*/ exportedModulesFromDeclarationEmit?: ExportedModulesFromDeclarationEmit;
     }
 
+    /* @internal */
+    export interface CommentDirective {
+        range: TextRange;
+        type: CommentDirectiveType,
+    }
+
+    /* @internal */
+    export const enum CommentDirectiveType {
+        ExpectError,
+        Ignore,
+    }
+
     /*@internal*/
     export type ExportedModulesFromDeclarationEmit = readonly Symbol[];
 
@@ -6667,6 +6681,12 @@ namespace ts {
         forEach(action: <TKey extends keyof PragmaPseudoMap>(value: PragmaPseudoMap[TKey] | PragmaPseudoMap[TKey][], key: TKey) => void): void;
     }
 
+    /* @internal */
+    export interface CommentDirectivesMap {
+        getUnusedExpectations(): CommentDirective[];
+        markUsed(matchedLine: number): boolean;
+    }
+
     export interface UserPreferences {
         readonly disableSuggestions?: boolean;
         readonly quotePreference?: "auto" | "double" | "single";

diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts
@@ -470,6 +470,34 @@ namespace ts {
             text.charCodeAt(start + 2) === CharacterCodes.exclamation;
     }
 
+    export function createCommentDirectivesMap(sourceFile: SourceFile, commentDirectives: CommentDirective[]): CommentDirectivesMap {
+        const directivesByLine = createMapFromEntries(
+            commentDirectives.map(commentDirective => ([
+                `${getLineAndCharacterOfPosition(sourceFile, commentDirective.range.pos).line}`,
+                commentDirective,
+            ]))
+        );
+
+        const usedLines = createMap<boolean>();
+
+        return { getUnusedExpectations, markUsed };
+
+        function getUnusedExpectations() {
+            return arrayFrom(directivesByLine.entries())
+                .filter(([line, directive]) => directive.type === CommentDirectiveType.ExpectError && !usedLines.get(line))
+                .map(([_, directive]) => directive);
+        }
+
+        function markUsed(line: number) {
+            if (!directivesByLine.has(`${line}`)) {
+                return false;
+            }
+
+            usedLines.set(`${line}`, true);
+            return true;
+        }
+    }
+
     export function getTokenPosOfNode(node: Node, sourceFile?: SourceFileLike, includeJsDoc?: boolean): number {
         // With nodes that have no width (i.e. 'Missing' nodes), we actually *don't*
         // want to skip trivia because this will launch us forward to the next token.
@@ -914,6 +942,17 @@ namespace ts {
         };
     }
 
+    export function createDiagnosticForRange(sourceFile: SourceFile, range: TextRange, message: DiagnosticMessage): DiagnosticWithLocation {
+        return {
+            file: sourceFile,
+            start: range.pos,
+            length: range.end - range.pos,
+            code: message.code,
+            category: message.category,
+            messageText: message.message,
+        };
+    }
+
     export function getSpanOfTokenAtPosition(sourceFile: SourceFile, pos: number): TextSpan {
         const scanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/ true, sourceFile.languageVariant, sourceFile.text, /*onError:*/ undefined, pos);
         scanner.scan();

diff --git a/src/services/services.ts b/src/services/services.ts
@@ -614,6 +614,7 @@ namespace ts {
         private namedDeclarations: Map<Declaration[]> | undefined;
         public ambientModuleNames!: string[];
         public checkJsDirective: CheckJsDirective | undefined;
+        public errorExpectations: TextRange[] | undefined;
         public possiblyContainDynamicImport?: boolean;
         public pragmas!: PragmaMap;
         public localJsxFactory: EntityName | undefined;

diff --git a/tests/baselines/reference/ts-expect-error.errors.txt b/tests/baselines/reference/ts-expect-error.errors.txt
@@ -0,0 +1,28 @@
+tests/cases/conformance/directives/ts-expect-error.ts(4,1): error TS2578: Unused '@ts-expect-error' directive.
+tests/cases/conformance/directives/ts-expect-error.ts(10,1): error TS2578: Unused '@ts-expect-error' directive.
+tests/cases/conformance/directives/ts-expect-error.ts(13,5): error TS2322: Type '"nope"' is not assignable to type 'number'.
+
+
+==== tests/cases/conformance/directives/ts-expect-error.ts (3 errors) ====
+    // @ts-expect-error additional commenting
+    var invalidCommentedFancy: number = 'nope';
+
+    // @ts-expect-error additional commenting
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+!!! error TS2578: Unused '@ts-expect-error' directive.
+    var validCommentedFancy: string = 'nope';
+
+    // @ts-expect-error
+    var invalidCommentedPlain: number = 'nope';
+
+    // @ts-expect-error
+    ~~~~~~~~~~~~~~~~~~~
+!!! error TS2578: Unused '@ts-expect-error' directive.
+    var validCommentedPlain: string = 'nope';
+
+    var invalidPlain: number = 'nope';
+        ~~~~~~~~~~~~
+!!! error TS2322: Type '"nope"' is not assignable to type 'number'.
+
+    var validPlain: string = 'nope';
+
diff --git a/tests/baselines/reference/ts-expect-error.js b/tests/baselines/reference/ts-expect-error.js
@@ -0,0 +1,29 @@
+//// [ts-expect-error.ts]
+// @ts-expect-error additional commenting
+var invalidCommentedFancy: number = 'nope';
+
+// @ts-expect-error additional commenting
+var validCommentedFancy: string = 'nope';
+
+// @ts-expect-error
+var invalidCommentedPlain: number = 'nope';
+
+// @ts-expect-error
+var validCommentedPlain: string = 'nope';
+
+var invalidPlain: number = 'nope';
+
+var validPlain: string = 'nope';
+
+
+//// [ts-expect-error.js]
+// @ts-expect-error additional commenting
+var invalidCommentedFancy = 'nope';
+// @ts-expect-error additional commenting
+var validCommentedFancy = 'nope';
+// @ts-expect-error
+var invalidCommentedPlain = 'nope';
+// @ts-expect-error
+var validCommentedPlain = 'nope';
+var invalidPlain = 'nope';
+var validPlain = 'nope';
diff --git a/tests/baselines/reference/ts-expect-error.symbols b/tests/baselines/reference/ts-expect-error.symbols
@@ -0,0 +1,23 @@
+=== tests/cases/conformance/directives/ts-expect-error.ts ===
+// @ts-expect-error additional commenting
+var invalidCommentedFancy: number = 'nope';
+>invalidCommentedFancy : Symbol(invalidCommentedFancy, Decl(ts-expect-error.ts, 1, 3))
+
+// @ts-expect-error additional commenting
+var validCommentedFancy: string = 'nope';
+>validCommentedFancy : Symbol(validCommentedFancy, Decl(ts-expect-error.ts, 4, 3))
+
+// @ts-expect-error
+var invalidCommentedPlain: number = 'nope';
+>invalidCommentedPlain : Symbol(invalidCommentedPlain, Decl(ts-expect-error.ts, 7, 3))
+
+// @ts-expect-error
+var validCommentedPlain: string = 'nope';
+>validCommentedPlain : Symbol(validCommentedPlain, Decl(ts-expect-error.ts, 10, 3))
+
+var invalidPlain: number = 'nope';
+>invalidPlain : Symbol(invalidPlain, Decl(ts-expect-error.ts, 12, 3))
+
+var validPlain: string = 'nope';
+>validPlain : Symbol(validPlain, Decl(ts-expect-error.ts, 14, 3))
+
diff --git a/tests/baselines/reference/ts-expect-error.types b/tests/baselines/reference/ts-expect-error.types
@@ -0,0 +1,29 @@
+=== tests/cases/conformance/directives/ts-expect-error.ts ===
+// @ts-expect-error additional commenting
+var invalidCommentedFancy: number = 'nope';
+>invalidCommentedFancy : number
+>'nope' : "nope"
+
+// @ts-expect-error additional commenting
+var validCommentedFancy: string = 'nope';
+>validCommentedFancy : string
+>'nope' : "nope"
+
+// @ts-expect-error
+var invalidCommentedPlain: number = 'nope';
+>invalidCommentedPlain : number
+>'nope' : "nope"
+
+// @ts-expect-error
+var validCommentedPlain: string = 'nope';
+>validCommentedPlain : string
+>'nope' : "nope"
+
+var invalidPlain: number = 'nope';
+>invalidPlain : number
+>'nope' : "nope"
+
+var validPlain: string = 'nope';
+>validPlain : string
+>'nope' : "nope"
+