Base: TwicePrecision: disallow some constructors

[nsajko]

Disables some constructors to help in catching errors earlier.

Disables converting constructors like, for example, TwicePrecision{Float64}(::BigFloat, ::BigFloat). This constructor was actually used in some cases before commit "Base: TwicePrecision: improve constructors (#49616)" (ce2c3ae), so disabling the constructor would have brought the possible accuracy losses and construction of unnormalized double-word numbers to attention sooner.

Also effectively blacklists some types, to prevent them from being the type of a word in a TwicePrecision type. For example, TwicePrecision{<:Integer} objects are now disallowed, which is good as such a type would be nonsensical. Blacklisting is required, as opposed to whitelisting via type constraints, because of the need to support various floating-point-like types that are not <:AbstractFloat, and, more generally, various number-like types that are not <:Number (such as types representing physical units).

In the same way it is now forbidden to construct TwicePrecision{T} objects where T is not a concrete type. This should prevent the construction of TwicePrecision objects with the high and low words of differing types.

Updates #49589

[Seelengrab]

This codepath can't be hit in practice - there are no concrete objects that have an abstract type as their type, so the hi::T/lo::T dispatch requirement can't be met.

[nsajko]

Not true:

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.10.0-DEV.unknown (2023-05-06)
 _/ |\__'_|_|_|\__'_|  |  tp_disable_constructors/fc6dfb7999 (fork: 1 commits, 0 days)
|__/                   |

julia> Base.TwicePrecision{Any}(3, 0.3)
ERROR: Any isn't a concrete type
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:35
 [2] Base.TwicePrecision{Any}(hi::Int64, lo::Float64)
   @ Base ./twiceprecision.jl:201
 [3] top-level scope
   @ REPL[1]:1

[Seelengrab]

Thanks, that's true. I still think requiring this to be concrete is a bit odd.

[nsajko]

The reason that non-concrete type-parameters are usually allowed for container types in Julia is so the container's elements could be of different types, right?

In this case we don't want to allow elements of differing types, so there's no reason to allow non-concrete types. Even if I'm wrong the check can always be removed later, seeing as this is just an internal type.

That's my reasoning, anyway.

[Seelengrab]

Yes, that's true. The explicit check for isconcretetype just feels a bit iffy, is all. My guess is that the intention is to keep the TwicePrecision{T}(a,b) constructor around, right? I'm not sure whether that's actually needed (nothing should rely on it 😓 ), but maybe something like this is better, instead of explicitly checking for concreteness of the type parameter:

julia> struct MyFoo{T}
           a::T
           b::T
           MyFoo(a::T, b::T) where T = new{T}(a,b)
       end

julia> MyFoo{T}(a::T, b::T) where T = MyFoo(a,b)

julia> MyFoo{Real}(1 ,2)
MyFoo{Int64}(1, 2)

julia> MyFoo{Real}(1, 2.0)
ERROR: MethodError: no method matching MyFoo(::Int64, ::Float64)

Closest candidates are:
  MyFoo(::T, ::T) where T
   @ Main REPL[1]:4

Stacktrace:
 [1] MyFoo{Real}(a::Int64, b::Float64)
   @ Main ./REPL[2]:1
 [2] top-level scope
   @ REPL[4]:1

That way, the constructor/API doesn't change, but we eliminate the case where the type parameter is specified to be different than the actual types of the passed in objects. Whether the MethodError is better than an explicit error message is debatable (though I'm inclined to prefer it, since it doesn't require reasoning about concreteness of types).

[timholy]

julia> MyFoo{Real}(1 ,2)
MyFoo{Int64}(1, 2)

That is horrifying and a big no-no. If I ask for a MyFoo{Real} you need to give me one or throw an error.

[Seelengrab]

Then I guess the isconcretetype query has to stay, if we want to restrict the type to that extent.

[vtjnash]

The more Julia-style way to write that query is probably that typeof(a)===typeof(b)

[Seelengrab]

That won't prevent TwicePrecision{Real}(1,2) though - for that you'd need T === typeof(a) === typeof(b), which is the same as isconcretetype if I'm not mistaken.

[vtjnash]

It is fairly abnormal to check the type parameter in that too

[Seelengrab]

This is not a good way to communicate these requirements - it doesn't tell a user anything about how they might fix the issue if they encounter the error message. Something like

throw(ArgumentError("Cannot construct a `TwicePrecision{<:Integer}` - only floating point types and quantities of them are supported."))

is more appropriate, though it does feel a bit weird to call out "quantities", since we don't really have any like that in Base itself... Maybe that's an argument that Unitful.jl should not rely on TwicePrecision - which would be good, since that would let us require <: AbstractFloat in the type itself, saving us from the manual error checking. I dread the amount of code churn that could cause...

[Seelengrab]

What kinds of errors are caught by this? Can they be reached by using the surface level API for ranges? Can this be tested for?

While I understand why this is generally a good idea when viewing TwicePrecision as an implementation of DoubleWords (which it isn't right now, even if it makes use of that sort of arithmetic to provide accurate ranges), I struggle to see how this fits into the larger context of TwicePrecision being used to provide accurate floating point ranges. No code from Base regarding integers should even touch this (as hopefully evidenced by no tests breaking due to this change, assuming the code in question is tested).

EDIT: Thinking on this some more, adding these sorts of errors is a good thing. While it's technically not necessary to do that in Base, since everything about this is highly internal, a quick juliahub search shows that at least 3 packages just access the type and use it for their code. The uses don't seem to touch non-floating point types, but enforcing the invariants on a type level to catch future misuse is good, if only to document for future readers what the context of the type is.

[nsajko]

when viewing TwicePrecision as an implementation of DoubleWords (which it isn't right now

There's no point in arguing about semantics, but it's definitely the case that this disallows some types that don't make sense semantically or algorithmically. It's simply a bug to construct those types.

No code from Base regarding integers should even touch this (as hopefully evidenced by no tests breaking due to this change, assuming the code in question is tested).

The second paragraph of the commit message addresses this, except I typoed BigInt into BigFloat.

[nsajko]

except I typoed BigInt into BigFloat.

Actually, I'm not sure whether it was one or the other, but I definitely remember an error being triggered during the ranges test suite if this PR is cherry-picked to the Git history before ce2c3ae.

[Seelengrab]

The second paragraph of the commit message addresses this, except I typoed BigInt into BigFloat.

Actually, I'm not sure whether it was one or the other, but I definitely remember an error being triggered during the ranges test suite

Would be good to have a test for these cases that are intended to error, with @test_throws.

[LilithHafner]

I'm mildly opposed to adding explicit code paths to catch potential misuse of internals, especially when that potential misuse has not been repeatedly demonstrated.