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.

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

invalid usage of @assume_effects in base/irrationals.jl #55874

Open
nsajko opened this issue Sep 25, 2024 · 3 comments · May be fixed by #55886
Open

invalid usage of @assume_effects in base/irrationals.jl #55874

nsajko opened this issue Sep 25, 2024 · 3 comments · May be fixed by #55886
Assignees
Labels
kind:bug Indicates an unexpected problem or unintended behavior

Comments

@nsajko
Copy link
Contributor

nsajko commented Sep 25, 2024

There are four methods with @assume_effects :total applied to the entire method in base/irrationals.jl:

@assume_effects :total function Rational{T}(x::AbstractIrrational) where T<:Integer

@assume_effects :total function (t::Type{T})(x::AbstractIrrational, r::RoundingMode) where T<:Union{Float32,Float64}

julia/base/irrationals.jl

Lines 113 to 120 in 6e5e87b

@assume_effects :total function rationalize(::Type{T}, x::AbstractIrrational; tol::Real=0) where T
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
return rx < big(x)
end

Issues:

  • The :total effect is too powerful and not future-proof. I guess what's actually desired is just :foldable.
  • It's not valid to apply @assume_effects to the ::AbstractIrrational methods, because users may define custom subtypes of AbstractIrrational, and thus calls like BigFloat(::AbstractIrrational) or big(::AbstractIrrational) may execute arbitrary code. That said ...
  • It's not valid to apply @assume_effects even just for ::Irrational, because users may define custom subtypes of Irrational! Example:
# in a user package
struct IrrationalSymbol end
function big(::Irrational{IrrationalSymbol})
    execute_arbitrary_code()
end

This seems perfectly legal, or at least it's not type piracy.

The solution seems simple: define a Union of all known irrational constants, something like the following, then dispatch on it for methods that have @assume_effects, instead of on AbstractIrrational or on Irrational:

const _KnownIrrational = (ℯ, π, MathConstants.γ, MathConstants.catalan, MathConstants.φ)

This will ensure calls involving the known constants are foldable, and allow the unsafe @assume_effects annotations to be deleted. Will make a PR later.

@nsajko nsajko added the kind:bug Indicates an unexpected problem or unintended behavior label Sep 25, 2024
@nsajko nsajko self-assigned this Sep 25, 2024
@aviatesk
Copy link
Sponsor Member

After looking at the blame, it seems that those @assume_effects were added by me in #44776 as replacements for the previous @pure, but I agree that @assume_effects :total was too strong. It should have been replaced with :foldable, and we also need to restrict the annotation to known concrete types as suggested.

nsajko added a commit to nsajko/julia that referenced this issue Sep 26, 2024
Other changes:
* replace `:total` with the less powerful `:foldable`
* add an `<:Integer` dispatch constraint on the `rationalize` method,
  closes JuliaLang#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 JuliaLang#55853.

Fixes JuliaLang#55874
@vtjnash
Copy link
Sponsor Member

vtjnash commented Sep 26, 2024

It isn't technically valid to be foldable either (there isn't technically any valid annotations to assume here), as it references global state (the precision) and returns a new allocation (a BigNum), but at least any reduction in that set of invalid annotations seems beneficial

@nsajko
Copy link
Contributor Author

nsajko commented Sep 26, 2024

references global state (the precision)

I think this could at least partly be addressed by using setprecision(::Function, ::Type{BigFloat}, ::Int). One such PR is #55853.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants