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

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

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):

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

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

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)

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:

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

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",

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)

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

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:

tests/misc/trunner.nim

@@ -31,7 +31,7 @@ const
 proc runNimCmd(file, options = "", rtarg = ""): auto =
   let fileabs = testsDir / file.unixToNativePath
   # doAssert fileabs.fileExists, fileabs # disabled because this allows passing `nim r --eval:code fakefile`
-  let cmd = fmt"{nim} {mode} {options} --hints:off {fileabs} {rtarg}"
+  let cmd = fmt"{nim} {mode} --hint:all:off {options} {fileabs} {rtarg}"
   result = execCmdEx(cmd)
   when false: # for debugging
     echo cmd
@@ -352,5 +352,29 @@ running: v2
     doAssert outp2 == "12345\n", outp2
     doAssert status2 == 0
 
+  block: # UnusedImport
+    proc fn(opt: string, expected: string) =
+      let output = runNimCmdChk("pragmas/mused3.nim", fmt"--warning:all:off --warning:UnusedImport --hint:DuplicateModuleImport {opt}")
+      doAssert output == expected, opt & "\noutput:\n" & output & "expected:\n" & expected
+    fn("-d:case1"): """
+mused3.nim(13, 8) Warning: imported and not used: 'mused3b' [UnusedImport]
+"""
+    fn("-d:case2"): ""
+    fn("-d:case3"): ""
+    fn("-d:case4"): ""
+    fn("-d:case5"): ""
+    fn("-d:case6"): ""
+    fn("-d:case7"): ""
+    fn("-d:case8"): ""
+    fn("-d:case9"): ""
+    fn("-d:case10"): ""
+    when false:
+      fn("-d:case11"): """
+  Warning: imported and not used: 'm2' [UnusedImport]
+  """
+    fn("-d:case12"): """
+mused3.nim(75, 10) Hint: duplicate import of 'mused3a'; previous import here: mused3.nim(74, 10) [DuplicateModuleImport]
+"""
+
 else:
   discard # only during debugging, tests added here will run with `-d:nimTestsTrunnerDebugging` enabled

tests/pragmas/mused3.nim

@@ -0,0 +1,76 @@
+#[
+ran from trunner
+]#
+
+
+
+
+
+
+# line 10
+when defined case1:
+  from mused3a import nil
+  from mused3b import nil
+  mused3a.fn1()
+
+when defined case2:
+  from mused3a as m1 import nil
+  m1.fn1()
+
+when defined case3:
+  from mused3a import fn1
+  fn1()
+
+when defined case4:
+  from mused3a as m1 import fn1
+  m1.fn1()
+
+when defined case5:
+  import mused3a as m1
+  fn1()
+
+when defined case6:
+  import mused3a except nonexistent
+  fn1()
+
+when defined case7:
+  import mused3a
+  mused3a.fn1()
+
+when defined case8:
+  # re-export test
+  import mused3a except nonexistent
+  gn1()
+
+when defined case9:
+  # re-export test
+  import mused3a
+  gn1()
+
+when defined case10:
+  #[
+  edge case which happens a lot in compiler code:
+  don't report UnusedImport for mused3b here even though it works without `import mused3b`,
+  because `a.b0.f0` is accessible from both mused3a and mused3b (fields are given implicit access)
+  ]#
+  import mused3a
+  import mused3b
+  var a: Bar
+  discard a.b0.f0
+
+when false:
+  when defined case11:
+    #[
+    xxx minor bug: this should give:
+    Warning: imported and not used: 'm2' [UnusedImport]
+    but doesn't, because currently implementation in `markOwnerModuleAsUsed`
+    only looks at `fn1`, not fully qualified call `m1.fn1()
+    ]#
+    from mused3a as m1 import nil
+    from mused3a as m2 import nil
+    m1.fn1()
+
+when defined case12:
+  import mused3a
+  import mused3a
+  fn1()

tests/pragmas/mused3a.nim

@@ -0,0 +1,41 @@
+when defined case1:
+  proc fn1*() = discard
+
+when defined case2:
+  proc fn1*() = discard
+
+when defined case3:
+  proc fn1*() = discard
+  proc fn2*() = discard
+
+when defined case4:
+  proc fn1*() = discard
+  proc fn2*() = discard
+
+when defined case5:
+  proc fn1*() = discard
+
+when defined case6:
+  proc fn1*() = discard
+
+when defined case7:
+  proc fn1*() = discard
+
+when defined case8:
+  import mused3b
+  export mused3b
+
+when defined case9:
+  import mused3b
+  export mused3b
+
+when defined case10:
+  import mused3b
+  type Bar* = object
+    b0*: Foo
+
+when defined case11:
+  proc fn1*() = discard
+
+when defined case12:
+  proc fn1*() = discard

tests/pragmas/mused3b.nim

@@ -0,0 +1,12 @@
+when defined case1:
+  proc gn1*()=discard
+
+when defined case8:
+  proc gn1*()=discard
+
+when defined case9:
+  proc gn1*()=discard
+
+when defined case10:
+  type Foo* = object
+    f0*: int