Introduce warnpcfail as a substitute for assert (#241)

Addresses JuliaLang/julia@c0f9666#commitcomment-47799292

Project.toml

@@ -1,7 +1,7 @@
 name = "SnoopCompile"
 uuid = "aa65fe97-06da-5843-b5b1-d5d13cad87d2"
 author = ["Tim Holy <[email protected]>"]
-version = "2.5.2"
+version = "2.6.0"
 
 [deps]
 Cthulhu = "f68482b8-f384-11e8-15f7-abe071a5a75f"

SnoopCompileCore/src/snoopi.jl

@@ -91,11 +91,11 @@ function __init__()
     # We do this in __init__ to make sure it gets compiled to native code
     # (the *.ji file stores only the inferred code)
     if isdefined(Core.Compiler, :Params)
-        @assert precompile(typeinf_ext_timed, (Core.MethodInstance, Core.Compiler.Params))
-        @assert precompile(typeinf_ext_timed, (Core.MethodInstance, UInt))
+        precompile(typeinf_ext_timed, (Core.MethodInstance, Core.Compiler.Params)) || error("precompilation of typeinf modifiers is not allowed to fail")
+        precompile(typeinf_ext_timed, (Core.MethodInstance, UInt)) || error("precompilation of typeinf modifiers is not allowed to fail")
     else
-        @assert precompile(typeinf_ext_timed, (Core.Compiler.NativeInterpreter, Core.MethodInstance))
-        @assert precompile(typeinf_ext_timed, (Core.MethodInstance, UInt))
+        precompile(typeinf_ext_timed, (Core.Compiler.NativeInterpreter, Core.MethodInstance)) || error("precompilation of typeinf modifiers is not allowed to fail")
+        precompile(typeinf_ext_timed, (Core.MethodInstance, UInt)) || error("precompilation of typeinf modifiers is not allowed to fail")
     end
     precompile(start_timing, ())
     precompile(stop_timing, ())

SnoopCompileCore/src/snoopi_deep.jl

@@ -96,7 +96,6 @@ end
 
 # These are okay to come at the top-level because we're only measuring inference, and
 # inference results will be cached in a `.ji` file.
-@assert precompile(Core.Compiler.Timings.reset_timings, ())
-@assert precompile(start_deep_timing, ())
-@assert precompile(stop_deep_timing, ())
-@assert precompile(finish_snoopi_deep, ())
+precompile(start_deep_timing, ())
+precompile(stop_deep_timing, ())
+precompile(finish_snoopi_deep, ())

docs/src/snoopi.md

@@ -184,15 +184,18 @@ inference on those methods, because it used the inference results from the cache
     methods that appear in your `"precompile.jl"` file.
     This will *not* result in an error; by default `precompile` fails silently.
     If you want to be certain that your precompile directives don't go stale,
-    preface each with an `@assert`.
-    Note that this forces you to update your precompile directives as you modify your package,
-    which may or may not be desirable.
+    you can check that `precompile` returns `true` and otherwise issue a warning.
+    By default, [`SnoopCompile.write`](@ref) generates
+    a macro, `@warnpcfail`, and you can use it by
+    changing `precompile(args...)` to `@warnpcfail precompile(args...)`.
+
 
 If you find that some precompile directives are
 ineffective (they appear in a new `@snoopi` despite being precompiled) and their
 inference time is substantial, sometimes a bit of manual investigation of the callees
 can lead to insights. For example, you might be able to introduce a precompile in a
 dependent package that can mitigate the total time.
+(`@snoopi_deep` makes the analysis and resolution of these issues more straightforward.)
 
 ## Producing precompile directives manually
 
@@ -278,8 +281,7 @@ This will cause Julia to infer this method for the given argument types. If you
 
 !!! note
     The `true` indicates that Julia was successfully able to find a method supporting this signature and precompile it.
-    Some people put `@assert` in front of their package's `precompile` statements--this way, if you delete or modify methods, "stale"
-    `precompile` directives will trigger an error, thus notifying you that they need to be updated.
+    See the note about `@warnpcfail` above for ways to exploit this in your package.
 
 
 But if you execute these lines in the REPL, and then check how well it worked, you might see something like the following:

src/SnoopCompile.jl

@@ -64,6 +64,21 @@ const kwbodyrex = r"^##(\w[^#]*)#\d+"         # detect keyword body methods
 const genrex = r"^##s\d+#\d+$"                # detect generators for @generated functions
 const innerrex = r"^#[^#]+#\d+"               # detect inner functions
 
+# This is for SnoopCompile's own directives. You don't want to call this from packages because then
+# SnoopCompile becomes a dependency of your package. Instead, make sure that `writewarnpcfail` is set to `true`
+# in `SnoopCompile.write` and a copy of this macro will be placed at the top
+# of your precompile files.
+macro warnpcfail(ex::Expr)
+    modl = __module__
+    file = __source__.file === nothing ? "?" : String(__source__.file)
+    line = __source__.line
+    quote
+        $(esc(ex)) || @warn """precompile directive
+     $($(Expr(:quote, ex)))
+ failed. Please report an issue in $($modl) (after checking for duplicates) or remove this directive.""" _file=$file _line=$line
+    end
+end
+
 # Parcel
 include("parcel_snoopc.jl")
 

src/parcel_snoopi_deep.jl

@@ -1804,12 +1804,12 @@ end
 
 for IO in (IOContext{Base.TTY}, IOContext{IOBuffer}, IOBuffer)
     for T = (InferenceTimingNode, InferenceTrigger, Precompiles, MethodLoc, MethodTriggers, Location, LocationTriggers)
-        @assert precompile(show, (IO, T))
+        @warnpcfail precompile(show, (IO, T))
     end
 end
-@assert precompile(flamegraph, (InferenceTimingNode,))
-@assert precompile(inference_triggers, (InferenceTimingNode,))
-@assert precompile(flatten, (InferenceTimingNode,))
-@assert precompile(accumulate_by_source, (Vector{InferenceTiming},))
-@assert precompile(isprecompilable, (MethodInstance,))
-@assert precompile(parcel, (InferenceTimingNode,))
+@warnpcfail precompile(flamegraph, (InferenceTimingNode,))
+@warnpcfail precompile(inference_triggers, (InferenceTimingNode,))
+@warnpcfail precompile(flatten, (InferenceTimingNode,))
+@warnpcfail precompile(accumulate_by_source, (Vector{InferenceTiming},))
+@warnpcfail precompile(isprecompilable, (MethodInstance,))
+@warnpcfail precompile(parcel, (InferenceTimingNode,))

src/write.jl

@@ -1,20 +1,35 @@
 # Write precompiles for userimg.jl
-function write(io::IO, pc::Vector{<:AbstractString})
+const warnpcfail_str = """
+# Use
+#    @warnpcfail precompile(args...)
+# if you want to be warned when a precompile directive fails
+macro warnpcfail(ex::Expr)
+    modl = __module__
+    file = __source__.file === nothing ? "?" : String(__source__.file)
+    line = __source__.line
+    quote
+        \$(esc(ex)) || @warn \"\"\"precompile directive
+     \$(\$(Expr(:quote, ex)))
+ failed. Please report an issue in \$(\$modl) (after checking for duplicates) or remove this directive.\"\"\" _file=\$file _line=\$line
+    end
+end
+"""
+
+function write(io::IO, pc::Vector{<:AbstractString}; writewarnpcfail::Bool=true)
+    writewarnpcfail && println(io, warnpcfail_str, '\n')
     for ln in pc
         println(io, ln)
     end
 end
 
-function write(filename::AbstractString, pc::Vector)
+function write(filename::AbstractString, pc::Vector; kwargs...)
     path, fn = splitdir(filename)
     if !isdir(path)
         mkpath(path)
     end
-    ret = nothing
-    open(filename, "w") do io
-        ret = write(io, pc)
+    return open(filename, "w") do io
+        write(io, pc)
     end
-    return ret
 end
 
 """
@@ -31,6 +46,7 @@ function write(prefix::AbstractString, pc::Dict; always::Bool = false)
     end
     for (k, v) in pc
         open(joinpath(prefix, "precompile_$k.jl"), "w") do io
+            println(io, warnpcfail_str, '\n')
             if any(str->occursin("__lookup", str), v)
                 println(io, lookup_kwbody_str)
             end

test/runtests.jl

@@ -18,6 +18,7 @@ end
 
 using SnoopCompile
 
+@testset "Miscellaneous" begin
 # issue #26
 logfile = joinpath(tempdir(), "anon.log")
 @snoopc logfile begin
@@ -46,6 +47,22 @@ data = SnoopCompile.read(logfile)
 pc = SnoopCompile.parcel(reverse!(data[2]))
 @test any(startswith.(pc[:IsDef], "isdefined"))
 
+@testset "Warning for failures to precompile" begin
+    pcs = ["@warnpcfail precompile(fsimple, (Char,))",
+           "@warnpcfail precompile(fsimple, (Char, Char))",
+           "@warnpcfail precompile(fsimple, (Char, Char, Char))",
+    ]
+    fn = tempname() * ".jl"
+    open(fn, "w") do io
+        println(io, "fsimple(x, y) = 1")
+        SnoopCompile.write(io, pcs; writewarnpcfail=true)
+    end
+    @test_logs (:warn, r"precompile\(fsimple, \(Char,\)\)") (:warn, r"precompile\(fsimple, \(Char, Char, Char\)\)") include(fn)
+    rm(fn)
+end
+
+end
+
 #=
 # Simple call
 let str = "sum"