Usability: {.experimental: “ForLoopMacros”.} is required at all macro callsites

[coffeepots]

There has been a series of regressions in various parts of the Nimbus toolchain starting from commit d8e66d6

The current latest HEAD ced1c13 is producing a regression for the stint library in all_tests.nim:

\stint\stint.nim(26, 10) template/generic instantiation from here
\stint\stint.nim(23, 10) template/generic instantiation from here
\stint\stint\io.nim(186, 8) template/generic instantiation from here
\stint\stint\io.nim(138, 31) template/generic instantiation from here
\stint\stint\uint_public.nim(24, 23) template/generic instantiation from here
\stint\stint\private\uint_addsub.nim(27, 10) template/generic instantiation from here
\stint\stint\private\uint_addsub.nim(22, 17) template/generic instantiation from here
\stint\stint\private\uint_comparison.nim(23, 24) Error: type mismatch: got <UintImpl[system.uint64], UintImpl[system.uint64]>
but expected one of:
macro asWords(x: ForLoopStmt): untyped
  first type mismatch at position: 1
  required type: ForLoopStmt
  but expression 'x' is of type: UintImpl[system.uint64]

expression: asWords(x, y)

It's tricky to know exactly what has caused this issue, because between d8e66d6 and ced1c13 there are different errors such as this in chronicles:

\.nimble\pkgs\chronicles-0.2.1\chronicles\options.nim(106, 18) Error: 'typedesc' metatype is not valid here; typed '=' instead of ':'?

The last successful build using stint was Aug 14 2018 (stint was last modified 10 Aug 2018), after which we start getting the above issues.
Currently the build issue has settled on the type mismatch for asWords in stint at the latest HEAD.

[mratsim]

Not a regression, it needs {.experimental: "ForLoopMacros".}, I'll fix it.

Introduced by da41fc1

[coffeepots]

Cheers @mratsim!

[coffeepots]

Unfortunately this didn't fix the issue.

[mratsim]

There is a critical usability issue currently.

Small test case:

# file "forloop1.nim"
import macros
{.experimental: "forLoopMacros".}

macro enumerate*(x: ForLoopStmt): untyped =
  expectKind x, nnkForStmt
  # we strip off the first for loop variable and use
  # it as an integer counter:
  result = newStmtList()
  result.add newVarStmt(x[0], newLit(0))
  var body = x[^1]
  if body.kind != nnkStmtList:
    body = newTree(nnkStmtList, body)
  body.add newCall(bindSym"inc", x[0])
  var newFor = newTree(nnkForStmt)
  for i in 1..x.len-3:
    newFor.add x[i]
  # transform enumerate(X) to 'X'
  newFor.add x[^2][1]
  newFor.add body
  result.add newFor

import forloop1
{.experimental: "forLoopMacros".} # Comment it and it will not work

for a, b in enumerate(items([1, 2, 3])):
  echo a, " ", b

for a2, b2 in enumerate([1, 2, 3, 5]):
  echo a2, " ", b2

I suspect there is another one with macro wrapped in procs, pending a new test case.

---

Edit, in Stint to workaround this I need to add {.experimental: "ForLoopMacros".} everywhere, even in library user code, even when the macro call is hidden away and used for internal implementations. See https://github.com/status-im/nim-stint/pull/62/files for me spraying {.experimental: "ForLoopMacros".}.

Unfortunately I couldn't reproduce with the following smaller test case so I might be missing something:

# forloop4.nim
import macros
{.experimental: "forLoopMacros".}

macro myMacro*(x: ForLoopStmt): untyped =
  result = quote do: echo "Success!"

# forloop5.nim
import forloop4, macros
{.experimental: "forLoopMacros".}

proc foo1* =
  for i in myMacro(a):
    echo i

proc foo2* =
  for i, j in myMacro(a, b):
    echo i

# forloop6
import forloop5
foo1()
foo2()

[mratsim]

Thanks for the tips @Araq.

Can now reproduce stint issue, it's due to generics (recurring early symbol resolution issue)

# forloop5.nim
import forloop4, macros
{.experimental: "forLoopMacros".}

proc foo1*[T](a: T) =
  for i in myMacro(a):
    echo i

proc foo2*[T](a, b: T) =
  for i, j in myMacro(a, b):
    echo i

import forloop5
foo1([1,2,3])
foo2([1,2,3], [4, 5, 6])

[jangko]

Nim manual says only a little explanation about experimental pragma.

The experimental pragma enables experimental language features.

It doesn't specify how exactly it should work across modules. Current implementation, most of experimental features activated by experimental pragma only active locally for the module that declare the pragma. Compiler switch --experimental:xxx will enable the feature globally for all modules.

Only a few experimental feature such as "dynamicBindSym" will work across module. Caller module doesn't have to use that pragma if it call macro(with bindSym) from a module with experimental pragma. But still the feature itself not activated at caller module. It only borrow the feature from the module that define the macro. If the caller module want to use dynamicBindSym inside it's own, still need to write the pragma. It is also the same if the called macro produce another macro that use dynamicBindSym, the caller must activate the feature first.

This is of course confusing and perhaps undesired, we need to make it clear in the spec and in the implementation. The experimental feature active globally for all modules might not wanted too.

I know experimental feature is an experimental, but it seems the experimental pragma itself also still experimental in it's current state, at least it is not well defined.

[Araq]

I know experimental feature is an experimental, but it seems the experimental pragma itself also still experimental in it's current state, at least it is not well defined.

True, but I think how this works across module boundaries needs to be decided and documented case by case.

[yglukhov]

Could it be just forLoopMacros issue and not experimental? Is it just "lack of experimental flag" is triggered too late? It probably should be triggered upon sem macro definition, and not upon it's instantiation.

Random thoughts: I think experimental should be used secretly inside a module/package. Making the users of your library add --experimental to the build script is really an awful idea. So in the end noone will ever want to experiment.

[zah]

I agree with @yglukhov and I think we should resolve all of these experimental pragma issues and complete the feature detection story before the 0.19 release. Let it be the first release that establishes a robust way to target multiple versions of Nim.

[Araq]

I agree. Now bring up good solutions please. :-)

The one idea that I have is to use Nim's "friend" mechanism (that allows generics to access private object fields, for example) to cover the .experimental switch. Then Nim would remember that the code section in a template was originally in a scope that enabled the experimental feature. But that would be quite some work and I'm not sure it would solve everything. The "friend module" feature itself has open issues...

[timotheecour]

/cc @Araq @zah @yglukhov @mratsim

How about this: introduce a new pragma stickyPragma: [..] which adds stickyPragma's arguments to every direct consumer of a symbol marked with stickyPragma

example:

# module define_enumerate
macro enumerate*(x: ForLoopStmt): untyped {.stickyPragma: [experimental: "forLoopMacros"] = ...

# module use_enumerate
import define_enumerate
# `experimental: "forLoopMacros"` not used here
for a, b in enumerate(items([1, 2, 3])): # `experimental: "forLoopMacros"` used here because `enumerate` defines `stickyPragma`
 echo a
# `experimental: "forLoopMacros"` not used here

I could see many other uses of this feature

[Araq]

You cannot use enumerate without knowing how for loop macros work. The whole idea of .experimental is to make you aware that you use an experimental feature. As such "attach this feature to the ForLoopStmt type" or "introduce a stickyPragma" fundamentally miss the point.

[yglukhov]

Since {.push experimental.} was introduced, it is now inline with the idea that you have to be aware of experimental feature being used and can wrap the macro call into push/pop experimental should you need to "hide" it. Thus I consider the issue to be resolved.

[Araq]

Great, that's how I antipicated it to work, closing.