Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

irrationals: restrict assume effects annotations to known types #55886

Merged
merged 3 commits into from
Oct 10, 2024
+34 −8
Merged

irrationals: restrict assume effects annotations to known types #55886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc58d23
irrationals: restrict assume effects annotations to known types
nsajko Sep 26, 2024
5dd2a85
qualify `lessrational` in MathConstants
nsajko Sep 26, 2024
8bc317c
Merge branch 'master' into known_irrational
nsajko Oct 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 14 additions & 8 deletions base/irrationals.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ AbstractFloat(x::AbstractIrrational) = Float64(x)::Float64
Float16(x::AbstractIrrational) = Float16(Float32(x)::Float32)
Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x))

# XXX this may change `DEFAULT_PRECISION`, thus not effect free
@assume_effects :total function Rational{T}(x::AbstractIrrational) where T<:Integer
function _irrational_to_rational(::Type{T}, x::AbstractIrrational) where T<:Integer
o = precision(BigFloat)
p = 256
while true
Expand All @@ -66,13 +65,16 @@ Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x))
p += 32
end
end
Rational{BigInt}(x::AbstractIrrational) = throw(ArgumentError("Cannot convert an AbstractIrrational to a Rational{BigInt}: use rationalize(BigInt, x) instead"))
Rational{T}(x::AbstractIrrational) where {T<:Integer} = _irrational_to_rational(T, x)
_throw_argument_error_irrational_to_rational_bigint() = throw(ArgumentError("Cannot convert an AbstractIrrational to a Rational{BigInt}: use rationalize(BigInt, x) instead"))
Rational{BigInt}(::AbstractIrrational) = _throw_argument_error_irrational_to_rational_bigint()

@assume_effects :total function (t::Type{T})(x::AbstractIrrational, r::RoundingMode) where T<:Union{Float32,Float64}
function _irrational_to_float(::Type{T}, x::AbstractIrrational, r::RoundingMode) where T<:Union{Float32,Float64}
setprecision(BigFloat, 256) do
T(BigFloat(x)::BigFloat, r)
end
end
(::Type{T})(x::AbstractIrrational, r::RoundingMode) where {T<:Union{Float32,Float64}} = _irrational_to_float(T, x, r)

float(::Type{<:AbstractIrrational}) = Float64

Expand Down Expand Up @@ -110,14 +112,18 @@ end
<=(x::AbstractFloat, y::AbstractIrrational) = x < y

# Irrational vs Rational
@assume_effects :total function rationalize(::Type{T}, x::AbstractIrrational; tol::Real=0) where T
function _rationalize_irrational(::Type{T}, x::AbstractIrrational, tol::Real) where {T<:Integer}
return rationalize(T, big(x), tol=tol)
end
@assume_effects :total function lessrational(rx::Rational{<:Integer}, x::AbstractIrrational)
# an @assume_effects :total version of `<` for determining if the rationalization of
# an irrational number required rounding up or down
function rationalize(::Type{T}, x::AbstractIrrational; tol::Real=0) where {T<:Integer}
return _rationalize_irrational(T, x, tol)
end
function _lessrational(rx::Rational, x::AbstractIrrational)
return rx < big(x)
end
function lessrational(rx::Rational, x::AbstractIrrational)
return _lessrational(rx, x)
end
function <(x::AbstractIrrational, y::Rational{T}) where T
T <: Unsigned && x < 0.0 && return true
rx = rationalize(T, x)
Expand Down
20 changes: 20 additions & 0 deletions base/mathconstants.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,26 @@ Base.@irrational γ euler
Base.@irrational φ (1+sqrt(big(5)))/2
Base.@irrational catalan catalan

const _KnownIrrational = Union{
typeof(π), typeof(ℯ), typeof(γ), typeof(φ), typeof(catalan)
}

function Rational{BigInt}(::_KnownIrrational)
Base._throw_argument_error_irrational_to_rational_bigint()
end
Base.@assume_effects :foldable function Rational{T}(x::_KnownIrrational) where {T<:Integer}
Base._irrational_to_rational(T, x)
end
Base.@assume_effects :foldable function (::Type{T})(x::_KnownIrrational, r::RoundingMode) where {T<:Union{Float32,Float64}}
Base._irrational_to_float(T, x, r)
end
Base.@assume_effects :foldable function rationalize(::Type{T}, x::_KnownIrrational; tol::Real=0) where {T<:Integer}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened a PR: #56793

Base._rationalize_irrational(T, x, tol)
end
Base.@assume_effects :foldable function Base.lessrational(rx::Rational, x::_KnownIrrational)
Base._lessrational(rx, x)
end
Comment on lines +19 to +37
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#47103

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


# aliases
"""
π
Expand Down
Loading