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

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

Closed

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 9, 2021

refs #17938 (comment), this PR is a light weight version of #17938, removing these:

ie, it does the following:

note

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

@timotheecour timotheecour marked this pull request as ready for review May 9, 2021 07:13
@timotheecour timotheecour changed the title light version of https://github.com/nim-lang/Nim/pull/17938 light version of https://github.com/nim-lang/Nim/pull/17938: fix most issues with UnusedImport, XDeclaredButNotUsed, etc; fix #17511, #17510, #14246 May 9, 2021
@timotheecour timotheecour changed the title light version of https://github.com/nim-lang/Nim/pull/17938: fix most issues with UnusedImport, XDeclaredButNotUsed, etc; fix #17511, #17510, #14246 light version of #17938: fix most issues with UnusedImport, XDeclaredButNotUsed, etc; fix #17511, #17510, #14246 May 9, 2021
@timotheecour timotheecour requested a review from Araq May 9, 2021 07:19
compiler/ast.nim Show resolved Hide resolved
@Varriount
Copy link
Contributor

Sooner or later, I think it might be prudent to have a module type (even if only as an input to RTTI procedures).

@Araq
Copy link
Member

Araq commented May 9, 2021

Here is how it should be done instead: For import m the m PSym is copied and this copy is inserted into the importing module's symbol table. Since it's always a copy, it can also have a different name so import m as m2 has a most natural representation.

@Araq
Copy link
Member

Araq commented May 9, 2021

Sooner or later, I think it might be prudent to have a module type (even if only as an input to RTTI procedures).

We already have an ImportedModule type in the compiler...

@timotheecour timotheecour force-pushed the pr_fix_used_conservative branch from 4add9ac to 570826a Compare June 25, 2021 18:17
@timotheecour
Copy link
Member Author

timotheecour commented Jun 25, 2021

PTAL, rebased after bitrot conflicts

Here is how it should be done instead: For import m the m PSym is copied and this copy is inserted into the importing module's symbol table. Since it's always a copy, it can also have a different name so import m as m2 has a most natural representation.

I'm already doing that, createModuleAlias is always called for imports and creates a PSym copy

EDIT:
it was green before rebase but after rebase, CI now fails for IC with following error:

Hint:  [Link]
duplicate symbol '_g_systemZschubfach_81' in:
    /Users/timothee/git_clone/nim/Nim_prs/nimcache/tests/ic/timports.nim_4a8a08f09d37b73795649038408b5f33/stdlib_system.nim.c.o
    /Users/timothee/git_clone/nim/Nim_prs/nimcache/tests/ic/timports.nim_4a8a08f09d37b73795649038408b5f33/stdlib_schubfach.nim.c.o
ld: 1 duplicate symbol for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

(the other errors are the same)

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Jun 25, 2021
@timotheecour timotheecour marked this pull request as draft June 25, 2021 19:38
@timotheecour timotheecour force-pushed the pr_fix_used_conservative branch from 570826a to 7e4a96d Compare June 25, 2021 21:57
@timotheecour timotheecour marked this pull request as ready for review June 25, 2021 22:50
@timotheecour
Copy link
Member Author

PTAL: it turns out the duplicate symbol link error is caused by a pre-existing bug in IC (just filed #18361 which explains it) that wasn't exhibited until this PR because until this PR, createModuleAlias wasn't being called for import foo (only for import foo as bar)

@timotheecour
Copy link
Member Author

trying another PR without realModule here: #18362

@timotheecour timotheecour marked this pull request as draft June 26, 2021 02:19
@timotheecour timotheecour removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jun 26, 2021
@Araq Araq closed this in #18362 Jun 26, 2021
@timotheecour timotheecour deleted the pr_fix_used_conservative branch June 26, 2021 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants