JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.55k stars 5.47k forks source link

issues with macrocall Exprs without LineNumberNodes #43976

Open StefanKarpinski opened 2 years ago

StefanKarpinski commented 2 years ago

Naturally occurring macrocall forms always have a LineNumberNode as their second arg:

julia> Meta.@dump @time true
Expr
  head: Symbol macrocall
  args: Array{Any}((3,))
    1: Symbol @time
    2: LineNumberNode
      line: Int64 1
      file: Symbol REPL[68]
    3: Bool true

LineNumberNodes don't have any semantic effect in blocks and you'd think they'd be optional in macro calls as well, but that's not the case—they both show and evaluate wrong:

julia> Expr(:macrocall, Symbol("@time"), true)
:(@time)

julia> Expr(:macrocall, Symbol("@time"), true) |> eval
ERROR: LoadError: MethodError: no method matching var"@time"(::LineNumberNode, ::Module)

If you stick any dummy second argument in there, it works:

julia> Expr(:macrocall, Symbol("@time"), nothing, true)
:(@time true)

julia> Expr(:macrocall, Symbol("@time"), nothing, true) |> eval
  0.000001 seconds
true

However, I'd argue that you shouldn't have to do this and that macro call expressions without line number nodes should both print and evaluate correctly.

simeonschaub commented 2 years ago

LineNumberNodes don't have any semantic effect in blocks and you'd think they'd be optional in macro calls as well, but that's not the case—they both show and evaluate wrong:

I am not sure the comparison with block expression makes sense here. I'd argue that LineNumberNodes aren't so much optional arguments, but instead just one kind of AST node and block is simply a collection of AST nodes

In a macrocall expression OTOH, the first two arguments have very special meanings, namely the first is the name of the macro and the second one is the location. Therefore I don't think passing the second argument as a regular first argument to the macro if it's not a LineNumberNode is consistent, since who's to argue if you have an expression Expr(:macrocall, Symbol("@name"), LineNumberNode(...), :x) that the first LineNumberNode wasn't actually meant as the first argument. (Our parser wouldn't construct such an expression, but I don't think that's an unreasonable thing to want to manually invoke.)

I'd say that the fact you can set the second argument to nothing is actually an (unintentional) feature which I know some packages rely on, since it can be used to strip any kind of line number information from expressions. So I am pretty sure such a change would be breaking.

As a secondary point, a change like this would also complicate our lowering and macro expansion passes, since this case would need to be special-cased everywhere.

bramtayl commented 2 years ago

I think it might make sense to have a __source__ that defaults to nothing, but if __source__ wasn't defined some macros wouldn't work?