Design issue of `warnpcfail`

[kimikage]

In PR #241, the warnpcfail macro was introduced. This is a good change in that it avoids the risk of making the package unusable due to assertion errors.

However, I think there is a problem with the implementation of the warnpcfail macro.

Performance issue

The following is a rough benchmark with ColorTypes.jl v0.10.10 （Julia v1.6.0-rc1 on Windows).

    @time include("precompile.jl")
    @time _precompile_()

julia> using ColorTypes # assert
[ Info: Precompiling ColorTypes [3da002f7-5984-5a60-b8a6-cbb66c0b333f]
  0.035316 seconds (8.81 k allocations: 548.591 KiB, 17.79% gc time, 21.84% compilation time)
  5.197671 seconds (49.64 M allocations: 2.816 GiB, 10.48% gc time, 96.62% compilation time)

julia> using ColorTypes # warnpcfail
[ Info: Precompiling ColorTypes [3da002f7-5984-5a60-b8a6-cbb66c0b333f]
  0.148557 seconds (63.32 k allocations: 3.417 MiB, 3.87% gc time, 9.79% compilation time)
  6.488513 seconds (51.23 M allocations: 2.931 GiB, 9.87% gc time, 97.33% compilation time)

It is a matter of personal values as to whether one considers this difference to be significant. However, warnings should rarely be displayed, and even when they are displayed, they will not have any positive effect on the end-user experience.

Uselessness within loops

Let's also take ColorTypes.jl as an example, and assume that the precompilation fails in the loop.

    for R in realtypes
        for T in eltypes
            @warnpcfail precompile(Tuple{Type{Gray{T}},R,R}) # invalid
            @warnpcfail precompile(Tuple{Type{Gray{T}},R})

And then, we get:

julia> using ColorTypes
[ Info: Precompiling ColorTypes [3da002f7-5984-5a60-b8a6-cbb66c0b333f]
┌ Warning: precompile directive
│     precompile(Tuple{Type{Gray{T}}, R, R})
│ failed. Please report an issue in ColorTypes (after checking for duplicates) or remove this directive.
└ @ ColorTypes ~\.julia\dev\ColorTypes\src\precompile.jl:21
┌ Warning: precompile directive
│     precompile(Tuple{Type{Gray{T}}, R, R})
│ failed. Please report an issue in ColorTypes (after checking for duplicates) or remove this directive.
└ @ ColorTypes ~\.julia\dev\ColorTypes\src\precompile.jl:21
┌ Warning: precompile directive
│     precompile(Tuple{Type{Gray{T}}, R, R})
│ failed. Please report an issue in ColorTypes (after checking for duplicates) or remove this directive.
└ @ ColorTypes ~\.julia\dev\ColorTypes\src\precompile.jl:21
...

There are two main problems. One is that a huge number of warnings will be generated depending on the configuration of the loop. The other is that there is no additional information available about the signature. 😕

Discussion

As for the performance issue, it should be improved by kicking out the @warn to a @noinline function.
However, the issue of signatures cannot essentially be addressed at parse time. And if we want to generate the warning messages at execution time, a function seems simpler than a macro.

It is possible to address only the performance issues first, but we will have to manually submit a PR for each individual package with each fix. 😕

[kimikage]

There is still a problem with suppressing specialization:confused:, but here is a proposal to solve the above problem.

macro warnpcfail(ex::Expr)
    if ex.head === :call && ex.args[1] === :precompile
        ex.args = Any[:_precompile_with_warnfail,
            __module__, QuoteNode(__source__.file), __source__.line, ex.args...]
    end
    esc(ex)
end
function _precompile_with_warnfail(modl, file, line, _, @nospecialize(args...))
    precompile(args...) || _warnpcfail(modl, file, line, Any[args])
end
@noinline function _warnpcfail(modl, file, line, @nospecialize(args))
    f = file === nothing ? "?" : String(file)
    @warn """precompile directive
      precompile($(args[1]...))
      failed. Please report an issue in $modl (after checking for duplicates) or remove this directive.""" _file=f _line=line maxlog=10
    return false
end

Edit:
The only advantage of using a macro here is the display of the line number. As long as the signature is displayed, the line number is not so important.

[kimikage]

JuliaLang/julia#39905 was merged. Therefore, @warnpcfail needs to be changed somehow, but that is not really relevant to the discussion here (i.e., to my interest).