irrationals: restrict assume effects annotations to known types

[nsajko]

Other changes:

• replace :total with the less powerful :foldable
• add an <:Integer dispatch constraint on the rationalize method, closes fix dispatch constraint on arg on rationalize for AbstractIrrational #55872
• replace Rational{<:Integer} with just Rational, they're equal

Other issues, related to BigFloat precision, are still present in irrationals.jl, to be fixed by followup PRs, including #55853.

Fixes #55874

[nsajko]

bump

[devmotion]

Could this be added to the @irrational macro to avoid hard to debug surprises when adding more @irrationals in Base (users and packages outside of Base are not supposed to define Irrationals and use @irrational anyway, so it's mainly about more @irrationals in Base)?

[nsajko]

users and packages outside of Base are not supposed to define Irrationals

Perhaps this was the intent, but there's nothing stopping users from doing this. Wrong effects may have catastrophic consequences like crashes or silently wrong results, or other hard-to-debug issues (undefined behavior, in short), so they're extremely undesirable.

[devmotion]

Sure. My only point was that maybe my suggestion is not worth it since it is supposed to be only relevant for uses in Base anyway. But even then it might be nice not to have to remember to update the union if a new @irrational is added.

[devmotion]

Perhaps this was the intent

This intent is documented (but of course that doesn't prevent definitions outside of Base):

  │ Warning
  │
  │  This macro should not be used outside of Base Julia.
  │
  │  The macro creates a new type Irrational{:sym} regardless of where
  │  it's invoked. This can lead to conflicting definitions if two
  │  packages define an irrational number with the same name but
  │  different values.

[devmotion]

#47103

[nsajko]

The macro is irrelevant to the question of safety here. As shown in the fixed issue, it's not required to add new subtypes of Irrational.

[nsajko]

it might be nice not to have to remember to update the union

I think there's no other way to achieve safety here.

[devmotion]

OK, I think there's a misunderstanding here, I'll try to clarify my initial comment:

1. The custom union only contains constants that are defined with the @irrational macro.
2. Instead of defining a custom union, it seems these definitions could have been added to

   julia/base/irrationals.jl

   Lines 247 to 257 in cb757c4

   	quote
   	const $esym = Irrational{$qsym}()
   	$bigconvert
   	let v = $val, v64 = Float64(v), v32 = Float32(v)
   	Base.Float64(::Irrational{$qsym}) = v64
   	Base.Float32(::Irrational{$qsym}) = v32
   	end
   	@assert isa(big($esym), BigFloat)
   	@assert Float64($esym) == Float64(big($esym))
   	@assert Float32($esym) == Float32(big($esym))
   	end

   .

This wouldn't reduce the amount of code here but would ensure that for Irrational types defined with the @irrational macro (with known value etc.) these effects would be defined automatically. So if at some point an @irrational in Base is added, removed, or modified, it is not necessary to update the union.

[nsajko]

Oh, you mean to add the methods with effect annotations using the macro instead of like in this PR. Yeah, I guess that would be correct, but it'd also unfortunately increase the number of methods.

[devmotion]

I was wondering whether the increased number of methods would be problematic. My gut feeling was that it might be fine given that it's only five Irrationals currently but on the other hand I don't know the compiler internals and why/when this could be problematic (or, on the other hand, if the compiler has problems with Unions if they contain too many elements).

[sostock]

This should be Base.rationalize. Right now it creates a new function Base.MathConstants.rationalize. Consequently, Base.rationalize(Int, π) isn’t constant-folded in Julia 1.10 and 1.11:

julia> @btime rationalize(Int,π);
  1.837 ns (0 allocations: 0 bytes)      # v1.9: constant-folded
  88.416 μs (412 allocations: 15.00 KiB) # v1.10: not constant-folded

[KristofferC]

A comment easily gets lost so I think it is preferable to create a new issue where it can be given a milestone for example.

[sostock]

I opened a PR: #56793