Repository navigation
Make the package JET-clean and run JET on all reported code - #363
PatrickHaecker wants to merge 1 commit 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.75%
=======================================
Files 7 7
Lines 800 800
=======================================
Hits 774 774
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.
| # Since `FixedPoint` is not an integer type, it is not clear in what type | ||
| # `signed` and `unsigned` for `FixedPoint` should return values. They should | ||
| # currently throw errors in case we support "unsigned Fixed" or "signed Normed" | ||
| # in the future. The following "incomplete" code is necessary for Julia v1.0 | ||
| # etc. to prevent accidental conversion to an integer type. | ||
| signed(x::X) where {X <: FixedPoint} = signed(X)(signed(x.i), 0) | ||
| unsigned(x::X) where {X <: FixedPoint} = unsigned(X)(unsigned(x.i), 0) | ||
| # `signed` and `unsigned` for `FixedPoint` should return values. They throw a | ||
| # `MethodError` until "unsigned Fixed" or "signed Normed" are supported. The | ||
| # explicit methods keep the generic fallbacks from converting a `FixedPoint` to | ||
| # an integer type. | ||
| signed(x::FixedPoint) = throw(MethodError(signed, (x,))) | ||
| unsigned(x::FixedPoint) = throw(MethodError(unsigned, (x,))) |
There was a problem hiding this comment.
While this isn't an explicit agreement, it should continue to throw an error until Julia v2 is released.
As noted in PR #356, "unsigned Fixed" or "signed Normed" are no longer relevant.
If you're going to make a change, I think it would be less misleading to simply state that FixedPoint is neither <:Signed nor <:Unsigned.
Since I think this would be considered a breaking change by your standards, I won't force it, but as I stated in #348 (comment), I prefer an ArgumentError with a kind message to a MethodError.
| _trunc(x::X) where {X <: FixedPoint{<:Unsigned}} = _floor(x) | ||
| _trunc(::Type{Ti}, x::X) where {X <: FixedPoint{<:Unsigned}, Ti <: Integer} = _floor(Ti, x) |
There was a problem hiding this comment.
This is the API I recently "broke" in PR #336, but I don’t think it’s a good idea to treat it as a regular private method based solely on SemVer.
At least, the JET report does not suggest that these methods should be removed.
This is simply a case of the JET stream flowing in by chance.
Essentially, it is an unavoidable issue as long as providing non-concrete types.
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)._truncforFixedPoint{<:Unsigned}also dispatched to theFixedmethod of_floor, with an impossibleFixed{Union{}}(Type{<:…}always containsUnion{}which is unfortunate, but it's the current definition). It is now defined forNormed, the only unsignedFixedPointtype.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.