Improve performance: do not add unused suggestion diagnostics unless …

…asking for a suggestion
microsoft · Mar 12, 2018 · a07f9f4 · a07f9f4
1 parent eaf1575
commit a07f9f4
Show file tree

Hide file tree

Showing 14 changed files with 63 additions and 62 deletions.
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
@@ -311,7 +311,22 @@ namespace ts {
                 return node && getTypeArgumentConstraint(node);
             },
 
-            getSuggestionDiagnostics: file => (suggestionDiagnostics.get(file.fileName) || emptyArray).concat(getUnusedDiagnostics(file)),
+            getSuggestionDiagnostics: file => {
+                return (suggestionDiagnostics.get(file.fileName) || emptyArray).concat(getUnusedDiagnostics());
+                function getUnusedDiagnostics(): ReadonlyArray<Diagnostic> {
+                    checkSourceFile(file);
+                    const diagnostics: Diagnostic[] = [];
+                    Debug.assert(!!(getNodeLinks(file).flags & NodeCheckFlags.TypeChecked));
+                    if (!file.isDeclarationFile) {
+                        checkUnusedIdentifiers(allPotentiallyUnusedIdentifiers.get(file.fileName)!, (kind, diag) => {
+                            if (!unusedIsError(kind)) {
+                                diagnostics.push({ ...diag, category: DiagnosticCategory.Suggestion });
+                            }
+                        });
+                    }
+                    return diagnostics;
+                }
+            },
         };
 
         const tupleTypes: GenericType[] = [];
@@ -18732,7 +18747,6 @@ namespace ts {
             return type;
         }
 
-        //why?
         function checkFunctionExpressionOrObjectLiteralMethodDeferred(node: ArrowFunction | FunctionExpression | MethodDeclaration) {
             Debug.assert(node.kind !== SyntaxKind.MethodDeclaration || isObjectLiteralMethod(node));
 
@@ -21564,11 +21578,16 @@ namespace ts {
         }
 
         function registerForUnusedIdentifiersCheck(node: PotentiallyUnusedIdentifier): void {
+            // May be in a call such as getTypeOfNode that happened to call this. But potentiallyUnusedIdentifiers is only defined in the scope of `checkSourceFile`.
+            if (potentiallyUnusedIdentifiers === undefined) return;
+
             if (contains(potentiallyUnusedIdentifiers, node)) {
-                // TODO: #22491 Apparently we check the BlockStatement in the callback in `getPropertyAssignment` twice.
+                // TODO: GH#22491 Apparently we check the BlockStatement in the callback in `getPropertyAssignment` twice.
                 // Debug.fail();
-            } else
+            }
+            else {
                 potentiallyUnusedIdentifiers.push(node);
+            }
         }
 
         type PotentiallyUnusedIdentifier =
@@ -21581,15 +21600,15 @@ namespace ts {
                 switch (node.kind) {
                     case SyntaxKind.SourceFile:
                     case SyntaxKind.ModuleDeclaration:
-                        checkUnusedModuleMembers(<ModuleDeclaration | SourceFile>node, addDiagnostic);
+                        checkUnusedModuleMembers(node, addDiagnostic);
                         break;
                     case SyntaxKind.ClassDeclaration:
                     case SyntaxKind.ClassExpression:
-                        checkUnusedClassMembers(<ClassDeclaration | ClassExpression>node, addDiagnostic);
-                        checkUnusedTypeParameters(<ClassDeclaration | ClassExpression>node, addDiagnostic);
+                        checkUnusedClassMembers(node, addDiagnostic);
+                        checkUnusedTypeParameters(node, addDiagnostic);
                         break;
                     case SyntaxKind.InterfaceDeclaration:
-                        checkUnusedTypeParameters(<InterfaceDeclaration>node, addDiagnostic);
+                        checkUnusedTypeParameters(node, addDiagnostic);
                         break;
                     case SyntaxKind.Block:
                     case SyntaxKind.CaseBlock:
@@ -21605,18 +21624,18 @@ namespace ts {
                     case SyntaxKind.MethodDeclaration:
                     case SyntaxKind.GetAccessor:
                     case SyntaxKind.SetAccessor:
-                        if ((<FunctionLikeDeclaration>node).body) {
-                            checkUnusedLocalsAndParameters(<FunctionLikeDeclaration>node, addDiagnostic);
+                        if (node.body) {
+                            checkUnusedLocalsAndParameters(node, addDiagnostic);
                         }
-                        checkUnusedTypeParameters(<FunctionLikeDeclaration>node, addDiagnostic);
+                        checkUnusedTypeParameters(node, addDiagnostic);
                         break;
                     case SyntaxKind.MethodSignature:
                     case SyntaxKind.CallSignature:
                     case SyntaxKind.ConstructSignature:
                     case SyntaxKind.FunctionType:
                     case SyntaxKind.ConstructorType:
                     case SyntaxKind.TypeAliasDeclaration:
-                        checkUnusedTypeParameters(<MethodSignature | CallSignatureDeclaration | ConstructSignatureDeclaration | FunctionTypeNode | ConstructorTypeNode | TypeAliasDeclaration>node, addDiagnostic);
+                        checkUnusedTypeParameters(node, addDiagnostic);
                         break;
                     default:
                         Debug.assertNever(node, "Node should not have been registered for unused identifiers check");
@@ -24549,19 +24568,6 @@ namespace ts {
             }
         }
 
-        function getUnusedDiagnostics(node: SourceFile): ReadonlyArray<Diagnostic> {
-            const diagnostics: Diagnostic[] = [];
-            Debug.assert(!!(getNodeLinks(node).flags & NodeCheckFlags.TypeChecked));
-            if (!node.isDeclarationFile) {
-                checkUnusedIdentifiers(allPotentiallyUnusedIdentifiers.get(node.fileName)!, (kind, diag) => {
-                    if (!unusedIsError(kind)) {
-                        diagnostics.push(diag);
-                    }
-                });
-            }
-            return diagnostics;
-        }
-
         // Fully type check a source file and collect the relevant diagnostics.
         function checkSourceFileWorker(node: SourceFile) {
             const links = getNodeLinks(node);
@@ -24593,7 +24599,7 @@ namespace ts {
                     registerForUnusedIdentifiersCheck(node);
                 }
 
-                if (!node.isDeclarationFile) { //&& (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters)) {
+                if (!node.isDeclarationFile && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters)) {
                     checkUnusedIdentifiers(potentiallyUnusedIdentifiers, (kind, diag) => {
                         if (unusedIsError(kind)) {
                             diagnostics.add(diag);

diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc15.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc15.ts
@@ -30,14 +30,6 @@ verify.codeFix({
  * @param {promise<String>} zeta
  */
 function f(x: boolean, y: string, z: number, alpha: object, beta: Date, gamma: Promise<any>, delta: Array<any>, epsilon: Array<number>, zeta: Promise<string>) {
-    x;
-    y;
-    z;
-    alpha;
-    beta;
-    gamma;
-    delta;
-    epsilon;
-    zeta;
+    x; y; z; alpha; beta; gamma; delta; epsilon; zeta;
 }`,
 });
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc22.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc22.ts
@@ -14,7 +14,6 @@ verify.codeFix({
 /** @param {Object<string, boolean>} sb
   * @param {Object<number, string>} ns */
 function f(sb: { [s: string]: boolean; }, ns: { [n: number]: string; }) {
-    sb;
-    ns;
+    sb; ns;
 }`,
 });
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc3.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc3.ts
@@ -27,10 +27,6 @@ verify.codeFix({
  * @param {*} beta - I have no idea how this got here
  */
 function f(x: number, y: { a: string; b: Date; }, z: string, alpha, beta: any) {
-    x;
-    y;
-    z;
-    alpha;
-    beta;
+    x; y; z; alpha; beta;
 }`,
 });
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc4.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc4.ts
@@ -26,12 +26,6 @@ verify.codeFix({
  * @param {number!} delta
  */
 function f(x: any, y: any, z: number | undefined, alpha: number[], beta: (this: { a: string; }, arg1: string, arg2: number) => boolean, gamma: number | null, delta: number) {
-    x;
-    y;
-    z;
-    alpha;
-    beta;
-    gamma;
-    delta;
+    x; y; z; alpha; beta; gamma; delta;
 }`,
 });
diff --git a/tests/cases/fourslash/annotateWithTypeFromJSDoc8.ts b/tests/cases/fourslash/annotateWithTypeFromJSDoc8.ts
@@ -15,7 +15,7 @@ verify.codeFix({
  * @param {number} x
  * @returns {number}
  */
-var f = function(x: number): number {
+var f = function (x: number): number {
     return x
 }`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts b/tests/cases/fourslash/refactorConvertToEs6Module_export_referenced.ts
@@ -11,12 +11,13 @@
 ////exports.y;
 ////
 ////exports.z = 2;
-////function f(z) {
-////    exports.z;
+////exports.f = function(z) {
+////    exports.z; z;
 ////}
 
 verify.codeFix({
     description: "Convert to ES6 module",
+    // TODO: GH#22492
     newFileContent:
 `export const x = 0;
 x;
@@ -28,7 +29,7 @@ _y;
 
 const _z = 2;
 export { _z as z };
-function f(z) {
-    _z;
+export function f(z) {
+        _z  z;
 }`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts b/tests/cases/fourslash/refactorConvertToEs6Module_expressionToDeclaration.ts
@@ -3,13 +3,13 @@
 // @allowJs: true
 
 // @Filename: /a.js
-////exports.f = async function* f(p) {}
+////exports.f = async function* f(p) { p; }
 ////exports.C = class C extends D { m() {} }
 
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
-`export async function* f(p) { }
+`export async function* f(p) { p; }
 export class C extends D {
     m() { }
 }`,

diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_arrayBindingPattern.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_arrayBindingPattern.ts
@@ -4,9 +4,11 @@
 
 // @Filename: /a.js
 ////const [x, y] = /*a*/require/*b*/("x");
+////x; y;
 
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent: `import _x from "x";
-const [x, y] = _x;`,
+const [x, y] = _x;
+x; y;`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleUniqueIdentifiers.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleUniqueIdentifiers.ts
@@ -6,6 +6,7 @@
 ////const x = require("x");
 ////const [a, b] = require("x");
 ////const {c, ...d} = require("x");
+////x; a; b; c; d;
 
 verify.codeFix({
     description: "Convert to ES6 module",
@@ -14,5 +15,6 @@ verify.codeFix({
 import _x from "x";
 const [a, b] = _x;
 import __x from "x";
-const { c, ...d } = __x;`,
+const { c, ...d } = __x;
+x; a; b; c; d;`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleVariableDeclarations.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_multipleVariableDeclarations.ts
@@ -6,11 +6,13 @@
 
 // @Filename: /a.js
 ////const x = require("x"), y = 0, { z } = require("z");
+////x; y; z;
 
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
 `import x from "x";
 const y = 0;
-import { z } from "z";`,
+import { z } from "z";
+x; y; z;`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_complex.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_complex.ts
@@ -4,10 +4,12 @@
 
 // @Filename: /a.js
 ////const { x: { a, b } } = require("x");
+////a; b;
 
 verify.codeFix({
     description: "Convert to ES6 module",
     newFileContent:
 `import x from "x";
-const { x: { a, b } } = x;`,
+const { x: { a, b } } = x;
+a; b;`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_plain.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_objectBindingPattern_plain.ts
@@ -4,8 +4,11 @@
 
 // @Filename: /a.js
 ////const { x, y: z } = require("x");
+////x; z;
 
 verify.codeFix({
     description: "Convert to ES6 module",
-    newFileContent: 'import { x, y as z } from "x";',
+    newFileContent:
+`import { x, y as z } from "x";
+x; z;`,
 });
diff --git a/tests/cases/fourslash/refactorConvertToEs6Module_import_propertyAccess.ts b/tests/cases/fourslash/refactorConvertToEs6Module_import_propertyAccess.ts
@@ -8,6 +8,7 @@
 ////const a = require("a").a;
 ////const [a, b] = require("c").d;
 ////const [a, b] = require("c").a; // Test that we avoid shadowing the earlier local variable 'a' from 'const [a,b] = d;'.
+////x; a; b;
 
 verify.codeFix({
     description: "Convert to ES6 module",
@@ -18,5 +19,6 @@ import { a } from "a";
 import { d } from "c";
 const [a, b] = d;
 import { a as _a } from "c";
-const [a, b] = _a; // Test that we avoid shadowing the earlier local variable 'a' from 'const [a,b] = d;'.`,
+const [a, b] = _a; // Test that we avoid shadowing the earlier local variable 'a' from 'const [a,b] = d;'.
+x; a; b;`,
 });