MilesCranmer / DispatchDoctor.jl

The dispatch doctor prescribes type stability
Apache License 2.0
138 stars 7 forks source link

Duplicated error messages during doctests #42

Open jakobjpeters opened 3 months ago

jakobjpeters commented 3 months ago

In a certain case, the error message from failed doctests of methods wrapped in @stable are duplicated. I've seen this before, but it was really challenging to debug and I couldn't pin down the source. Here is an example repository and CI job that demonstrate it. In the CI job, note that the error for f and g is displayed once but the error for h is displayed twice.

Works as expected

"""
```jldoctest
julia> f()

""" @stable f() = 1

"""

julia> g()

""" @stable begin g() = 2 end


### Duplicates error messages

````julia
@stable begin
"""
```jldoctest
julia> h()

""" h() = 3 end

jakobjpeters commented 3 months ago

It looks like the docstring gets attached to the simulator, and then for some reason Documenter.jl also tests that docstring even though it isn't included in the manual. Then, the function name isn't displayed in the error and the line numbers match, so it's difficult to tell what's going on.

julia> @macroexpand @stable begin
       """
       ```jldoctest
       julia> f()
   """
   f() = 1
   end

quote

= REPL[6]:2 =

begin
    #= /home/jakob/.julia/packages/DispatchDoctor/bx82n/src/stabilization.jl:316 =#
    begin
        function var"##f_simulator#231"(; )
            #= REPL[6]:7 =#
            1
        end
        (Base.Docs.doc!)(Main, (Base.Docs.Binding)(Main, Symbol("##f_simulator#231")), (Base.Docs.docstr)((Core.svec)("```jldoctest\njulia> f()\n```\n"), (Dict{Symbol, Any})(:path => "REPL[6]", :linenumber => 2, :module => Main)), Union{Tuple{}})
    end
    #= /home/jakob/.julia/packages/DispatchDoctor/bx82n/src/stabilization.jl:317 =#
    begin
        $(Expr(:meta, :doc))
        begin
            function f(; )
                #= /home/jakob/.julia/packages/DispatchDoctor/bx82n/src/stabilization.jl:298 =#
                var"##f_return_type#232" = (Base).promote_op(var"##f_simulator#231", map(DispatchDoctor._Utils.specializing_typeof, ())...)
                #= /home/jakob/.julia/packages/DispatchDoctor/bx82n/src/stabilization.jl:299 =#
                if (DispatchDoctor._Utils.type_instability)(var"##f_return_type#232") && (!((DispatchDoctor._Interactions.ignore_function)(f)) && (DispatchDoctor._RuntimeChecks.checking_enabled)())
                    #= /home/jakob/.julia/packages/DispatchDoctor/bx82n/src/stabilization.jl:300 =#
                    throw((TypeInstabilityError)("`f`", "REPL[6]:7", (), (;), (DispatchDoctor._Stabilization._construct_pairs)((), ()), var"##f_return_type#232"))
                end
                #= /home/jakob/.julia/packages/DispatchDoctor/bx82n/src/stabilization.jl:303 =#
                begin
                    #= REPL[6]:7 =#
                    1
                end
            end
            (Base.Docs.doc!)(Main, (Base.Docs.Binding)(Main, :f), (Base.Docs.docstr)((Core.svec)("```jldoctest\njulia> f()\n```\n"), (Dict{Symbol, Any})(:path => "REPL[6]", :linenumber => 2, :module => Main)), Union{Tuple{}})
        end
    end
end

end

MilesCranmer commented 3 months ago

Thanks for the report. Does this also occur if you set default_codegen_level="min"?

I wonder if we should try to strip the line numbers from the simulator function. As you can see, both f and f_simulator have the line #= REPL[6]:7 =# (I think this is also what confuses code coverage for default_codegen_level="debug"). Maybe Documenter is somehow attaching it based on that?

This is the generated call: https://github.com/MilesCranmer/DispatchDoctor.jl/blob/7cf8c0215420606d0c82b4287569505e71175043/src/stabilization.jl#L315-L318

I wonder if the simulator function needs to be defined second for this to work? (And the regular function's symbol repeated at the end so it is the return value)

jakobjpeters commented 3 months ago

Both default_codegen_level = "min" and removing the line number nodes still exhibit this issue.

EDIT: And also independently of each other too.

eval(Base.remove_linenums!(@macroexpand @stable default_codegen_level = "min" begin
"""
```jldoctest
julia> i()

""" i() = 4 end))

MilesCranmer commented 3 months ago

Hm... Very weird. I wonder if MacroTools.jl is adding that (Base.Docs.doc!) here?

jakobjpeters commented 3 months ago

Worth looking into! Removing the simulator's (Base.Docs.doc!) fixes the problem.

const x = @macroexpand @stable begin
"""
```jldoctest
julia> j()

""" j() = 5 end

pop!(x.args[2].args[2].args) eval(x)

jakobjpeters commented 3 months ago

Assuming DontPropagateMacro means that the macro isn't added to macros_to_use, then @doc is incorrectly branching into CompatibleMacro instead. The macro is being passed to get_macro_behavior as a GlobalRef, which defaults to CompatibleMacro. I don't think there's a public API to get the symbol out though?

As a test, switching the fallback behavior to DontPropagateMacro indeed fixes the issue.

MilesCranmer commented 3 months ago

Does putting @doc as DontPropagateMacro fix it? Or does it need to be default for all macros?

jakobjpeters commented 3 months ago

@doc is already DontPropagateMacro.

https://github.com/MilesCranmer/DispatchDoctor.jl/blob/7cf8c0215420606d0c82b4287569505e71175043/src/macro_interactions.jl#L20

The problem is that get_macro_behavior is passed a GlobalRef (containing @doc), which dispatches to this fallback.

https://github.com/MilesCranmer/DispatchDoctor.jl/blob/7cf8c0215420606d0c82b4287569505e71175043/src/macro_interactions.jl#L55

MilesCranmer commented 3 months ago

I see, thanks. One thing I’m confused about is I though Base.@__doc__ would already take care of this. So it’s weird this is happening. Maybe the _stabilize_all needs a branch for GlobalRef?