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

timotheecour · 2021-05-04T19:59:07Z

{.used.} now accepts symbols, e.g. {.used: mymodule.} or {.used: myFun.}. This allows using a clean approach instead of hacks/workarounds like discard mymodule.someSymbol that also don't always work
fix UnusedImport should not be triggered for --import:mymodule #13185 (UnusedImport should not be triggered for --import:mymodule)
module imports are now consistently creating a module alias for all forms of imports, eg import foo vs import foo as foo2 both call createModuleAliasImpl; this allows keeping module modifications (eg moduleSym.flags) isolated from the actual imported module
fix Incorrect UnusedImport warning in presence of import foo as bar #17511, see [1]
add new warning DuplicateModuleImport that triggers for eg on import foo; import foo
fix a bug where import foo as bar would not show deprecation msg for foo (before PR, only import foo would show the deprecation msg), eg with {.deprecated: "blah".}
fix Incorrect UnusedImport warnings regression from 1.4.2 to 1.5.1 #17510
fix Incorrect UnusedImport warning with re-exported symbols or with tyAlias (type Foo* = Bar) #14246
relies on the recent nimoutFull which is critical for this kind of test

note

[1] Incorrect UnusedImport warning in presence of import foo as bar #17511 has 2 components A and B, I'm fixing both:
A is the test.nim(1, 17) Warning: imported and not used: 'sequtils' [UnusedImport] (warning in the imported module referring to:

import sequtils as su2
export su2

B is the 2nd warning test2.nim(1, 11) Warning: imported and not used: 'test' [UnusedImport] which is a regression

this PR also introduces `genPNode`

it simplify writing PNode-based compiler code

see the full runnableExamples in the PR:

    let a = [1,2]
    let b = cache.getIdent("foo")
    let node: PNode = genPNode(c, a, b):
      for i in 0..<3:
        let b = @[i, 1]
        echo (a, b, i, "abc")

future work

(all pre-existing)

incorrect UnusedImport warning for var/let/const/type invoked inside a generic #17941 (generics + var/let/const/type) => fix #17941: UnusedImport works for var/let/const/type invoked inside a generic #17942
add tests for fix #17941: UnusedImport works for var/let/const/type invoked inside a generic #17942
use {.used: module.} to fix UnusedImport warning is displayed for every runnable example #11819 in a better, more targeted way
add switch("warningAsError", "UnusedImport") to nimStrictMode now that it works more reliably
see note D20210504T200053: XDeclaredButNotUsed notes should be sorted to avoid getting different results in 32 vs 64 bit

timotheecour · 2021-05-04T20:03:58Z

compiler/passes.nim

+      of nkImportStmt: code = "import $1 as $2\n{.used: $2.}" % [quoted, name]
+      of nkIncludeStmt: code = "include $1" % quoted
+      else: assert false
+      let node = parseString(code, graph.cache, graph.config, filename = "", line = 0, errorHandler = nil)


this pattern via parseString simplifies code generation a lot in the compiler, we should use this more in the future instead of manually constructing the AST via PNode apis, which is error prone, hard to read/write/edit.

(for the same reasons, quote/genAst simplifies macros, but here with raw PNode, it's even more pronounced)

What? That's terrible. Why not use an AST construction macro instead?

Why is this so terrible? the traditional way based on manually constructing the AST via newNodeI etc is arguably more complex to write and maintain; even with the above simple snippet ("import $1 as $2\n{.used: $2.}") it's hard to write the corresponding code manually.

Why not use an AST construction macro instead?

which one, or how would it work? if such macro doesn't exist yet i suggest to delay this to future work; this PR is already quite complex and solves many hard to fix long standing bugs (4 filed issues plus other issues I didn't bother writing bugs for).

Well this style is simply not acceptable. Since it's a bugfix, why not fix the bug instead of pushing for idioms that are dubious at best. (How do we know "".dup(addQuoted(module)) respects Nim's quoting rules?)

Why not use an AST construction macro instead?

done

timotheecour · 2021-05-05T05:54:15Z

compiler/importer.nim

  template createModuleAliasImpl(ident): untyped =
-    createModuleAlias(realModule, nextSymId c.idgen, ident, realModule.info, c.config.options)


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

compiler/importer.nim

Araq · 2021-05-05T16:53:46Z

compiler/ast.nim

@@ -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`


Does this need IC support?

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

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.

alaviss

{.used.} now accepts symbols, e.g. {.used: mymodule.} or {.used: myFun.}. This allows using a clean approach instead of hacks/workarounds like discard mymodule.someSymbol that also don't always work

Is this needed to fix the bugs you listed? If no then shouldn't it be in a separated PR while this one focuses on the bugs you fixed?

timotheecour · 2021-05-05T18:43:17Z

Is this needed to fix the bugs you listed? If no then shouldn't it be in a separated PR while this one focuses on the bugs you fixed?

yes, it's needed, at least for fixing #13185, and I don't think this extension to {.used.} is controversial.
(and it can also be used to properly fix #11819 in future work)

timotheecour · 2021-05-06T07:33:20Z

PTAL, CI failures unrelated:

Clyybber · 2021-05-06T21:46:31Z

yes, it's needed, at least for fixing #13185

I don't think so, it can be fixed without introducing a new feature.

IMO the PR needs to be split up into bugfix PR and feature addition PR+RFC.

…ases

…`genAst`

timotheecour · 2021-05-09T07:16:52Z

I don't think so, it can be fixed without introducing a new feature.
IMO the PR needs to be split up into bugfix PR and feature addition PR+RFC.

I prefer the way i did it in this PR but here you go: #17978

…eclaredButNotUsed, etc; fix #17511, #17510, #14246 (without realModule) (#18362) * {.used: symbol} * add tests * fix tests with --import * --import works without giving spurious unused warnings * new warning warnDuplicateModuleImport for `import foo; import foo` * fix test, add resolveModuleAlias, use proper line info for module aliases * fix spurious warnings * fix deprecation msg for deprecated modules even with `import foo as bar` * disable a test for i386 pending sorting XDeclaredButNotUsed errors * UnusedImport now works with re-exported symbols * fix typo [skip ci] * ic support * add genPNode to allow writing PNode-based compiler code similarly to `genAst` * fix DuplicateModuleImport warning * adjust test * fixup * fixup * fixup * fix after rebase * fix for IC * keep the proc inline, move the const out * [skip ci] fix changelog * experiment: remove calls to resolveModuleAlias * followup * fixup * fix tests/modules/tselfimport.nim * workaround tests/deprecated/tmodule1.nim * fix properly * simplify

…port, XDeclaredButNotUsed, etc; fix nim-lang#17511, nim-lang#17510, nim-lang#14246 (without realModule) (nim-lang#18362) * {.used: symbol} * add tests * fix tests with --import * --import works without giving spurious unused warnings * new warning warnDuplicateModuleImport for `import foo; import foo` * fix test, add resolveModuleAlias, use proper line info for module aliases * fix spurious warnings * fix deprecation msg for deprecated modules even with `import foo as bar` * disable a test for i386 pending sorting XDeclaredButNotUsed errors * UnusedImport now works with re-exported symbols * fix typo [skip ci] * ic support * add genPNode to allow writing PNode-based compiler code similarly to `genAst` * fix DuplicateModuleImport warning * adjust test * fixup * fixup * fixup * fix after rebase * fix for IC * keep the proc inline, move the const out * [skip ci] fix changelog * experiment: remove calls to resolveModuleAlias * followup * fixup * fix tests/modules/tselfimport.nim * workaround tests/deprecated/tmodule1.nim * fix properly * simplify

stale · 2022-05-31T02:23:11Z

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

timotheecour commented May 4, 2021

View reviewed changes

timotheecour force-pushed the pr_fix_used branch from f923c72 to c45eb67 Compare May 4, 2021 22:14

timotheecour changed the title ~~{.used: symbol}: fixes lots of issues with UnusedImport, XDeclaredButNotUsed, etc~~ {.used: symbol}: fixes lots of issues with UnusedImport, XDeclaredButNotUsed, etc; fix #13185, #17511, #17510, #14246 May 5, 2021

timotheecour marked this pull request as ready for review May 5, 2021 05:48

timotheecour commented May 5, 2021

View reviewed changes

timotheecour mentioned this pull request May 5, 2021

fix #17941: UnusedImport works for var/let/const/type invoked inside a generic #17942

Merged

Araq reviewed May 5, 2021

View reviewed changes

compiler/importer.nim Outdated Show resolved Hide resolved

Araq reviewed May 5, 2021

View reviewed changes

timotheecour force-pushed the pr_fix_used branch from cee07c6 to 0e94075 Compare May 5, 2021 17:56

alaviss reviewed May 5, 2021

View reviewed changes

This was referenced May 6, 2021

nimsuggest/tests/tmacro_highlight.nim fails after unrelated changes on i386 #17945

Open

tests/stdlib/tchannels.nim flaky, gives reTimeout #17946

Closed

timotheecour added 13 commits May 7, 2021 18:21

{.used: symbol}

985b610

add tests

ac3e2d4

fix tests with --import

2f42905

--import works without giving spurious unused warnings

98ec125

new warning warnDuplicateModuleImport for import foo; import foo

f4eee1d

fix test, add resolveModuleAlias, use proper line info for module ali…

46d51cc

…ases

fix spurious warnings

b235191

fix deprecation msg for deprecated modules even with import foo as bar

fcb2cca

disable a test for i386 pending sorting XDeclaredButNotUsed errors

6f116fb

UnusedImport now works with re-exported symbols

f6afeae

fix typo [skip ci]

414b3b2

ic support

9428b31

add genPNode to allow writing PNode-based compiler code similarly to …

621b2d9

…`genAst`

timotheecour force-pushed the pr_fix_used branch from d1a6e5a to 621b2d9 Compare May 8, 2021 01:21

fix DuplicateModuleImport warning

4808772

timotheecour mentioned this pull request May 9, 2021

light version of #17938: fix most issues with UnusedImport, XDeclaredButNotUsed, etc; fix #17511, #17510, #14246 #17978

Closed

timotheecour mentioned this pull request Jun 26, 2021

even lighter version of #17938: fix most issues with UnusedImport, XDeclaredButNotUsed, etc; fix #17511, #17510, #14246 (without realModule) #18362

Merged

2 tasks

timotheecour mentioned this pull request Aug 3, 2021

make var, let work in static, const sections #18637

Closed

stale bot added the stale Staled PR/issues; remove the label after fixing them label May 31, 2022

stale bot closed this Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

timotheecour commented May 4, 2021 •

edited

Loading

timotheecour May 4, 2021 •

edited

Loading

Araq May 4, 2021

timotheecour May 5, 2021 •

edited

Loading

Araq May 5, 2021

timotheecour May 6, 2021

timotheecour May 5, 2021

Araq May 5, 2021

timotheecour May 5, 2021 •

edited

Loading

Araq May 6, 2021

alaviss left a comment

timotheecour commented May 5, 2021 •

edited

Loading

timotheecour commented May 6, 2021

Clyybber commented May 6, 2021 •

edited

Loading

timotheecour commented May 9, 2021

stale bot commented May 31, 2022

		template createModuleAliasImpl(ident): untyped =
		createModuleAlias(realModule, nextSymId c.idgen, ident, realModule.info, c.config.options)

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

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

Conversation

timotheecour commented May 4, 2021 • edited Loading

note

this PR also introduces genPNode

future work

timotheecour May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Araq May 4, 2021

Choose a reason for hiding this comment

timotheecour May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Araq May 5, 2021

Choose a reason for hiding this comment

timotheecour May 6, 2021

Choose a reason for hiding this comment

timotheecour May 5, 2021

Choose a reason for hiding this comment

Araq May 5, 2021

Choose a reason for hiding this comment

timotheecour May 5, 2021 • edited Loading

Choose a reason for hiding this comment

Araq May 6, 2021

Choose a reason for hiding this comment

alaviss left a comment

Choose a reason for hiding this comment

timotheecour commented May 5, 2021 • edited Loading

timotheecour commented May 6, 2021

Clyybber commented May 6, 2021 • edited Loading

timotheecour commented May 9, 2021

stale bot commented May 31, 2022

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

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

timotheecour commented May 4, 2021 •

edited

Loading

this PR also introduces `genPNode`

timotheecour May 4, 2021 •

edited

Loading

timotheecour May 5, 2021 •

edited

Loading

timotheecour May 5, 2021 •

edited

Loading

timotheecour commented May 5, 2021 •

edited

Loading

Clyybber commented May 6, 2021 •

edited

Loading