[superseded] import foo {.all.}

[timotheecour]

EDIT: formerly was {.privateImport.}, I renamed it to {.all.}

proposal to solve Does Nim need package-level visibility?
(I had originally proposed this here: import foo {.private.} to allows access to private fields )

Will add unittests later (I worked on a test suite)

EDIT: I've added extensive unittests (13 files to try all possible situations including declared/compiles/import as/from/field access etc ; actually these were generated from a single file in HAR format [1] that is expanded automatically). Note: this results in a single compilation (tmain.nim) so this is fast to compile/run.

it does work, feel free to try it out

# these put private (top-level) symbols in scope in the way you'd expect
{.push experimental: "allowPrivateImport".} # or just --experimental:allowPrivateImport
import system {.privateImport.}
import std/macros {.privateImport.} as macros2
from foo {.privateImport.} import bar
from foo {.privateImport.} as foo2 import bar
import foo {.privateImport.} except bar
{.pop.}

use cases

• would fix Enable cross-module initialization of private fields RFCs#145

• when a 3rd party library didn't expose a symbol that you need, you can use this (I keep running into this kind of issue)

this obviously can break if the 3rd party library then changes its implementation (eg renames/removes a private field), but you need to know what you're doing when using this feature. Note that include would have the same problem, but worse: since the include is more invasive than something like: from foo {.privateImport.} import bar which is a much more targeted violation of visibility)

The alternative is to patch the 3rd party library (eg: nim compiler code itself, or any other nimble package) by making some private symbols public, however this is worse (eg the private to public change can have un-intended side effects and would affect all clients, whereas with privateImport the field is only accessed in a targeted, easy to audit, place;

Furthermore waiting for 3rd party library to accept the change can take a while (or never happen), and maintaining a separate fork of the 3rd party library will cause even more issues (especially if project is to be shared).

• when organizing your code, (eg splitting out a types module to overcome lack of cyclic imports), it's a very common problem that the splitted module has to export more things than would be ideal, so that these symbols can be accessed. this PR solves this problem, allowing to make more symbols private, and override visibility with privateImport

• misc use cases, eg for a testing framework, to test private functions (that shouldn't be part of public API)

• helps with serialization (private fields) and reflection

• for debugging: i use this a lot as well: it allows writing a tool to call an arbitrary (eg private) proc with specified runtime args, without having to modify source code; using include for that has similar issues as mentioned below

• reduce need for include

• improve error messages now that we have more context, eg: undeclared bar => undeclared bar: foo.bar is private

• unbloat system.nim
  eg: [superseded] fix #12996 #12997 (comment)
  from system {.privateImport.} import since

besides removing most needs for include, making testing easier and other benefits I mentioned in top post of that PR, this would actually help un-bloat system.nim in similar ways.

why not use include?

while include has its use cases, the privateImport feature from this PR is in general a much better alternative that doesn't break modularity:

• include duplicates code, eg this won't work if the module has some state such as global or threadlocal variables
• this won't work if there are exportc procs, giving duplicate symbol errors
• include is all or nothing, you can't selectively access a symbol, and will lead to more symbol clashes
• it suffers from same perrenial problems as C #include
• it has other caveats, eg see behavior with nim doc, currentSourcePath, nimrtl which exports unmangled proc names, so also runs into above issues
• EDIT: weird pitfalls, eg: nim doc lib/std/sha1.nim works but nim doc lib/deprecated/pure/securehash.nim fails with: Error: redefinition of 'secureHash' (securehash.nim includes sha1)
• also explained in other places eg https://forum.nim-lang.org/t/4296#26730

import pragma

• using a pragma, as done in this PR, is preferable to using a new keyword (eg importPrivate foo), as it makes better code reuse and provides orthogonal functionality; there are other use cases for import pragmas, eg this PR Experimental typeImports #8660 (Experimental typeImports) could've used a pragma instead, eg: from colors import Color {.typeImports.}; and this too: import std/json {.undef:js.} as suggested here nim js prevents valid compile time functionality #11988

[1] for details on HAR see https://github.com/marler8997/har (a human readable multifile archive similar to org jointly proposed with @marler8997, which I've since reimplemented in nim); the file that generated these tests looks like this: https://github.com/timotheecour/vitanim/blob/master/testcases/har/test1.har

note

keeps coming up, eg:

• Deprecated operators for DateTime private members - Nim forum
• follow #15357 and move decodeQuery #15860 (comment)

[mratsim]

This is excellent, I've always wanted a way to keep symbols private but was restricted due to the common datatypes.nim with every type.

Why the push/pop pattern while we could just {.experimental: "allowPrivateImport".}? After all, all modules where we want private accesses are already tagged explicitly.

[krux02]

@zah Since this PR aims to fix your problem, I would like to have your opinion on it.

[timotheecour]

@mratsim

Why the push/pop pattern while we could just {.experimental: "allowPrivateImport".}? After all, all modules where we want private accesses are already tagged explicitly.

{.experimental: "allowPrivateImport".} also works, and so does --experimental:allowPrivateImport (eg in your nim cfg); this was just to illustrate that push/pop works with this feature if you need it in a specific spot only for ease of auditing (this isn't always the case, eg compiletimeFFI can't be push/pop'd and has to be enabled via global --experimental:compiletimeFFI).

[awr1]

We should make an attempt to make include less necessary, so I'm for this. I'm bikeshedding here but I think another keyword (or changing the pragma name, import XYZ {.privateImport.} comes off as a little redundant.)

Would the grammar need to be modified to reflect this change? I don't recall pragmas on imports anywhere else in the language...

[timotheecour]

I'm bikeshedding here but I think another keyword (or changing the pragma name, import XYZ {.privateImport.} comes off as a little redundant.)

import foo {.private.} is also possible, but privateImport is easier to search. Suggestions welcome.

Would the grammar need to be modified to reflect this change? I don't recall pragmas on imports anywhere else in the language...

I didn't have to change the parser as you can see from the PR, so I don't think grammar (nor auto generated doc/grammar.txt ) needs to be updated

[krux02]

I can't say I like this feature. But the use case for unit testing private functions is in my opinion a valid use case example. But I think maybe it is a better solution to have a special solution that just fixes unit testing without being so disruptive to the language.

[mratsim]

The only alternative is to do unit-testing within the source file.

Beyond unit-testing it also address why people would want recursive imports: because with a common datatypes.nim file with all types everything is public.

[GULPF]

Note that include would have the same problem, but worse: since the include is more invasive than something like: from foo {.privateImport.} import bar which is a much more targeted violation of visibility)

You'll still be able to access anything from foo with foo.someSymbol, so it's not really a target violation of visibility. You'll also be able to access private fields without any syntax indicating that you're violating visibility (this can also break the use of setters and getters from the foo module, since getters and setters can use the same name as a field).

[krux02]

@GULPF thanks for pointing out the dangers of features like this.

[awr1]

import foo {.private.} is also possible, but privateImport is easier to search. Suggestions welcome.

{.exposeAll.}, {.unhide.}, {.reveal.}

[awr1]

Beyond unit-testing it also address why people would want recursive imports: because with a common datatypes.nim file with all types everything is public.

I thought about this and I'm not certain that it does solve the desire for recursive imports completely: don't you still need to have a module specifically for code shared between multiple imports (essentially a reduced datatypes.nim?)

[timotheecour]

@GULPF

Note that include would have the same problem, but worse: since the include is more invasive than something like: from foo {.privateImport.} import bar which is a much more targeted violation of visibility)

You'll still be able to access anything from foo with foo.someSymbol, so it's not really a target violation of visibility.

I disagree, this PR indeeed offers a more targetted violation of visibility. With include foo, everything goes in scope, and you have exactly 0 control over that: private top level symbol bar from foo becomes accessible; theres' no such thing as include foo as foo2.

With this PR, user can do:
from foo {.privateImport.} as foo2 import nil to ensure that only accesses via foo2 (eg foo2.bar) will access private symbols; for fields, see below. This can even be used in conjunction with a regular import foo.

You'll also be able to access private fields without any syntax indicating that you're violating visibility (this can also break the use of setters and getters from the foo module, since getters and setters can use the same name as a field).

include is the same in this regard, it also bypasses getters/setters; once again, no downside compared to include.

And allowing raw field access is the point: this lets user access raw fields (eg for serialization / debugging / etc) as escape hatch to bypass restrictions from public API (which may not have thought about your use case when it was designed). The getters/setters can still be used explictly, via field(obj) instead of obj.field, ditto for setters.

without any syntax indicating that you're violating visibility

{.privateImport.} is the syntax indicating visibilty violation. That being said, my original implementation offered a finer granularity, allowing user to specify for which fields/objects he wishes to bypass field visibility (hence in particular controlling whether getters/setters would be bypassed). I simplified it into just 1 pragma for this PR, but [EDIT] future PR's can add an option to control private field visibility, as a backward compatible addition.

@krux02

But I think maybe it is a better solution to have a special solution that just fixes unit testing

A special solution wouldn't help with the other use cases mentioned besides unit testing.

without being so disruptive to the language

I'm not seeing how that is disruptive, given that a user can already bypass visibility restrictions with include. The main point is that {.privateImport.} has pretty much no downside compared to include and lots of upsides. nim, as a systems programming language, shouldn't restrict valid use cases for the sake of preventing a user shoot itself in the foot; you're in fact more likely to shoot yourself in the foot with include.

@awr1

I thought about this and I'm not certain that it does solve the desire for recursive imports completely

cyclic import is a separate topic; this PR decreases but doesn't eliminate its need.

[zah]

@krux02, my opinion is explained in significant detail in the linked thread:

https://forum.nim-lang.org/t/4293#26876
https://forum.nim-lang.org/t/4293#26692

This opinion is considered controversial, but I think the best way to solve the package-level visibility issues is to work on supporting cyclic imports. Private imports or other more precise ways to control the visibility in packages tend to work more as a band-aid for the original root problem.

[pb-cdunn]

Compilation time keeps getting worse. I hope that whatever solution we adopt does not reduce it further.

In the forums, Zah discusses cyclic dependencies, which C++ partially solves via forward-declared references. But those are also used simply to reduce compilation time.

In Go, everything in the same "package" (i.e. directory) can access each other's privates. In Nim, you could combine all files in a directory into a single file. I'm wondering: Is that sufficient to solve cyclic dependencies? In other words, do completely separate directories/modules/whatever really require cyclic dependencies?

I wouldn't mind a Go-style solution, where separate files are possible but they act as if they are a single file in that directory. Or maybe we need to look more closely into "modules", e.g. the new-ish Go modules or C++17 (which improves compilation speed significantly).

[Araq]

It seems to me that include + a call to patchFile in your config can achieve your goals.

[awr1]

What I would like in auxiliary to this is some way of controlling doc generation: if you have a module that you want to exclusively private-import, it would be useful to be able to have some directive to not generate docs for that module.

[timotheecour]

@Araq

It seems to me that include + a call to patchFile in your config can achieve your goals.

that was the 1st thing I had tried, but I gave up with that approach as it just doesn't work in practice (see example below):

• using include to patch the file with export at the end produces Error: recursive dependency, which makes sense
• so what I had done instead was to automatically copy the file to be patched in config.nims, automatically add the export at the end, and put it in a temp location along with the things I needed to export (which is already quite hacky), however this still doesn't work well at all:
• private fields don't get exported, so patchFile doesn't help here
• the symbols I've exported are now exported for all code that imports foo, ie the patch has global effects; this breaks code with ambiguous identifier errors; that's a fundamental flaw of this approach
• any code that relies oncurrentSourcePath in patched file will give wrong results
• fully qualified access foo.fun doesn't work if foo is patched as foo2.nim since the module identifier changes to foo2 ; this affects code I'm not even touching (all code that imports foo)
• if the patching file is put in a different directory than the patched file (eg to preserve same filename), it won't work if there is any relative import; or worse, it'll silently import the wrong file.
• patchFile is ambiguous if 2 modules have same name in package

# top/main.nim
import top/bar/foo
let t = foo.fun1() # BUG: undeclared identifier since foo was patched as `foo2`
echo t.x # BUG: patchFile won't work with private fields

# top/bar/foo.nim
import ./fooRelative # BUG: relative imports won't work
type A = object
  x: int
proc fun1(): A = discard

# top/overrides/foo2.nim
include top/bar/foo # BUG: Error: recursive dependency
export A
export fun1

# config.nims
patchFile("top", "foo", "top/overrides/foo2.nim")

this PR has none of those issues and just works

[timotheecour]

I've added a large test suite (which explains why there are many files in this PR; see updated top-level post for details); this feature seems to work in all cases: these all work as you'd expect with this feature:
declared / export / compiles / various forms of import / using fully or non-fully qualified idents, field access / mixing both private and non-private import in the same file

[awr1]

Also, had a question: can you private-import using import xyz / [a, b, c] syntax? Or is it just not allowed? Does this also work with from xyz import nil? (nvm, saw tests)

[timotheecour]

Also, had a question: can you private-import using import xyz / [a, b, c] syntax? Or is it just not allowed?

not implemented but not hard to add, I can add that in a follow-up PR after this one is merged

[Araq]

Cyclic imports are more important and could be more beautiful than this rather ugly solution. And before we have a half-working version of incremental compilation (IC) we shouldn't add cyclic imports either. So I'm afraid this PR is stuck until IC is in beta-quality shape.

Worse than that, the feature doesn't allow anything that's currently impossible. Yes, public fields are suboptimal, but who can judge at this point that {.privateImport.} is not worse?

[pb-cdunn]

So I can do this?

from foo {.all.} import nil

That's mainly what I would do.

[c-blake]

Possibly a redundant argument over simply "more descriptive", but {.private*.} also almost sound, at first blush if one doesn't fully understand Nim/think, "more safe" rather than the intended "less safe"/"more power user-y"/more taking API compatibility/understanding internals into one's own hands things that they are.

In that light, import foo {.unsafe.} might also be a good option. I realize I'm undermining my own triple-registered/single-counted upvote. { He Who Does The Counting is free to adjust however, like Araq counts as +3 or something. ;-) } Personally, I wouldn't mind importAll or importUnsafe new keyword approaches either.

My idea of import[all] was that "parameterized imports" might be able to help with cyclic imports by getting guidance from the user, as in import[cyclic] or some such. Such guidance may or may not help much, and I agree it would be better to not need it. Having the qualifying text be smack-right-up-against "import" would also make answers to questions like @pb-cdunn's maybe more "self-evident". OTOH "import pragmas" might be a fine new vector, possibly opening up creativity. Just mentioning some trade-offs/concerns/ideas.

[timotheecour]

@Araq PTAL

• added changelog

• renamed to {.all.}, following @c-blake's suggestion and arguments which I agree with.

EDIT: And also the fact that the same pragma can be reused for a future export {.all.} foo with analog meaning.

So I can do this?
from foo {.all.} import nil
That's mainly what I would do.

yes, see test suite (tests/privateimports/mt4b.nim); all import features are supported and work the way you'd expect.

from https://forum.nim-lang.org/t/7193#45495

Also, it did not seem to arise on the GH discussion, and I didn't see it at a glance in the PR, but it probably bears mention that multi-module imports probably all get the same treatment. import a, b {.all.} === import a {.all.}; import b {.all.}.

no, it's import ./m1, m2 {.all.}, ./m3 {.all.} which is more flexible and consistent with pragmas in nim in general, eg: var a {.noinit.}, b {.noinit.}, c: array[2, float]

I've now added a test for that, see ./tests/privateimports/mt8.nim

[c-blake]

Ok. I agree per-import-argument repetition is more flexible. I guess that sinks my import[all] and importAll ideas, too. Ah well. (Of course, one could always split into multiple statements.)

What about re-export via export statements? Do we want A) a requirement to have to say export m2 {.all.} to re-export everything, B) to automatically export as we imported, or C) forbid re-export of hidden nams? I couldn't quite figure out what would happen just looking at your PR. There was a commented out # export m3. I would probably lean toward A) myself, but maybe allow an explicit mention of some symbol like export m3.hidden. That might allow more surgical/precise name visibility manipulations as well as broad brush stuff. Apologies if this is all already handled.

Also, you should probably rename your tests/ subdir to like importalls instead of privateimports, but you might want to see if that name sticks first. :-)

[timotheecour]

Ok. I agree per-import-argument repetition is more flexible. I guess that sinks my import[all] and importAll ideas, too. Ah well. (Of course, one could always split into multiple statements.)

It's easy to write importAll(m1, m2) as library code on top of import foo {.all.} to do what you want, see how I do this in #10527

What about re-export via export statements?

I added a test, see mt9.nim; it's almost like what you suggested except for export m3h3 which is allowed even without export mt9.m3h3 since it's in scope.

import ./m3 {.all.}
export m3 # only public
# export m3 {.all.} # xxx this can be supported in future
export m3h3 # private m3.m3h3 is in scope, so its fair game to re-export
export m3.m3h4 # ditto

Also, you should probably rename your tests/ subdir

done

[Araq]

Needs an RFC and also probably will linger in limbo for a bit because I want to merge my araq-ic4 branch first which touches the same modules...

[timotheecour]

Needs an RFC

done, see nim-lang/RFCs#299

and also probably will linger in limbo for a bit because I want to merge my araq-ic4 branch first which touches the same modules...

Note that this PR has been ready since july 2019 and rebased many times to fix bitrot conflicts. I would really like to see this feature being merged sooner rather than later.