Emit unused identifier suggestion diagnostics in declaration files and ambient nodes (microsoft#35119)

Author: andrewbranch

src/compiler/checker.ts

@@ -589,21 +589,16 @@ namespace ts {
                     Debug.assert(!!(getNodeLinks(file).flags & NodeCheckFlags.TypeChecked));
 
                     diagnostics = addRange(diagnostics, suggestionDiagnostics.getDiagnostics(file.fileName));
-                    if (!file.isDeclarationFile && (!unusedIsError(UnusedKind.Local) || !unusedIsError(UnusedKind.Parameter))) {
-                        addUnusedDiagnostics();
-                    }
-                    return diagnostics || emptyArray;
-                }
-                finally {
-                    cancellationToken = undefined;
-                }
-
-                function addUnusedDiagnostics() {
                     checkUnusedIdentifiers(getPotentiallyUnusedIdentifiers(file), (containingNode, kind, diag) => {
-                        if (!containsParseError(containingNode) && !unusedIsError(kind)) {
+                        if (!containsParseError(containingNode) && !unusedIsError(kind, !!(containingNode.flags & NodeFlags.Ambient))) {
                             (diagnostics || (diagnostics = [])).push({ ...diag, category: DiagnosticCategory.Suggestion });
                         }
                     });
+
+                    return diagnostics || emptyArray;
+                }
+                finally {
+                    cancellationToken = undefined;
                 }
             },
 
@@ -29611,7 +29606,7 @@ 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 (produceDiagnostics && !(node.flags & NodeFlags.Ambient)) {
+            if (produceDiagnostics) {
                 const sourceFile = getSourceFileOfNode(node);
                 let potentiallyUnusedIdentifiers = allPotentiallyUnusedIdentifiers.get(sourceFile.path);
                 if (!potentiallyUnusedIdentifiers) {
@@ -29778,8 +29773,6 @@ namespace ts {
         }
 
         function checkUnusedLocalsAndParameters(nodeWithLocals: Node, addDiagnostic: AddUnusedDiagnostic): void {
-            if (nodeWithLocals.flags & NodeFlags.Ambient) return;
-
             // Ideally we could use the ImportClause directly as a key, but must wait until we have full ES6 maps. So must store key along with value.
             const unusedImports = createMap<[ImportClause, ImportedDeclaration[]]>();
             const unusedDestructures = createMap<[ObjectBindingPattern, BindingElement[]]>();
@@ -33151,7 +33144,10 @@ namespace ts {
             performance.measure("Check", "beforeCheck", "afterCheck");
         }
 
-        function unusedIsError(kind: UnusedKind): boolean {
+        function unusedIsError(kind: UnusedKind, isAmbient: boolean): boolean {
+            if (isAmbient) {
+                return false;
+            }
             switch (kind) {
                 case UnusedKind.Local:
                     return !!compilerOptions.noUnusedLocals;
@@ -33191,7 +33187,7 @@ namespace ts {
 
                 if (!node.isDeclarationFile && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters)) {
                     checkUnusedIdentifiers(getPotentiallyUnusedIdentifiers(node), (containingNode, kind, diag) => {
-                        if (!containsParseError(containingNode) && unusedIsError(kind)) {
+                        if (!containsParseError(containingNode) && unusedIsError(kind, !!(containingNode.flags & NodeFlags.Ambient))) {
                             diagnostics.add(diag);
                         }
                     });

src/harness/fourslash.ts

@@ -1135,13 +1135,19 @@ namespace FourSlash {
         }
 
         private testDiagnostics(expected: readonly FourSlashInterface.Diagnostic[], diagnostics: readonly ts.Diagnostic[], category: string) {
-            assert.deepEqual(ts.realizeDiagnostics(diagnostics, "\n"), expected.map((e): ts.RealizedDiagnostic => ({
-                message: e.message,
-                category,
-                code: e.code,
-                ...ts.createTextSpanFromRange(e.range || this.getRanges()[0]),
-                reportsUnnecessary: e.reportsUnnecessary,
-            })));
+            assert.deepEqual(ts.realizeDiagnostics(diagnostics, "\n"), expected.map((e): ts.RealizedDiagnostic => {
+                const range = e.range || this.getRangesInFile()[0];
+                if (!range) {
+                    this.raiseError("Must provide a range for each expected diagnostic, or have one range in the fourslash source.");
+                }
+                return {
+                    message: e.message,
+                    category,
+                    code: e.code,
+                    ...ts.createTextSpanFromRange(range),
+                    reportsUnnecessary: e.reportsUnnecessary,
+                };
+            }));
         }
 
         public verifyQuickInfoAt(markerName: string | Range, expectedText: string, expectedDocumentation?: string) {
@@ -2134,6 +2140,10 @@ namespace FourSlash {
             return this.testData.ranges;
         }
 
+        public getRangesInFile(fileName = this.activeFile.fileName) {
+            return this.getRanges().filter(r => r.fileName === fileName);
+        }
+
         public rangesByText(): ts.Map<Range[]> {
             if (this.testData.rangesByText) return this.testData.rangesByText;
             const result = ts.createMultiMap<Range>();

tests/cases/fourslash/unusedAmbient1.ts

@@ -0,0 +1,27 @@
+/// <reference path="fourslash.ts" />
+
+// @Filename: /a.ts
+////export {};
+////declare var [|a|]: string;
+
+// @Filename: /b.d.ts
+////declare module 'b' {
+////  var [|b|]: string;
+////  export {};
+////}
+
+goTo.file("/a.ts");
+verify.getSuggestionDiagnostics([{
+  code: ts.Diagnostics._0_is_declared_but_its_value_is_never_read.code,
+  message: "'a' is declared but its value is never read.",
+  reportsUnnecessary: true
+}]);
+verify.getSemanticDiagnostics([]);
+
+goTo.file("/b.d.ts");
+verify.getSuggestionDiagnostics([{
+  code: ts.Diagnostics._0_is_declared_but_its_value_is_never_read.code,
+  message: "'b' is declared but its value is never read.",
+  reportsUnnecessary: true
+}]);
+verify.getSemanticDiagnostics([]);

tests/cases/fourslash/unusedAmbient2.ts

@@ -0,0 +1,38 @@
+/// <reference path="fourslash.ts" />
+
+// @noUnusedLocals: true
+// @noUnusedParameters: true
+
+// @Filename: /a.ts
+////export {};
+////declare var [|a|]: string;
+
+// @Filename: /b.d.ts
+////declare module 'b' {
+////  var [|b|]: string;
+////  function [|f|]([|p|]: string): any;
+////  export {};
+////}
+
+goTo.file("/a.ts");
+verify.getSuggestionDiagnostics([{
+  code: ts.Diagnostics._0_is_declared_but_its_value_is_never_read.code,
+  message: "'a' is declared but its value is never read.",
+  reportsUnnecessary: true
+}]);
+verify.getSemanticDiagnostics([]);
+
+goTo.file("/b.d.ts");
+verify.getSuggestionDiagnostics([{
+  range: test.rangesByText().get("b")[0],
+  code: ts.Diagnostics._0_is_declared_but_its_value_is_never_read.code,
+  message: "'b' is declared but its value is never read.",
+  reportsUnnecessary: true
+}, {
+  range: test.rangesByText().get("f")[0],
+  code: ts.Diagnostics._0_is_declared_but_its_value_is_never_read.code,
+  message: "'f' is declared but its value is never read.",
+  reportsUnnecessary: true
+}]);
+
+verify.getSemanticDiagnostics([]);