Repository navigation
Make the package JET-clean and run JET on all reported code - #363
PatrickHaecker wants to merge 3 commits into
Conversation
This seems to combine the best of both worlds [from the discussion](JuliaMath#358 (comment)) of the feature. The JET workflow analyzed only FixedPointNumbers itself and printed an unrestricted report on the side, which can be confusing. Reports from Base code reached through FixedPointNumbers are worth catching as well, so the workflow now runs `test_package` without `target_modules` and fails on any report. This helps to find regressions both in future FPN and in future Julia versions. This required removing three causes of reports on Julia 1.10 and 1.13: - `signed` and `unsigned` for `FixedPoint` threw a `MethodError` only because `rem` has no method for a type argument. They now throw the `MethodError` explicitly. - `_trunc` for `FixedPoint{<:Unsigned}` also dispatched to the `Fixed` method of `_floor`, with an impossible `Fixed{Union{}}`. It is now defined for `Normed`, the only unsigned `FixedPoint` type. - `rand!` converted the array pointer with `reinterpret`, whose Base internals JET reports on Julia 1.10. The `Ptr{T}` constructor does the same conversion and is better style anyway.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
+ Coverage 96.75% 96.78% +0.03%
==========================================
Files 7 7
Lines 800 808 +8
==========================================
+ Hits 774 782 +8
Misses 26 26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kimikage
left a comment
kimikage
left a comment
There was a problem hiding this comment.
The approach for the _trunc case is not appropriate.
If we make an exception for this particular case, it could lead to “incorrect” solutions for other potential issues involving non-concrete types.
If we were to address the JET report, it would be within throw_converterror.
I'll avoid getting into a debate about whether this truly qualifies as a breaking change, but I've labeled it "breaking" with the intention that it should be addressed in FPN v0.10.0.
The generic `_trunc` for `FixedPoint{<:Unsigned}` stays available to unsigned
`FixedPoint` types outside this package. JET reached the `Fixed` method of
`_floor` through it, because `Union{}` is both `Signed` and `Unsigned`. Bounding
that method by `T <: Signed` limits the overlap to `Fixed{Union{}, f}`, which
has no values and now has its own method.
The comment on `signed` and `unsigned` now only states why the methods exist.
|
Thanks for the review, @kimikage. I think I have covered everything. Please check again. |
|
I haven’t tested it myself, but I suspect there are other pieces of code that break in the |
`Union{}` is a subtype of every type, so methods for `Type{X} where {X <: FixedPoint}`
also match it. Calls like `floattype(Union{})` or `0.5 % Union{}` therefore ended in
ambiguity errors that named internal functions, or in errors from inside unrelated
methods. Since Julia 1.10, a method of the form `f(..., ::Type{Union{}}, slurp...)`
takes precedence over all other methods of `f`, which is how Base handles the same
case.
The exported functions with a type argument get such a method. `*`, `rem` and the
functions of `Base.Checked` belong to Base, so the internal `_convert` and `_rem`
they call are guarded instead. On older Julia versions, these calls still throw an
ambiguous `MethodError`.
The `Fixed{Union{}}` method of `_floor` cannot be reached at runtime, because
`Fixed{Union{}}` has no values, so it is excluded from coverage.
2 participants
I think I found a solution which is the best of both worlds from our discussion of the feature.
The JET workflow analyzed only
FixedPointNumbersitself and printed an unrestricted report on the side, which is less efficient can be confusing as they differed. Reports from Base code reached throughFixedPointNumbersare worth catching as well, so the workflow now runstest_packagewithouttarget_modulesand fails on any report. This helps to find regressions both in futureFixedPointNumbersversions and in future Julia versions.This required removing the three remaining causes of reports on Julia 1.10 and 1.13 when not limiting JET to
FixedPointNumbers:signedandunsignedforFixedPointthrew aMethodErroronly becauseremhas no method for a type argument. They now throw theMethodErrorexplicitly. This enables JET to understand that this is on purpose (and humans who haven't read the comment).isintegerreached theFixedmethod of_floorthrough the generic_truncforFixedPoint{<:Unsigned}, with an impossibleFixed{Union{}}, becauseUnion{}is bothSignedandUnsigned. Bounding that method byT <: Signedlimits the overlap toFixed{Union{}, f}, which has no values and now has its own method._truncstays unchanged.rand!converted the array pointer withreinterpret, whose Base internals JET reports on Julia 1.10. ThePtr{T}constructor does the same conversion and is better style anyway.