-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Comments
After looking at the blame, it seems that those |
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
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 |
I think this could at least partly be addressed by using |
There are four methods with
@assume_effects :total
applied to the entire method inbase/irrationals.jl
:julia/base/irrationals.jl
Line 55 in 6e5e87b
julia/base/irrationals.jl
Lines 70 to 71 in 6e5e87b
julia/base/irrationals.jl
Lines 113 to 120 in 6e5e87b
Issues:
:total
effect is too powerful and not future-proof. I guess what's actually desired is just:foldable
.@assume_effects
to the::AbstractIrrational
methods, because users may define custom subtypes ofAbstractIrrational
, and thus calls likeBigFloat(::AbstractIrrational)
orbig(::AbstractIrrational)
may execute arbitrary code. That said ...@assume_effects
even just for::Irrational
, because users may define custom subtypes ofIrrational
! Example: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 onAbstractIrrational
or onIrrational
: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.The text was updated successfully, but these errors were encountered: