Open timholy opened 3 years ago
I'm really glad to know you gave a try on this package ! I've always wanted to make more collaboration with other "code-analysis" tools, and yes, making a comparison would be the valuable first step. I'll definitely try to address #104 this weekend.
SnoopCompile is only going to be able to catch a subset of what JET can detect
I think JET won't cover all the code quality issues that SnoopCompile can detect, as far as SnoopCompile interacts with the actual runtime (at least in the near future). JET is "almost-static" code analyzer and it's supposed to only concretize toplevel definitions and such, and so if toplevel call signatures are hard to be inferred, then there will be many things that JET will miss while SnoopCompile can know without any problem. One of my ambitious goal of JET is to make it configurable so that users can specify "how much JET should concretize my code" and can use JET for various kinds of code (I'm thinking something like what explained in https://dl.acm.org/doi/10.1145/3290356). Once we can realize it, then we may be able to say "JET subsumes SnoopCompile's analysis features", but I think it won't be the very near future; my current highest priority lies in improving analysis accuracy and robustness for now..
Yeah, I agree, it's really interesting to look at both. Having SnoopCompile with access to the runtime has made me realize that even tools like Cthulhu are partially handicapped in terms of replicating what would actually happen in real-world usage. So you're right that there's stuff SnoopCompile can do that JET can't, though the converse is also true. I definitely would enjoy looking for any chances to collaborate, though of course since the modes of operation and data sources are quite different there may be fewer of those than one might hope.
Bottom line, if you see anything in SnoopCompile's code that you'd like factored out for use in JET, don't be shy about asking. And I'll keep playing with JET and getting to know its code base (time permitting) and do the same.
So now I believe #104 is fixed, and I got JET analysis result against the test file:
julia -e 'using Pkg; Pkg.activate("."); Pkg.instantiate(); using JET; @time JET.profile_file("test/runtests.jl")'
Good or bad, all of the reported issues are from Base
and Core.Compiler
(i.e. those addressed by https://github.com/JuliaLang/julia/pull/39761 and those from Core.Compiler
's missing imports, also note, the errors from Core.Compiler
is reached from Base.return_types
call from @inferred
macro).
Actually, JET concretizes too many code within the test file because of #113 , but as far as I manually tried to find errors within PaddedViews (e.g. by n = 10; @report_call PaddedView(0, ones(Int,ntuple(d->1,n))
), I couldn't find actual errors within PaddedViews.jl.
I still haven't detailed look into SnoopCompile's analysis result, so will do a comparison this week.
That's awesome! I'll have to look too. Immersed in other things at the moment, though.
Looks like PaddedViews
is JET-clean now:
═════ 1 possible error found ═════
┌ @ C:\Users\Win10\.julia\packages\PaddedViews\7EBsT\src\PaddedViews.jl:300 PaddedViews.error("must supply at least one array with concrete axes")
│┌ @ error.jl:33 error(::String)
││ may throw: Base.throw(Base.ErrorException(s::String)::ErrorException)
│└───────────────
Not just due to JET, though I'm sure that helps. See https://github.com/JuliaArrays/PaddedViews.jl/pull/45.
If you want a real-world test case, I got #104 from attempting to use JET on the PaddedViews.jl
runtest.jl
file. I comment out the ambiguity and doctest portions, and then with SnoopCompile I get this:Once #104 is addressed, this is basically what I'd try next until one gets to the state where JET can run this same example. Even with everything I've added (e.g., the
isignorable
stuff), I think most of the remainder is unfixable in the context of this package itself (Base might be a different story). SnoopCompile is only going to be able to catch a subset of what JET can detect, and even then SNR is a bit of a problem. (Still, 11 out of an initial 264 potential issues is a nice reduction.)I'm suggesting PaddedViews simply because it's a package that has already gotten "the treatment" with SnoopCompile so it would be especially rewarding if JET discovered additional improvements. Obviously, it would be good to compare on packages that do have fixable issues; your README demo has examples of things that SnoopCompile can't catch.