Symbols lookup regression

[cooldome]

Used to compile before:

Example

import macros

var interesting_funcs {.compileTime.}: seq[NimNode] = @[]

macro interest(fns: varargs[typed]): typed =
  for fn in fns:
    interesting_funcs.add fn[0]

proc myproc(x: float): float {.interest.} =
  2*x

myproc(3.0)

Now fails on the last line with error: undeclared identifier 'myproc'

[cooldome]

Regression is caused by this change: fb58066

@Clyybber please have a look

[Clyybber]

This is intended. Previously calling it would have just created cgen errors (at least with fns: typed, not sure if varargs elided the bug).

[Clyybber]

Adapted code would be:

import macros

var interesting_funcs {.compileTime.}: seq[NimNode] = @[]

macro interest(fns: varargs[typed]): typed =
  result = newNimNode(nnkStmtList)
  for fn in fns:
    interesting_funcs.add fn[0]
    result.add fn

proc myproc(x: float): float {.interest.} =
  2*x

echo myproc(3.0)

but this fails currently, due to a bug thats fixed with #15252

[cooldome]

Hi @Clyybber,
cgen is not related at all.

The problem is that myproc now disappears from symbol list if you apply typed macro to it.
interest here plays a role of pragma that should not affect the visibility of the myproc.

This is simplified example that fails with the same error:

import macros

macro interest(fn: typed) =
  discard

proc myproc(x: float): float {.interest.} =
  2*x

myproc(3.0)

[Clyybber]

Yeah, this is intended. cgen is related because before my PR this would just call a proc that codegen would never encounter, thus generate invalid C code.

In this case the proc is generated anyways on older Nim (not sure why, I haven't investigated), but other cases like

import macros

macro interest(fn: typed) =
  discard

interest:
  var x = 1

echo x

show why the previous behaviour was broken.

[cooldome]

Agree you change is useful and I am not suggesting reverting it, but it has unintended consequences that we should fix.

Possibly the issue is in the way sempass is handling pragmas:

proc p {.mymacro.}

internally is rewritten into:

mymacro:
  proc p

which is no the same from the scope handling point of view.
Once you have fixed the scope handling the issue started to manifest itself.

[Clyybber]

Yeah, but this is how it should work I'm arguing. There is no bug here.

[cooldome]

Not sure I agree, for the following use case yes it is clearly local function that can be swallowed:

mymacro:
  proc p

but this one, I believe is not what users expect

proc p* {.mymacro.}

you don't expect that pragma can make proc disappear, it is even exported function.

Anyway, it is more of design question. Let's get #15252 over the line that will help.

[Araq]

@cooldome we're about to release, is this really a bug that should block 1.4?

[cooldome]

Yes, absolutely. Please block the release.
I am working hard to make it fixed asap

[jyapayne]

@cooldome is this fixed now with the merge of the above two fixes?

[Clyybber]

The original snippet adapted is, but @cooldome is still hitting issues in a different codebase. We will keep this open until they are fixed (The PR that caused this issue won't ship with 1.4, so it only affects devel)

[Araq]

What's the status of this bug?

[Clyybber]

IMO this can be closed. macro pragmas consuming the definition they are attached to is better and more consistent (untyped does the same) and is needed for typed async/cps macros.