Also match argnames to validate methods

[timholy]

While working on TypedSyntax.jl it became apparent that CodeTracking
sometimes returns spurious results. At least some of these arise from
the recent support of anonymous functions, #102, which might in
retrospect have been ill-considered. Rather than back that change
out, this adopts a different resolution: validate the hits more
carefully. The primary mechanism introduced here is to match not just
the function name, but also the argument names. This can work even for
anonymous functions, so we do not need to drop support for them.

This also adds quite a few new tests. These additions would have
passed before, but they proved valuable to ensure that the new
argname-matching works sufficiently well.

On TypedSyntax's "exhaustive.jl" test, this brings the
number of failed cases (specifically, the badmis) from
either 460 or 94 (depending on whether you include a few fixes
in TypedSyntax) to just 2.

TODO:

• search JuliaHub to see if any of the deleted code is used by other packages (see suggestion below)
• analyze the missingmethod output of TypedSyntax's exhaustive.jl test and see if this is missing some "real" methods (the risk is that greater finickyness may be excluding some actual methods)
• check whether JuliaInterpreter, Debugger, and Revise pass with this PR
• (most important): decide if this package should only return "usable" methods or whether "it's in here somewhere" is the behavior we'd want (see below, Also match argnames to validate methods #108 (comment)).

[timholy]

@KristofferC, one concern is whether internals here are too widely used in other packages to just delete some of the old methods.

[codecov]

Codecov Report

Patch coverage: 90.00% and project coverage change: -2.88 ⚠️

Comparison is base (ef674c6) 91.66% compared to head (91d9837) 88.79%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
- Coverage   91.66%   88.79%   -2.88%     
==========================================
  Files           3        3              
  Lines         252      357     +105     
==========================================
+ Hits          231      317      +86     
- Misses         21       40      +19     

Impacted Files	Coverage Δ
src/utils.jl	89.23% <89.76%> (-8.48%)	⬇️
src/CodeTracking.jl	86.80% <100.00%> (-0.27%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[KristofferC]

Could maybe do some grepping with JuliaHub to see how it looks?

[timholy]

I did check JuliaInterpreter, but that's a good idea. I should also run all the JuliaInterpreter/Debugger/Revise tests and make sure this doesn't mess them up.

[timholy]

To be explicit about some of the old things that went wrong: after #102, internal lines like

    idx = findfirst(str -> startswith(str, "foo"), list)

were being reported as independent function definitions if you searched for them appropriately, e.g., definition(String, Base.var"#1050"). It's not entirely wrong: here is a function definition here, f = str -> startswith(str, "foo"), and the anonymous function is indeed Base.var"#1050" (I'm making up the specific example, don't take any of this as more than an illustration). But it's more than a function definition. #102 was trying to make sure that it matches the pattern in #80, but it was incorrectly interpreting the idx = as an assignment of the anonymous function to a named variable (and completely ignoring the findfirst call).

I wanted to mention it because this PR excludes such hits, and it's not 100% obvious that this is what we want. There are good reasons to exclude them and return nothing: the extraneous material makes the result unusable by downstream tools like Debugger & TypedSyntax and may cause them to error or exhibit bizarre behavior, and returning nothing is a clear sign that they should fall back to other sources of information (defaulting to lowered/inferred code). On the other hand, for anyone using definition purely for inspection, there is useful information in returning this as the answer.

[timholy]

At least on 1.9, I am now satisfied that this is not a regression compared to what we had before. I've code- and spot-checked all the TypedSyntax-parseable methods in Base and the only ones this misses seem justifiable. There are only 32, and most of them look like https://github.com/JuliaLang/julia/blob/6d678fec0ad0c78e80f357aac1b1e99ff0ff47ca/base/toml_parser.jl#L408 (the internal -> method, not skip_ws_nl_no_comment) which somehow slip through even TypedSyntax's checks. (Weird.)

They all seem to be of that flavor, with one exception:

julia> omitted2
[1] (::Core.Compiler.var"#402#403")(x) @ Core.Compiler compiler/abstractinterpretation.jl:859
[2] (::Core.Compiler.var"#350#351")(x) @ Core.Compiler compiler/tfuncs.jl:48
[3] (::Core.Compiler.var"#250#251")(X) @ Core.Compiler abstractset.jl:91
[4] (::Core.Compiler.var"#77#78")(args) @ Core.Compiler expr.jl:829
[5] (::Base.var"#926#927")(i) @ Base errorshow.jl:928
[6] (::Base.TOML.var"#3#4")(x) @ Base.TOML toml_parser.jl:408
[7] (::Base.var"#693#694")(k) @ Base env.jl:100
[8] (::Base.Filesystem.var"#16#17")(x) @ Base.Filesystem file.jl:507
[9] (::Core.Compiler.var"#474#475")(pi, idx) @ Core.Compiler compiler/ssair/passes.jl:190
[10] (::Base.var"#66#67")(args) @ Base expr.jl:829
[11] (::Base.var"#439#442")() @ Base io.jl:1013
[12] (::Core.Compiler.var"#45#46")(x) @ Core.Compiler promotion.jl:406
[13] (::Core.Compiler.var"#231#232")(p, q) @ Core.Compiler bitset.jl:310
[14] (::Base.MathConstants.var"#7#8")() @ Base.MathConstants irrationals.jl:217
[15] (::Core.Compiler.var"#362#363")(x) @ Core.Compiler compiler/tfuncs.jl:472
[16] (::Base.var"#645#646")(task) @ Base task.jl:554
[17] (::Base.var"#68#69")(x) @ Base expr.jl:851
[18] (::Base.var"#371#373")(K, V) @ Base dict.jl:109
[19] (::Base.var"#372#374")(x) @ Base dict.jl:111
[20] (::Core.Compiler.var"#426#428")(stmt) @ Core.Compiler compiler/ssair/slot2ssa.jl:179
[21] (::Core.Compiler.var"#354#355")(x) @ Core.Compiler compiler/tfuncs.jl:204
[22] (::Base.Iterators.var"#3#4")(::Any) @ Base.Iterators iterators.jl:406
[23] (::Base.MathConstants.var"#3#4")() @ Base.MathConstants irrationals.jl:217
[24] (::Base.var"#45#46")(x) @ Base promotion.jl:406
[25] (::Base.var"#138#139")(x) @ Base abstractarray.jl:741
[26] (::Core.Compiler.var"#360#361")(f, args...) @ Core.Compiler compiler/tfuncs.jl:471
[27] (::Core.Compiler.var"#348#349")(a) @ Core.Compiler compiler/typeutils.jl:71
[28] (::Core.Compiler.var"#352#353")(fptr, rt, at, a...) @ Core.Compiler compiler/tfuncs.jl:199
[29] (::Core.Compiler.var"#384#386")(a) @ Core.Compiler compiler/tfuncs.jl:2133
[30] return_type(interp::Core.Compiler.AbstractInterpreter, t::DataType) @ Core.Compiler compiler/typeinfer.jl:1128
[31] (::Core.Compiler.Iterators.var"#3#4")(::Any) @ Core.Compiler.Iterators iterators.jl:406
[32] (::Base.var"#375#376")(X) @ Base abstractset.jl:91

The 30th one is because of a const renaming of a Core method in Base, so the method name does not have an underscore but the source-text does. That's it. So out of 32-presumptive false-negatives, only one is real, and it's a corner case. For the other 31, the new implementation appears to be doing its job by rejecting them.

[timholy]

OK, I've verified that on 1.9 this doesn't break

• JuliaInterpreter (but fails on 1.9 unless you have Fix kw pattern matching, other changes on 1.9+ JuliaInterpreter.jl#568)
• Debugger (but on 1.9 we need to add ==(::JuliaInterpreter.Variable, ::JuliaInterpreter.Variable). Did in switch its implementation from isequal to ==?)
• LoweredCodeUtils (there are a couple of failures in printing tests, but I assume it's just the usual churn and not a deeper problem)
• Revise. EDIT: hmm, there might be a failure in the REPL test, which may not run with regular Pkg.test. (fixed)

So, the only remaining question in my mind is "do we want this"? Is it better for definition(String, m) to return an incorrect result or nothing?

I favor nothing, since if your goal is visual inspection one can always edit(m) and browse the source to find it. IMO definition(String, m) mostly serves automated tools like Debugger, and this PR should be better at not serving up source code that doesn't match the method.