Merge pull request #24234 from Microsoft/unusedIdentifierCorrectFile

Unused variable error reporting needs to handle nodes that could not belong to current source file

Author: sheetalkamat

src/compiler/checker.ts

@@ -319,7 +319,7 @@ namespace ts {
                     checkSourceFile(file);
                     const diagnostics: Diagnostic[] = [];
                     Debug.assert(!!(getNodeLinks(file).flags & NodeCheckFlags.TypeChecked));
-                    checkUnusedIdentifiers(allPotentiallyUnusedIdentifiers.get(file.fileName)!, (kind, diag) => {
+                    checkUnusedIdentifiers(getPotentiallyUnusedIdentifiers(file), (kind, diag) => {
                         if (!unusedIsError(kind)) {
                             diagnostics.push({ ...diag, category: DiagnosticCategory.Suggestion });
                         }
@@ -450,9 +450,7 @@ namespace ts {
         let deferredGlobalExtractSymbol: Symbol;
 
         let deferredNodes: Node[];
-        const allPotentiallyUnusedIdentifiers = createMap<ReadonlyArray<PotentiallyUnusedIdentifier>>(); // key is file name
-        let potentiallyUnusedIdentifiers: PotentiallyUnusedIdentifier[]; // Potentially unused identifiers in the source file currently being checked.
-        const seenPotentiallyUnusedIdentifiers = createMap<true>(); // For assertion that we don't defer the same identifier twice
+        const allPotentiallyUnusedIdentifiers = createMap<PotentiallyUnusedIdentifier[]>(); // key is file name
 
         let flowLoopStart = 0;
         let flowLoopCount = 0;
@@ -22553,7 +22551,13 @@ 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) {
+            if (produceDiagnostics) {
+                const sourceFile = getSourceFileOfNode(node);
+                let potentiallyUnusedIdentifiers = allPotentiallyUnusedIdentifiers.get(sourceFile.path);
+                if (!potentiallyUnusedIdentifiers) {
+                    potentiallyUnusedIdentifiers = [];
+                    allPotentiallyUnusedIdentifiers.set(sourceFile.path, potentiallyUnusedIdentifiers);
+                }
                 // TODO: GH#22580
                 // Debug.assert(addToSeen(seenPotentiallyUnusedIdentifiers, getNodeId(node)), "Adding potentially-unused identifier twice");
                 potentiallyUnusedIdentifiers.push(node);
@@ -25535,6 +25539,10 @@ namespace ts {
             }
         }
 
+        function getPotentiallyUnusedIdentifiers(sourceFile: SourceFile): ReadonlyArray<PotentiallyUnusedIdentifier> {
+            return allPotentiallyUnusedIdentifiers.get(sourceFile.path) || emptyArray;
+        }
+
         // Fully type check a source file and collect the relevant diagnostics.
         function checkSourceFileWorker(node: SourceFile) {
             const links = getNodeLinks(node);
@@ -25553,11 +25561,6 @@ namespace ts {
                 clear(potentialNewTargetCollisions);
 
                 deferredNodes = [];
-                if (produceDiagnostics) {
-                    Debug.assert(!allPotentiallyUnusedIdentifiers.has(node.fileName));
-                    allPotentiallyUnusedIdentifiers.set(node.fileName, potentiallyUnusedIdentifiers = []);
-                }
-
                 forEach(node.statements, checkSourceElement);
 
                 checkDeferredNodes();
@@ -25567,16 +25570,14 @@ namespace ts {
                 }
 
                 if (!node.isDeclarationFile && (compilerOptions.noUnusedLocals || compilerOptions.noUnusedParameters)) {
-                    checkUnusedIdentifiers(potentiallyUnusedIdentifiers, (kind, diag) => {
+                    checkUnusedIdentifiers(getPotentiallyUnusedIdentifiers(node), (kind, diag) => {
                         if (unusedIsError(kind)) {
                             diagnostics.add(diag);
                         }
                     });
                 }
 
                 deferredNodes = undefined;
-                seenPotentiallyUnusedIdentifiers.clear();
-                potentiallyUnusedIdentifiers = undefined;
 
                 if (isExternalOrCommonJsModule(node)) {
                     checkExternalModuleExports(node);

tests/cases/fourslash/suggestionOfUnusedVariableWithExternalModule.ts

@@ -0,0 +1,31 @@
+/// <reference path='fourslash.ts' />
+
+//@allowJs: true
+
+// @Filename: /mymodule.js
+////(function ([|root|], factory) {
+////    module.exports = factory();
+////}(this, function () {
+////    var [|unusedVar|] = "something";
+////    return {};
+////}));
+
+// @Filename: /app.js
+//////@ts-check
+////require("./mymodule");
+
+const [range0, range1] = test.ranges();
+
+goTo.file("/app.js");
+verify.getSuggestionDiagnostics([]);
+
+goTo.file("/mymodule.js");
+verify.getSuggestionDiagnostics([{
+    message: "'root' is declared but its value is never read.",
+    code: 6133,
+    range: range0
+}, {
+        message: "'unusedVar' is declared but its value is never read.",
+        code: 6133,
+        range: range1
+}]);