Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

{.used: symbol}: fixes lots of issues with UnusedImport, XDeclaredButNotUsed, etc; fix #13185, #17511, #17510, #14246 #17938

Closed
wants to merge 14 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,9 @@

- `typeof(voidStmt)` now works and returns `void`.

- `{.used.}` now accepts symbols, e.g. `{.used: mymodule.}` or `{.used: myFun.}`.


## Compiler changes

- Added `--declaredlocs` to show symbol declaration location in messages.
Expand Down
9 changes: 9 additions & 0 deletions compiler/ast.nim
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,8 @@ type
TSym* {.acyclic.} = object of TIdObj # Keep in sync with PackedSym
# proc and type instantiations are cached in the generic symbol
case kind*: TSymKind
of skModule:
realModule*: PSym # for `createModuleAlias`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need IC support?

Copy link
Member Author

@timotheecour timotheecour May 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, but untestable beyond the existing ic tests (that still do pass) because of pre-existing issue with ic+export: #17943; new tests can be added in future work.

Also, manually serializing/deserializing the AST/symbols/various compiler structures as in done in packed_ast is IMO the wrong approach, it duplicates logic and adds significant complexity, and on top of that in my experience using ic makes compilation slower that a full recompilation. We should really explore alternatives:

  • using a generic serializer/deserializer that works with any types including loopy structures, plus ability to customize in the rare places where it needs to; it's not hard and much more maintaniable
  • the compiler server approach we discussed before, which IMO is the best (still need to share my code, what worked, and what didn't yet work); this definitely gives the best performance, almost as fast as nim secret despite using c backend

we should probably discuss this elsewhere though, separate topic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well since you bring it up:

using a generic serializer/deserializer that works with any types including loopy structures, plus ability to customize in the rare places where it needs to; it's not hard and much more maintaniable

Well if IC is already too slow, a more generic solution won't make it faster... Furthermore I regard the .rod file format as the future for Nim's tooling and it enforces us to be honest about phase ordering issues (no more unprincipled mutations possible). A compiler server accomplishes nothing comparable. We already have nimsuggest, it eats gigabytes of RAM and occasionally 100% CPU...

The "loopy structures" are harmful and cause many many bugs; don't write tooling to hide them, avoid them.

of routineKinds:
#procInstCache*: seq[PInstantiation]
gcUnsafetyReason*: PSym # for better error messages wrt gcsafe
Expand Down Expand Up @@ -1493,6 +1495,13 @@ proc createModuleAlias*(s: PSym, id: ItemId, newIdent: PIdent, info: TLineInfo;
result.position = s.position
result.loc = s.loc
result.annex = s.annex
result.realModule = s # xxx can we just use id ?

proc resolveModuleAlias*(s: PSym): PSym =
assert s.kind == skModule
result = s
if result.realModule != nil: # owner is unrelated
result = result.realModule

proc initStrTable*(x: var TStrTable) =
x.counter = 0
Expand Down
12 changes: 10 additions & 2 deletions compiler/ic/ic.nim
Original file line number Diff line number Diff line change
Expand Up @@ -394,11 +394,15 @@ proc storeSym*(s: PSym; c: var PackedEncoder; m: var PackedModule): PackedItemId
storeNode(p, s, ast)
storeNode(p, s, constraint)

if s.kind in {skLet, skVar, skField, skForVar}:
case s.kind
of {skLet, skVar, skField, skForVar}:
c.addMissing s.guard
p.guard = s.guard.safeItemId(c, m)
p.bitsize = s.bitsize
p.alignment = s.alignment
of skModule:
p.realModule = s.realModule.safeItemId(c, m)
else: discard

p.externalName = toLitId(if s.loc.r.isNil: "" else: $s.loc.r, m)
p.locFlags = s.loc.flags
Expand Down Expand Up @@ -847,10 +851,14 @@ proc symBodyFromPacked(c: var PackedDecoder; g: var PackedModuleGraph;
when hasFFI:
result.cname = g[si].fromDisk.strings[s.cname]

if s.kind in {skLet, skVar, skField, skForVar}:
case s.kind
of {skLet, skVar, skField, skForVar}:
result.guard = loadSym(c, g, si, s.guard)
result.bitsize = s.bitsize
result.alignment = s.alignment
of skModule:
result.realModule = loadSym(c, g, si, s.realModule)
else: discard
result.owner = loadSym(c, g, si, s.owner)
let externalName = g[si].fromDisk.strings[s.externalName]
if externalName != "":
Expand Down
1 change: 1 addition & 0 deletions compiler/ic/packed_ast.nim
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ type
when hasFFI:
cname*: LitId
constraint*: NodeId
realModule*: PackedItemId

PackedType* = object
kind*: TTypeKind
Expand Down
31 changes: 18 additions & 13 deletions compiler/importer.nim
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import
intsets, ast, astalgo, msgs, options, idents, lookups,
semdata, modulepaths, sigmatch, lineinfos, sets,
modulegraphs, wordrecg
from strutils import `%`

proc readExceptSet*(c: PContext, n: PNode): IntSet =
assert n.kind in {nkImportExceptStmt, nkExportExceptStmt}
Expand Down Expand Up @@ -224,19 +225,21 @@ proc importForwarded(c: PContext, n: PNode, exceptSet: IntSet; fromMod: PSym; im

proc importModuleAs(c: PContext; n: PNode, realModule: PSym, importHidden: bool): PSym =
result = realModule
c.unusedImports.add((realModule, n.info))
template createModuleAliasImpl(ident): untyped =
createModuleAlias(realModule, nextSymId c.idgen, ident, realModule.info, c.config.options)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

realModule.info would lead to wrong line info in error msgs

createModuleAlias(realModule, nextSymId c.idgen, ident, n.info, c.config.options)
if n.kind != nkImportAs: discard
elif n.len != 2 or n[1].kind != nkIdent:
localError(c.config, n.info, "module alias must be an identifier")
elif n[1].ident.id != realModule.name.id:
# some misguided guy will write 'import abc.foo as foo' ...
result = createModuleAliasImpl(n[1].ident)
if result == realModule:
# avoids modifying `realModule`, see D20201209T194412 for `import {.all.}`
# and also for `import foo; {.used: foo.}`
result = createModuleAliasImpl(realModule.name)
if importHidden:
if result == realModule: # avoids modifying `realModule`, see D20201209T194412.
result = createModuleAliasImpl(realModule.name)
result.options.incl optImportHidden
c.unusedImports.add((result, n.info))

proc transformImportAs(c: PContext; n: PNode): tuple[node: PNode, importHidden: bool] =
var ret: typeof(result)
Expand Down Expand Up @@ -281,16 +284,15 @@ proc myImportModule(c: PContext, n: var PNode, importStmtResult: PNode): PSym =
#echo "set back to ", L
c.graph.importStack.setLen(L)
# we cannot perform this check reliably because of
# test: modules/import_in_config)
when true:
if result.info.fileIndex == c.module.info.fileIndex and
result.info.fileIndex == n.info.fileIndex:
localError(c.config, n.info, "A module cannot import itself")
if sfDeprecated in result.flags:
if result.constraint != nil:
message(c.config, n.info, warnDeprecated, result.constraint.strVal & "; " & result.name.s & " is deprecated")
# test: modules/import_in_config) # xxx is that still true?
let realModule = result.resolveModuleAlias
if realModule == c.module:
localError(c.config, n.info, "module '$1' cannot import itself" % c.module.name.s)
if sfDeprecated in realModule.flags:
if realModule.constraint != nil:
message(c.config, n.info, warnDeprecated, realModule.constraint.strVal & "; " & realModule.name.s & " is deprecated")
else:
message(c.config, n.info, warnDeprecated, result.name.s & " is deprecated")
message(c.config, n.info, warnDeprecated, realModule.name.s & " is deprecated")
suggestSym(c.graph, n.info, result, c.graph.usageSym, false)
importStmtResult.add newSymNode(result, n.info)
#newStrNode(toFullPath(c.config, f), n.info)
Expand All @@ -303,6 +305,9 @@ 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.resolveModuleAlias:
c.exportIndirections.incl((m.id, s.id))

proc evalImport*(c: PContext, n: PNode): PNode =
result = newNodeI(nkImportStmt, n.info)
Expand Down
2 changes: 2 additions & 0 deletions compiler/lineinfos.nim
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type
warnStrictNotNil = "StrictNotNil",
warnCannotOpen = "CannotOpen",
warnFileChanged = "FileChanged",
warnDuplicateModuleImport = "DuplicateModuleImport",
warnUser = "User",

hintSuccess = "Success", hintSuccessX = "SuccessX", hintBuildMode = "BuildMode",
Expand Down Expand Up @@ -140,6 +141,7 @@ const
warnStrictNotNil: "$1",
warnCannotOpen: "cannot open: $1",
warnFileChanged: "file changed: $1",
warnDuplicateModuleImport: "$1",
warnUser: "$1",
hintSuccess: "operation successful: $#",
# keep in sync with `testament.isSuccess`
Expand Down
22 changes: 14 additions & 8 deletions compiler/lookups.nim
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ proc considerQuotedIdent*(c: PContext; n: PNode, origin: PNode = nil): PIdent =
template addSym*(scope: PScope, s: PSym) =
strTableAdd(scope.symbols, s)

proc addUniqueSym*(scope: PScope, s: PSym): PSym =
result = strTableInclReportConflict(scope.symbols, s)
proc addUniqueSym*(scope: PScope, s: PSym, onConflictKeepOld: bool): PSym =
result = strTableInclReportConflict(scope.symbols, s, onConflictKeepOld)

proc openScope*(c: PContext): PScope {.discardable.} =
result = PScope(parent: c.currentScope,
Expand Down Expand Up @@ -282,32 +282,38 @@ proc ensureNoMissingOrUnusedSymbols(c: PContext; scope: PScope) =
# maybe they can be made skGenericParam as well.
if s.typ != nil and tfImplicitTypeParam notin s.typ.flags and
s.typ.kind != tyGenericParam:
# xxx D20210504T200053:here these should be sorted to have reproducible errors, in particular
# across 32 vs 64 bit; can be done by buffering those and then sorting.
message(c.config, s.info, hintXDeclaredButNotUsed, s.name.s)
s = nextIter(it, scope.symbols)

proc wrongRedefinition*(c: PContext; info: TLineInfo, s: string;
conflictsWith: TLineInfo) =
conflictsWith: TLineInfo, note = errGenerated) =
## Emit a redefinition error if in non-interactive mode
if c.config.cmd != cmdInteractive:
localError(c.config, info,
localError(c.config, info, note,
"redefinition of '$1'; previous declaration here: $2" %
[s, c.config $ conflictsWith])

proc addDecl*(c: PContext, sym: PSym, info: TLineInfo) =
let conflict = c.currentScope.addUniqueSym(sym)
let conflict = c.currentScope.addUniqueSym(sym, onConflictKeepOld = true)
if conflict != nil:
wrongRedefinition(c, info, sym.name.s, conflict.info)
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)

proc addDecl*(c: PContext, sym: PSym) =
let conflict = strTableInclReportConflict(c.currentScope.symbols, sym, true)
if conflict != nil:
wrongRedefinition(c, sym.info, sym.name.s, conflict.info)

proc addPrelimDecl*(c: PContext, sym: PSym) =
discard c.currentScope.addUniqueSym(sym)
discard c.currentScope.addUniqueSym(sym, onConflictKeepOld = false)

proc addDeclAt*(c: PContext; scope: PScope, sym: PSym) =
let conflict = scope.addUniqueSym(sym)
let conflict = scope.addUniqueSym(sym, onConflictKeepOld = true)
if conflict != nil:
wrongRedefinition(c, sym.info, sym.name.s, conflict.info)

Expand Down
2 changes: 1 addition & 1 deletion compiler/passaux.nim
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
## implements some little helper passes

import
ast, passes, idents, msgs, options, lineinfos
ast, passes, msgs, options, lineinfos

from modulegraphs import ModuleGraph, PPassContext

Expand Down
21 changes: 15 additions & 6 deletions compiler/passes.nim
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import
options, ast, llstream, msgs,
idents,
syntaxes, modulegraphs, reorder,
lineinfos, pathutils
lineinfos, pathutils, vmconv
from os import splitFile

type
TPassData* = tuple[input: PNode, closeOutput: PNode]
Expand Down Expand Up @@ -88,11 +89,19 @@ proc processImplicits(graph: ModuleGraph; implicits: seq[string], nodeKind: TNod
for module in items(implicits):
# implicit imports should not lead to a module importing itself
if m.position != resolveMod(graph.config, module, relativeTo).int32:
var importStmt = newNodeI(nodeKind, m.info)
var str = newStrNode(nkStrLit, module)
str.info = m.info
importStmt.add str
if not processTopLevelStmt(graph, importStmt, a): break
var c = GenContext(cache: graph.cache, info: m.info)
var node: PNode
case nodeKind
of nkImportStmt:
let name = graph.cache.getIdent(module.splitFile.name)
node = genPNode(c, module, name):
import module as name
{.used: name.}
of nkIncludeStmt:
node = genPNode(c, module):
include module
else: assert false
if not processTopLevelStmt(graph, node, a): break

const
imperativeCode = {low(TNodeKind)..high(TNodeKind)} - {nkTemplateDef, nkProcDef, nkMethodDef,
Expand Down
12 changes: 9 additions & 3 deletions compiler/pragmas.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1212,9 +1212,15 @@ proc singlePragma(c: PContext, sym: PSym, n: PNode, i: var int,
of wBoolDefine:
sym.magic = mBoolDefine
of wUsed:
noVal(c, it)
if sym == nil: invalidPragma(c, it)
else: sym.flags.incl sfUsed
case it.kind
of nkExprColonExpr: # {.used: mysym.}
let sym2 = qualifiedLookUp(c, it[1], {checkUndeclared, checkModule})
assert sym2 != nil # PRTEMP: localError
sym2.flags.incl sfUsed
of nkIdent: # {.used.}
if sym == nil: invalidPragma(c, it)
else: sym.flags.incl sfUsed
else: invalidPragma(c, it)
of wLiftLocals: discard
of wRequires, wInvariant, wAssume, wAssert:
pragmaProposition(c, it)
Expand Down
3 changes: 2 additions & 1 deletion compiler/suggest.nim
Original file line number Diff line number Diff line change
Expand Up @@ -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)):
let candidate2 = candidate.resolveModuleAlias
if candidate2 == module or c.exportIndirections.contains((candidate.id, s.id)):
# mark it as used:
c.unusedImports.del(i)
else:
Expand Down
Loading