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.} #299

Closed
timotheecour opened this issue Dec 9, 2020 · 9 comments
Closed

import foo {.all.} #299

timotheecour opened this issue Dec 9, 2020 · 9 comments

Comments

@timotheecour
Copy link
Member

timotheecour commented Dec 9, 2020

PR

See nim-lang/Nim#11865; fully implemented, with extensive tests, trying all possible situations including declared/compiles/import as/from/field access etc. It works and I've been using it successfully for some time in my patched version of nim, try it out.

description

  • provides a surgical way for client code to access specific private symbols in a module.
  • works in combination with all import syntaxes (eg from x {.all.} as x2 import y1, y2, etc)
  • for now, hidden behind a {.experimental: "enableImportAll".} which can be scoped or passed via cmdline
  • import foo {.all.} doesn't affect other modules importing foo

example

simplest example (see PR tests for more complex examples)

# bar.nim
proc fn() = discard

# tbar.nim
from bar {.all.} import fn
fn()

use cases

  • allows testing private symbols without having to make them public just to test them, or without having to refactor your project, or using include, which breaks modularity
  • simplifies code reusing within a package without requiring complex refactorings or having to expose internal functionality
  • allows (at your own risk) accessing private 3rdparty symbol you need (eg during temporary development, debugging etc) without having to patch 3rdparty code; Note that include would have the same problem, but worse: since the include is more invasive than something like: from foo {.all.} import bar which is a much more targeted violation of visibility)
  • reduces need for include in compiler sources, unbloat system.nim etc

This is orthogonal to cyclic imports which solve the organization of modules.

why not use include?

this has been explained in many places, eg here https://forum.nim-lang.org/t/4296#26730
while include has its (now, very few) use cases, import {.all.} 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 breaks nim doc, since doc comments of included module appear in including module (see regression + RFC to deprecate prelude Nim#16238)
  • it affects semantics of currentSourcePath, nimrtl which exports unmangled proc names, so also runs into above issues
  • many other 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)

would fix recurring issues

this fixes a frequently occuring problem in a simple way, with surgical effect (no global effects).

I had originally proposed this here: import foo {.private.} to allows access to private fields (eg: package-level visibility) - Nim forum

@bluenote10
Copy link

What about

import bar
from bar {.all.} import CertainType

vs

from bar {.all.} import CertainType
import bar

Would CertainType be imported with access to its private fields in both cases (which would probably mean that {.all.} imports in general override what is already imported?), or does the order of import matter?

@Araq
Copy link
Member

Araq commented Dec 9, 2020

for now, hidden behind a {.experimental: "enableImportAll".} which can be scoped or passed via cmdline

I know that it fits our guidelines, but these guidelines have to be changed. Experimental should not be for new features that are clearly opt-in already with dedicated syntax. It makes no sense. There is no chance I would write {.all.} without knowing what it does, it shouldn't be necessary to enable it via .experimental. A feature is experimental if the manual says so.

@disruptek
Copy link

disruptek commented Dec 9, 2020

I mean, I kinda sympathize with putting control in the hands of the developer, but at the same time... why have limited visibility if you're going to give people x-ray goggles.

Here's an alternate approach that gives the library author more control:
https://github.com/disruptek/grok/blob/master/grok/star.nim

Note starIf in the example:

  macro hasStar(n: typed, op: string): bool =
    var n = n
    if n.kind == nnkCheckedFieldExpr:
      n = n[0]
    result = infix(newLit n[0].getTypeInst.isExported,
                   op.strVal, newLit n[1].isExported)

  type
    Yep* {.star.} = object
      one: int
      two: float
      x, y: string
    Nah {.star.} = object
      one: int
      two: float
    Oof* {.star.} = object
      case kind: bool
      of on:
        one: int
      of off:
        two: float
      x, y: string
    Dev {.starIf: not defined(release).} = object
      one: int

  let a = Yep()
  let b = Nah()
  let c = Oof(kind: on)
  let d = Dev()

  assert a.one.hasStar("and")
  assert a.two.hasStar("and")
  assert a.y.hasStar("and")
  assert not b.one.hasStar("or")
  assert not b.two.hasStar("or")
  assert c.kind.hasStar("and")
  assert c.one.hasStar("and")
  when defined(release):
    assert not d.one.hasStar("or")
  else:
    assert d.one.hasStar("and")

@c-blake
Copy link

c-blake commented Dec 10, 2020

I think the per-symbol ways like your thing or accessField are complementary to opt-in x-ray goggles {.all.}. When you want to write code that is "just as if it were in module but for whatever reason it is not" and want Spider-Man responsibilities you can do {.all.}. When you are doing something more fine-grained you could use accessField. That said, if we had accessField, one could just write an operator that kept out-of-module code looking pretty similar (but for the operator not being . which might be fine).

I mostly just think this comes up often enough, and early enough in Nim user's usage that something simpler/more direct than deep macro magic would be more helpful than hurtful. Simple things should be simple, but yes, simple things are often ill-motivated. Tricky. :)

@timotheecour
Copy link
Member Author

this has finally been merged: nim-lang/Nim#17706

@kaushalmodi
Copy link

@timotheecour

for now, hidden behind a {.experimental: "enableImportAll".} which can be scoped or passed via cmdline

Looking at the PR and committed changelog, I don't see this mentioned. Should this requirement be removed from the description of this RFC?

@timotheecour
Copy link
Member Author

I've removed need for an experimental following #299 (comment) (and I agree with that comment)

import foo {.all.} is mentioned in changelog

@kaushalmodi
Copy link

import foo {.all.} is mentioned in changelog

Yes, that's why I commented above. To avoid confusion, can you remove this line from the description of this RFC?

image

@timotheecour
Copy link
Member Author

done; that's why PRs make more sense than issues for complex RFCs (with an issue it's hard to make sense of a single description on top and the comments associated, since the history is not easily visible (only via the "edited" github UI, but it's not the same)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants