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

import foo {.all.}: import private symbols #17706

Merged
merged 57 commits into from
Apr 16, 2021

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 13, 2021

supersedes #11865 which was ready for ~2 years but which I had to re-implement from scratch because #16550 changed too many things.

This represents a lot of effort, I hope this PR can be merged soon to avoid rebasing/bitrot/re-implementing many times.

rfc

details

  • proc privateAccess*(t: typedesc) enables access to private fields of t in current scope
  • import foo {.all.} enables access to private symbols of foo, plus all existing import syntaxes are supported. Private fields of symbols from foo are not exposed; privateAccess can be used for that

this gives maximum flexiblity, allowing by-passing of symbol/field visibility in a given scope without affecting other clients of the imported module / type, and without any of the gotchas that come with include.

note to reviewer

  • there's a lot of files in this PR, but they're mostly tests

future work

@timotheecour timotheecour changed the title import foo {.all.} reboot import foo {.all.} reboot Apr 13, 2021
@Araq
Copy link
Member

Araq commented Apr 13, 2021

This represents a lot of effort, I hope this PR can be merged soon to avoid rebasing/bitrot/re-implementing many times.

I'm behind this language extension and will help out with the implementation, if required.

timotheecour added a commit to timotheecour/Nim that referenced this pull request Apr 13, 2021
timotheecour added a commit to timotheecour/Nim that referenced this pull request Apr 13, 2021
@timotheecour timotheecour marked this pull request as ready for review April 13, 2021 05:51
@timotheecour timotheecour force-pushed the pr_privateImportv2 branch 2 times, most recently from bbbfc12 to 8519225 Compare April 13, 2021 09:28
changelog.md Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Apr 13, 2021

This lacks IC support. ;-)

@timotheecour timotheecour force-pushed the pr_privateImportv2 branch 4 times, most recently from 16429d1 to 2910048 Compare April 14, 2021 20:03
compiler/modulegraphs.nim Outdated Show resolved Hide resolved
@@ -343,31 +364,44 @@ proc hash*(u: SigHash): Hash =

proc hash*(x: FileIndex): Hash {.borrow.}

template getC(): untyped =
when c is PContext: c
else: c.c
Copy link
Member

Choose a reason for hiding this comment

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

Too ugly... Can we find a better solution?

Copy link
Member Author

@timotheecour timotheecour Apr 14, 2021

Choose a reason for hiding this comment

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

this is pre-existing and I've strictly improved things in this PR via getC by avoiding this:

    when compiles(c.c.graph):
      if c.c.graph.onUsage != nil: c.c.graph.onUsage(c.c.graph, s, info)
    else:
      if c.graph.onUsage != nil: c.graph.onUsage(c.graph, s, info)

which had compiles and was not DRY

But I've added a future work item to further improve this.

Copy link
Member

Choose a reason for hiding this comment

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

It was only code for "nimfind" which is dead code now that we got "nim check --defusages"

Copy link
Member Author

@timotheecour timotheecour Apr 16, 2021

Choose a reason for hiding this comment

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

@Araq i see you've removed nimfind in #17737 (good or bad, I'm not sure), but please don't remove onDef + friends, they're very useful during debugging (and i wrote a competitor tool of nimfind that added more features, which relied on those onDef, onUsage etc). It's very useful to have 1 place defined that catches all definition/usage events.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to distinguish between nkSym and nkSymDef instead but it would break the macro system until we have an AST to AST translation step. :-/

compiler/modulegraphs.nim Outdated Show resolved Hide resolved
compiler/modulegraphs.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented Apr 14, 2021

Also ... what if every top level symbol is in the "exported" scope and then there is another filtering step? This could also be used to improve the error message to "symbol module.X exists but is private". I think this would simplify the logic quite a bit.

@timotheecour timotheecour added the TODO: followup needed remove tag once fixed or tracked elsewhere label Apr 14, 2021
@timotheecour
Copy link
Member Author

timotheecour commented Apr 15, 2021

PTAL

(btw, in code comments (like #17706 (comment)) instead of at PR level (like #17706 (comment)) are easier to tackle, so that threading is maintained)

Also ... what if every top level symbol is in the "exported" scope and then there is another filtering step? [...] I think this would simplify the logic quite a bit.

I thought about this but let's please defer this to future work after evaluating that it doesn't hurt CT performance (because the common case is import foo, not import foo {.all.}, and symbol lookup in a small table should be faster than in a larger table).

This could also be used to improve the error message to "symbol module.X exists but is private".

ya, that's another benefit i had listed in #11865; but note that this can be done even with the 2-tables approach

i've added those 2 items in future work section

compiler/ic/ic.nim Outdated Show resolved Hide resolved
compiler/ic/ic.nim Outdated Show resolved Hide resolved
compiler/semdata.nim Outdated Show resolved Hide resolved
changelog.md Outdated Show resolved Hide resolved
@timotheecour
Copy link
Member Author

PTAL

@Araq
Copy link
Member

Araq commented Apr 16, 2021

Merging this for now but the feature itself might not be the best idea. It must be documented as "experimental".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: followup needed remove tag once fixed or tracked elsewhere
Projects
None yet
3 participants