followup #18362: make `UnusedImport` work robustly (#18366)

* warnDuplicateModuleImport => hintDuplicateModuleImport
* improve DuplicateModuleImport msg, add test

10 files changed, 32 insertions, 24 deletions

diff --git a/compiler/cgen.nim b/compiler/cgen.nim
index 5d6de2a03..3b004d399 100644
--- a/compiler/cgen.nim
+++ b/compiler/cgen.nim
@@ -24,8 +24,6 @@ import strutils except `%` # collides with ropes.`%`
 
 from ic / ic import ModuleBackendFlag
 from modulegraphs import ModuleGraph, PPassContext
-from lineinfos import
-  warnGcMem, errXMustBeCompileTime, hintDependency, errGenerated, errCannotOpenFile
 import dynlib
 
 when not declared(dynlib.libCandidates):
diff --git a/compiler/dfa.nim b/compiler/dfa.nim
index ea60dbc59..402945042 100644
--- a/compiler/dfa.nim
+++ b/compiler/dfa.nim
@@ -29,7 +29,7 @@
 ## "A Graph–Free Approach to Data–Flow Analysis" by Markus Mohnen.
 ## https://link.springer.com/content/pdf/10.1007/3-540-45937-5_6.pdf
 
-import ast, types, intsets, lineinfos, renderer
+import ast, intsets, lineinfos, renderer
 import std/private/asciitables
 
 type
diff --git a/compiler/docgen.nim b/compiler/docgen.nim
index 08311eade..e6bb7cef8 100644
--- a/compiler/docgen.nim
+++ b/compiler/docgen.nim
@@ -16,7 +16,7 @@ import
   packages/docutils/rst, packages/docutils/rstgen,
   json, xmltree, trees, types,
   typesrenderer, astalgo, lineinfos, intsets,
-  pathutils, trees, tables, nimpaths, renderverbatim, osproc
+  pathutils, tables, nimpaths, renderverbatim, osproc
 
 from uri import encodeUrl
 from std/private/globs import nativeToUnixPath
diff --git a/compiler/importer.nim b/compiler/importer.nim
index 7a5c4de4a..af392f849 100644
--- a/compiler/importer.nim
+++ b/compiler/importer.nim
@@ -12,7 +12,7 @@
 import
   intsets, ast, astalgo, msgs, options, idents, lookups,
   semdata, modulepaths, sigmatch, lineinfos, sets,
-  modulegraphs, wordrecg
+  modulegraphs, wordrecg, tables
 from strutils import `%`
 
 proc readExceptSet*(c: PContext, n: PNode): IntSet =
@@ -239,6 +239,7 @@ proc importModuleAs(c: PContext; n: PNode, realModule: PSym, importHidden: bool)
   if importHidden:
     result.options.incl optImportHidden
   c.unusedImports.add((result, n.info))
+  c.importModuleMap[result.id] = realModule.id
 
 proc transformImportAs(c: PContext; n: PNode): tuple[node: PNode, importHidden: bool] =
   var ret: typeof(result)
@@ -296,6 +297,13 @@ proc myImportModule(c: PContext, n: var PNode, importStmtResult: PNode): PSym =
     importStmtResult.add newSymNode(result, n.info)
     #newStrNode(toFullPath(c.config, f), n.info)
 
+proc afterImport(c: PContext, m: PSym) =
+  # fixes bug #17510, for re-exported symbols
+  let realModuleId = c.importModuleMap[m.id]
+  for s in allSyms(c.graph, m):
+    if s.owner.id != realModuleId:
+      c.exportIndirections.incl((m.id, s.id))
+
 proc impMod(c: PContext; it: PNode; importStmtResult: PNode) =
   var it = it
   let m = myImportModule(c, it, importStmtResult)
@@ -304,9 +312,7 @@ proc impMod(c: PContext; it: PNode; importStmtResult: PNode) =
     addDecl(c, m, it.info) # add symbol to symbol table of module
     importAllSymbols(c, m)
     #importForwarded(c, m.ast, emptySet, m)
-    for s in allSyms(c.graph, m): # fixes bug #17510, for re-exported symbols
-      if s.owner != m:
-        c.exportIndirections.incl((m.id, s.id))
+    afterImport(c, m)
 
 proc evalImport*(c: PContext, n: PNode): PNode =
   result = newNodeI(nkImportStmt, n.info)
@@ -345,6 +351,7 @@ proc evalFrom*(c: PContext, n: PNode): PNode =
       if n[i].kind != nkNilLit:
         importSymbol(c, n[i], m, im.imported)
     c.addImport im
+    afterImport(c, m)
 
 proc evalImportExcept*(c: PContext, n: PNode): PNode =
   result = newNodeI(nkImportStmt, n.info)
@@ -355,3 +362,4 @@ proc evalImportExcept*(c: PContext, n: PNode): PNode =
     addDecl(c, m, n.info)               # add symbol to symbol table of module
     importAllSymbolsExcept(c, m, readExceptSet(c, n))
     #importForwarded(c, m.ast, exceptSet, m)
+    afterImport(c, m)
diff --git a/compiler/injectdestructors.nim b/compiler/injectdestructors.nim
index 8ef08adeb..6447557c5 100644
--- a/compiler/injectdestructors.nim
+++ b/compiler/injectdestructors.nim
@@ -15,7 +15,7 @@
 
 import
   intsets, strtabs, ast, astalgo, msgs, renderer, magicsys, types, idents,
-  strutils, options, dfa, lowerings, tables, modulegraphs, msgs,
+  strutils, options, dfa, lowerings, tables, modulegraphs,
   lineinfos, parampatterns, sighashes, liftdestructors, optimizer,
   varpartitions
 
@@ -73,7 +73,7 @@ proc nestedScope(parent: var Scope): Scope =
 proc p(n: PNode; c: var Con; s: var Scope; mode: ProcessMode): PNode
 proc moveOrCopy(dest, ri: PNode; c: var Con; s: var Scope; isDecl = false): PNode
 
-import sets, hashes, tables
+import sets, hashes
 
 proc hash(n: PNode): Hash = hash(cast[pointer](n))
 
@@ -245,8 +245,6 @@ template isUnpackedTuple(n: PNode): bool =
   ## hence unpacked tuples themselves don't need to be destroyed
   (n.kind == nkSym and n.sym.kind == skTemp and n.sym.typ.kind == tyTuple)
 
-from strutils import parseInt
-
 proc checkForErrorPragma(c: Con; t: PType; ri: PNode; opname: string) =
   var m = "'" & opname & "' is not available for type <" & typeToString(t) & ">"
   if (opname == "=" or opname == "=copy") and ri != nil:
diff --git a/compiler/jsgen.nim b/compiler/jsgen.nim
index 3b183b9d6..393ac83c2 100644
--- a/compiler/jsgen.nim
+++ b/compiler/jsgen.nim
@@ -37,8 +37,6 @@ import
 
 import json, sets, math, tables, intsets, strutils
 
-from modulegraphs import ModuleGraph, PPassContext
-
 type
   TJSGen = object of PPassContext
     module: PSym
diff --git a/compiler/lineinfos.nim b/compiler/lineinfos.nim
index 11fa45480..131fe480b 100644
--- a/compiler/lineinfos.nim
+++ b/compiler/lineinfos.nim
@@ -67,12 +67,12 @@ type
     warnResultUsed = "ResultUsed",
     warnCannotOpen = "CannotOpen",
     warnFileChanged = "FileChanged",
-    warnDuplicateModuleImport = "DuplicateModuleImport",
     warnUser = "User",
     # hints
     hintSuccess = "Success", hintSuccessX = "SuccessX",
     hintCC = "CC",
-    hintLineTooLong = "LineTooLong", hintXDeclaredButNotUsed = "XDeclaredButNotUsed",
+    hintLineTooLong = "LineTooLong",
+    hintXDeclaredButNotUsed = "XDeclaredButNotUsed", hintDuplicateModuleImport = "DuplicateModuleImport",
     hintXCannotRaiseY = "XCannotRaiseY", hintConvToBaseNotNeeded = "ConvToBaseNotNeeded",
     hintConvFromXtoItselfNotNeeded = "ConvFromXtoItselfNotNeeded", hintExprAlwaysX = "ExprAlwaysX",
     hintQuitCalled = "QuitCalled", hintProcessing = "Processing", hintCodeBegin = "CodeBegin",
@@ -149,7 +149,6 @@ const
     warnResultUsed: "used 'result' variable",
     warnCannotOpen: "cannot open: $1",
     warnFileChanged: "file changed: $1",
-    warnDuplicateModuleImport: "$1",
     warnUser: "$1",
     hintSuccess: "operation successful: $#",
     # keep in sync with `testament.isSuccess`
@@ -157,6 +156,7 @@ const
     hintCC: "CC: $1",
     hintLineTooLong: "line too long",
     hintXDeclaredButNotUsed: "'$1' is declared but not used",
+    hintDuplicateModuleImport: "$1",
     hintXCannotRaiseY: "$1",
     hintConvToBaseNotNeeded: "conversion to base object is not needed",
     hintConvFromXtoItselfNotNeeded: "conversion from $1 to itself is pointless",
diff --git a/compiler/lookups.nim b/compiler/lookups.nim
index 0e057a79a..9b878dd4b 100644
--- a/compiler/lookups.nim
+++ b/compiler/lookups.nim
@@ -300,11 +300,15 @@ proc wrongRedefinition*(c: PContext; info: TLineInfo, s: string;
 proc addDeclAt*(c: PContext; scope: PScope, sym: PSym, info: TLineInfo) =
   let conflict = scope.addUniqueSym(sym, onConflictKeepOld = true)
   if conflict != nil:
-    var note = errGenerated
     if sym.kind == skModule and conflict.kind == skModule and sym.owner == conflict.owner:
-      # import foo; import foo
-      note = warnDuplicateModuleImport
-    wrongRedefinition(c, info, sym.name.s, conflict.info, note)
+      # e.g.: import foo; import foo
+      # xxx we could refine this by issuing a different hint for the case
+      # where a duplicate import happens inside an include.
+      localError(c.config, info, hintDuplicateModuleImport,
+        "duplicate import of '$1'; previous import here: $2" %
+        [sym.name.s, c.config $ conflict.info])
+    else:
+      wrongRedefinition(c, info, sym.name.s, conflict.info, errGenerated)
 
 proc addDeclAt*(c: PContext; scope: PScope, sym: PSym) {.inline.} =
   addDeclAt(c, scope, sym, sym.info)
diff --git a/compiler/semdata.nim b/compiler/semdata.nim
index f763a00e4..bd863505c 100644
--- a/compiler/semdata.nim
+++ b/compiler/semdata.nim
@@ -156,7 +156,8 @@ type
     features*: set[Feature]
     inTypeContext*, inConceptDecl*: int
     unusedImports*: seq[(PSym, TLineInfo)]
-    exportIndirections*: HashSet[(int, int)]
+    exportIndirections*: HashSet[(int, int)] # (module.id, symbol.id)
+    importModuleMap*: Table[int, int] # (module.id, module.id)
     lastTLineInfo*: TLineInfo
 
 template config*(c: PContext): ConfigRef = c.graph.config
diff --git a/compiler/suggest.nim b/compiler/suggest.nim
index a5b4ac87d..eaa30040a 100644
--- a/compiler/suggest.nim
+++ b/compiler/suggest.nim
@@ -32,7 +32,7 @@
 
 # included from sigmatch.nim
 
-import algorithm, sets, prefixmatches, lineinfos, parseutils, linter
+import algorithm, sets, prefixmatches, lineinfos, parseutils, linter, tables
 from wordrecg import wDeprecated, wError, wAddr, wYield
 
 when defined(nimsuggest):
@@ -572,7 +572,8 @@ proc markOwnerModuleAsUsed(c: PContext; s: PSym) =
     var i = 0
     while i <= high(c.unusedImports):
       let candidate = c.unusedImports[i][0]
-      if candidate == module or c.exportIndirections.contains((candidate.id, s.id)):
+      if candidate == module or c.importModuleMap.getOrDefault(candidate.id, int.low) == module.id or
+        c.exportIndirections.contains((candidate.id, s.id)):
         # mark it as used:
         c.unusedImports.del(i)
       else: