Make the package JET-clean and run JET on all reported code

[PatrickHaecker]

I think I found a solution which is the best of both worlds from our discussion of the feature.

The JET workflow analyzed only FixedPointNumbers itself and printed an unrestricted report on the side, which is less efficient can be confusing as they differed. 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 FixedPointNumbers versions 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:

• signed and unsigned for FixedPoint threw a MethodError only because rem has no method for a type argument. They now throw the MethodError explicitly. This enables JET to understand that this is on purpose (and humans who haven't read the comment).
• isinteger reached the Fixed method of _floor through the generic _trunc for FixedPoint{<:Unsigned}, with an impossible Fixed{Union{}}, 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. _trunc stays unchanged.
• 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]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.78%. Comparing base (d62abf3) to head (6a7e78f).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kimikage]

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.

[PatrickHaecker]

Thanks for the review, @kimikage. I think I have covered everything. Please check again.

[kimikage]

I haven’t tested it myself, but I suspect there are other pieces of code that break in the T == Union{} case.
Are you planning to add guards to all of them?
And are you planning to add test cases to all of them just to cover those lines?
In terms of the goal of eliminating JET warnings, this clearly constitutes an anti-pattern.
Such guards are only justified when ambiguity or invalidation actually causes real harm.

[PatrickHaecker]

In terms of the goal of eliminating JET warnings, this clearly constitutes an anti-pattern.

I'd like this to only be a JET problem. Yet, in general (although not for the very example you were referring to) it is a weakness of Julia's type system. We normally don't want to support Bottom, but we can't exclude it without an additional method. So all sort of mechanisms can go off, like ambiguity and invalidation. And I wouldn't be surprised if it could also effect inference and codegen.

I guess defining a method for types you didn't want to define it, is just not the best idea.

I suspect there are other pieces of code that break in the T == Union{} case.
Are you planning to add guards to all of them?

Your intuition was correct. I hope I have now identified and fixed all of them.

Let's define "all" a bit. We need to define it

• if we own a function (our module is the parentmodule) and
• if the function has a method which accepts a Type and
• if that Type is not exactly specified, but only has an upper limit (i.e. lower limit is Bottom) and
• if the function is public API or we have an internal user for a function that is private API

So that limits the number of affected functions quite a bit.

For private API we don't need to do it proactively. We can do it when not having the method actually creates a problem.

Such guards are only justified when ambiguity or invalidation actually causes real harm.

If we don't own the function, it's not our responsibility, because it would be type piracy. For public functions we own, however, we can't wait to create it, because it can already be a problem somewhere in the ecosystem now or it could become one any time.

And are you planning to add test cases to all of them just to cover those lines?

Yes, added.

Thanks again for the finding. I hadn't realized that the problem is indeed larger than I thought.

[kimikage]

You may not think so, but the conversation has evolved beyond the PR goal of suppressing JET's warnings.

Unless I know what the specific harm is, I cannot assess whether the countermeasure for Union{} is effective.
The term "harm" as used here refers to problems encountered in real-world scenarios involving this package, not to test cases created artificially.
If JET's warning is indeed an "actual problem", then you are the one causing the problem. In other words, if I reject this PR, the problem will be resolved. But that's not really the point, is it?

Including the changes to JET.yml, this PR covers four distinct topics.
Let's split the PR to make the change history easier to understand.

[kimikage]

floattype and scaledual are exported. While the case of scaledual is a bit debatable, the change to floattype affects the API design, as discussed in PR #362.