nim-lang / RFCs

A repository for your Nim proposals.
135 stars 26 forks source link

import foo {.all.} #299

Closed timotheecour closed 3 years ago

timotheecour commented 3 years ago

PR

See https://github.com/nim-lang/Nim/pull/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

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

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:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

this has finally been merged: https://github.com/nim-lang/Nim/pull/17706

kaushalmodi commented 3 years ago

@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 commented 3 years ago

I've removed need for an experimental following https://github.com/nim-lang/RFCs/issues/299#issuecomment-741590240 (and I agree with that comment)

import foo {.all.} is mentioned in changelog

kaushalmodi commented 3 years ago

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 commented 3 years ago

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)