test: add JET and fix issues#687
test: add JET and fix issues#687oameye wants to merge 5 commits intoJuliaHomotopyContinuation:mainfrom
Conversation
|
Thanks! I pushed after running JuliaFormatter (we still use an old version, should update it at some point...). |
|
btw I think it would also be good to add AQUA.jl. However this will likely require bigger changes. |
|
Seems that the JET test failed on lts. I didn't test on 1.10, and since the compiler changes between Julia version, it finds different issues. Will have a look later. Feel free to review already the other changes. |
|
On LTS it report a false positive for the macro |
|
Since tests take absolutely ages, probably best to split this into a separate check |
The test took very long already before this PR. It doesnt seem to spend significantly more time. But yes, splitting it into a separate check makes sense to keep the overview. |
|
Yeah good idea to parallelize :) |
I can take care of that in April (I dont have time before :() |
Now worries. I am on a PhD thesis deadline until the end of the month. Will try to finish this afterwards :) |
Fix 148 JET.jl static analysis reports (151 → 3)
Hey, I am looking to improve the TTFX of the package a bit. I thought a good start would be to add JET.jl to the test suite.
This PR used Claude during development to solve the JET reports. The overview below was also made with Claude.
It found a couple of serious bugs like:
However, since the current test suite didn't catch it. These methods can likely be removed?
Anyway let me know what you think.
Summary
Ran
JET.report_package(HomotopyContinuation; target_modules=(HomotopyContinuation,))and systematically fixed 148 of 151 reports. The remaining 3 are filtered as known (cos/sin/sqrtforIComplexF64— implementing them causes +4s TTFX regression via@generated execute_instructions!recompilation).Zero performance regression on precompilation, load time, TTFX, and steady-state runtime. Full test suite passes (2549/2549).
Highlights
Real bugs found (12 fixes)
JET uncovered genuine bugs that would crash at runtime — meaning these code paths are never exercised by the test suite and may be candidates for removal:
DoubleDouble.jl:351—mod(x, y)used undefineda,b(copy-paste fromdivremabove)interval_arithmetic.jl:50—hash(::Interval, h)used undefineduinstead ofhinterval_arithmetic.jl:249—inv(pow(x, -n))wherepowis never definedsymbolic.jl:789—rethrow(e)but the catch variable iserrsymbolic.jl:1102,1442—map(var_groups)beforevar_groupsis assigned (should bemap(variable_groups))intermediate_representation.jl:384—throw(ExprError(...))butExprErroris never definedinterpreted_homotopy.jl:221— Acbevaluate!missingtparameter entirelylinear_algebra.jl:342— unqualifiedSingularException(needsLA.SingularException)utils.jl:484/498/513—mul!(c::Float64, x::BigFloat) = mul!(x, c)— 2-arg form doesn't exist (infinite recursion)norm.jl:20,27—distance/normfallbacks return aMethodErrorobject instead of throwing itDoubleDouble.jl:1032,1046—DomainError()with no argumentsThese all crash unconditionally when reached, proving the code paths have zero test coverage. In particular
mod(::DoubleF64, ::DoubleF64),hash(::Interval),Interval^(-n), and theBigFloatmul!/add!2-arg forms are completely broken — worth considering whether they should be removed rather than fixed.Type narrowing (JET can't prove runtime invariants)
The bulk of fixes help JET verify types through patterns it can't reason about. For example, many mutable structs store
Union{Nothing,T}fields that are lazily initialized:For the same reason
something()is used throughout — it unwrapsUnion{Nothing,T}and throws a clearArgumentError("nothing")instead of a confusingMethodError:Other type narrowing patterns used:
Union{Nothing,Vector}toVectorwhere constructors always provide concrete values (subspace_homotopies.jla_minus_b/offset)F::RandomizedSystem,cell::MixedCell,NewtonCache(...)::NewtonCache{...})numerical_irreducible_decomposition.jlandwitness_set.jlDispatch / missing method fixes
on_affine_chart(::AbstractSystem)hadcompileas positional instead of keyword — actual dispatch failureCompiledSystem,InterpretedSystem,CompiledHomotopy,InterpretedHomotopy,MixedHomotopy) didn't acceptkwargs...passed byfixed()variable_groups(::Homotopy),Homotopy(::InterpretedHomotopy),path_results(::AbstractVector{<:PathResult}),jacobian!(::InterpretedSystem, 5-arg),_variables(::ExpressionRef),horner(::Expression, ::Variable)MatrixWorkspaceconstructor refactored for type-stability (extracted helper to avoidUnion{Matrix, StructArray}in constructor body)Filtered reports (3)
cos/sin/sqrtforIComplexF64are not implemented. Adding them (whether onBase.cosor on the package-internalop_cos) triggers recompilation of the@generated execute_instructions!interpreter for allIComplextape types, causing +4s TTFX on firstcertify(). These only matter when certifying systems with transcendental functions. The test filters them with a comment explaining the trade-off.Test integration
Added
test/jet_test.jlas part of the regular test suite (JET added as test dependency). Filters the 3 knownIComplexF64reports and asserts 0 remaining.Benchmarks (vs main)
Benchmarked sequentially (no concurrency), alternating branches, 3 runs each.
Precompilation:
@elapsed Pkg.precompile()after clearing~/.julia/compiled/*HomotopyContinuation*Load time:
@elapsed using HomotopyContinuationwith cache presentTTFX:
@timeon first call (includes JIT):Runtime:
@benchmarkafter full warmup:No runtime regression. Identical allocation counts (±15 allocs on certify from the split Arblib method).