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

inferred macro causes :block Expr arguments to lose LineNumberNodes #31334

Open
NHDaly opened this issue Mar 13, 2019 · 9 comments
Open

inferred macro causes :block Expr arguments to lose LineNumberNodes #31334

NHDaly opened this issue Mar 13, 2019 · 9 comments
Labels
testsystem The unit testing framework and Test stdlib tooling

Comments

@NHDaly
Copy link
Member

NHDaly commented Mar 13, 2019

I'm not sure if this is related to #20620 or not.

The @inferred macro causes block quotes in the arguments to a function to lose their LineNumberNodes:

julia> using Test

julia> f(e::Expr) = e
f (generic function with 1 method)

julia> f(quote 5+5 end)
quote
    #= none:1 =#
    5 + 5
end

julia> @inferred f(quote 5+5 end)
quote
    5 + 5
end

I'm assuming it's got something to do with the way the temporary args variable is generated, since it's already gone by that line:

julia> @macroexpand @inferred f(quote 5+5 end)
quote
    let
        begin
            #1378#args = ($(Expr(:copyast, :($(QuoteNode(quote
    5 + 5
end))))),)
            #1379#result = f(#1378#args...)
            #1380#inftypes = (Test.Base).return_types(f, (Test.Base).typesof(#1378#args...))
        end
        if (Test.length)(#1380#inftypes) == 1
            nothing
        else
            (Base.throw)((Base.AssertionError)("length(inftypes) == 1"))
        end
        #1381#rettype = if #1379#result isa Test.Type
                (Test.Type){#1379#result}
            else
                (Test.typeof)(#1379#result)
            end
        #1381#rettype == #1380#inftypes[1] || (Test.error)("return type $(#1381#rettype) does not match inferred return type $(#1380#inftypes[1])")
        #1379#result
    end
end

but i'm not sure what about that is wrong.

@NHDaly
Copy link
Member Author

NHDaly commented Mar 13, 2019

The relevant section of the @inferred macro is here:

julia/stdlib/Test/src/Test.jl

Lines 1383 to 1387 in 5fac1b5

quote
args = ($([esc(ex.args[i]) for i = 2:length(ex.args)]...),)
result = $(esc(ex.args[1]))(args...)
inftypes = Base.return_types($(esc(ex.args[1])), Base.typesof(args...))
end

@NHDaly
Copy link
Member Author

NHDaly commented Mar 13, 2019

Oh, haha, jk, i guess it's probably this call to Base.remove_linenums! that's doing it:

Base.remove_linenums!(quote

What's the reasoning behind that call? Can we do it in a way that it doesn't affect the arguments to the expression?

@KristofferC
Copy link
Member

3bf50df

@NHDaly
Copy link
Member Author

NHDaly commented Mar 13, 2019

😆 Haha thanks. yeah it took me a while to notice that call there, literally spelling out my complaint. 😝

remove line numbers from inferred test macro
avoids interfering with line numbers in containing file

The reasoning there makes sense. Any idea about how to restructure this to avoid removing them from user input? I'm playing with it now, but Expr traversals man...

@vtjnash
Copy link
Member

vtjnash commented Mar 13, 2019

We could just drop the call. The issue it was working around is fixed. (or with more difficultly, change the structure the macro to avoid calling it on the user input, :(let args = ($([esc(ex.args[i]) for i = 2:length(ex.args)]...),); $(Base.remove_linenums!(quote ... end))); end))

@NHDaly
Copy link
Member Author

NHDaly commented Mar 13, 2019

:(let args = ($([esc(ex.args[i]) for i = 2:length(ex.args)]...),); $(Base.remove_linenums!(quote ... end))); end)

Ah, damn, that makes sense. It's better than what i came up with, which is pretty silly in retrospect:
https://github.com/JuliaLang/julia/pull/31335/files

@NHDaly
Copy link
Member Author

NHDaly commented Aug 27, 2019

I was finally picking this up again, and i noticed that actually @test does the same thing, which was added here:
6396218#diff-bef5fc2625b92f0ae308eb13927125abR185

julia> e = quote 2 + 2 end
quote
    #= none:1 =#
    2 + 2
end

julia> @test quote 2+2 end == e
Test Failed at none:1
  Expression: $(Expr(:quote, quote
    2 + 2
end)) == e
   Evaluated: begin
        2 + 2
    end == begin
        #= none:1 =#
        2 + 2
    end
ERROR: There was an error during testing

Can I make the same change to @test? That is, to remove the linenumbers from the test code, but not from Expr arguments?

@NHDaly
Copy link
Member Author

NHDaly commented Aug 27, 2019

We could just drop the call. The issue it was working around is fixed. (or with more difficultly, change the structure the macro to avoid calling it on the user input, :(let args = ($([esc(ex.args[i]) for i = 2:length(ex.args)]...),); $(Base.remove_linenums!(quote ... end))); end))

And actually, unfortunately, @vtjnash i don't think the solution you presented would work here; that still leaves linenumbers, as far as I can tell. I think the problem is that the let ... end block ends up inserting line-numbers:

julia> :(let args = (1,2,3); $(Base.remove_linenums!(quote 2+2 end)); end)
:(let args = (1, 2, 3)
      #= REPL[5]:1 =#
      begin
          2 + 2
      end
  end)

@NHDaly
Copy link
Member Author

NHDaly commented Aug 27, 2019

After thinking more about this, I wonder if the right solution is maybe to change Base.remove_linenums! to not recurse into esc() Expr trees.

I think this would solve the above problem, and match the intended behavior of Base.remove_linenums!:

  • The reason we use Base.remove_linenums! is to avoid injecting the linenumbers of the Test code into the user's test expressions. So we want to remove linenums from any code iff it was introduced by us and not the user.
  • Conversely, we want to always esc any code provided by the user exactly once, and we don't want to esc() our code, since we want our variable names to get gensym'd.

So I think that 1:1 correspondence is a nice property that we can take advantage of to solve this problem, by just changing Base.remove_linenums! to not recurse into Expr(:escape, ...) trees. Does that seem reasonable to you? :)


EDIT: I have implemented this remove_linenums! solution, here: #31335

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsystem The unit testing framework and Test stdlib tooling
Projects
None yet
4 participants