-
-
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
replace @pure
annotations in Base with effect settings
#44776
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,9 +151,37 @@ macro isdefined(s::Symbol) | |
return Expr(:escape, Expr(:isdefined, s)) | ||
end | ||
|
||
function _is_internal(__module__) | ||
if ccall(:jl_base_relative_to, Any, (Any,), __module__)::Module === Core.Compiler || | ||
nameof(__module__) === :Base | ||
return true | ||
end | ||
return false | ||
end | ||
|
||
# can be used in place of `@pure` (supposed to be used for bootstrapping) | ||
macro _pure_meta() | ||
return Expr(:meta, :pure) | ||
return _is_internal(__module__) && Expr(:meta, :pure) | ||
end | ||
# can be used in place of `@assume_effects :total` (supposed to be used for bootstrapping) | ||
macro _total_meta() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe name this |
||
return _is_internal(__module__) && Expr(:meta, Expr(:purity, | ||
#=:consistent=#true, | ||
#=:effect_free=#true, | ||
#=:nothrow=#true, | ||
#=:terminates_globally=#true, | ||
#=:terminates_locally=#false)) | ||
end | ||
# can be used in place of `@assume_effects :total_may_throw` (supposed to be used for bootstrapping) | ||
macro _total_may_throw_meta() | ||
return _is_internal(__module__) && Expr(:meta, Expr(:purity, | ||
#=:consistent=#true, | ||
#=:effect_free=#true, | ||
#=:nothrow=#false, | ||
#=:terminates_globally=#true, | ||
#=:terminates_locally=#false)) | ||
end | ||
|
||
# another version of inlining that propagates an inbounds context | ||
macro _propagate_inbounds_meta() | ||
return Expr(:meta, :inline, :propagate_inbounds) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,8 @@ AbstractFloat(x::AbstractIrrational) = Float64(x)::Float64 | |
Float16(x::AbstractIrrational) = Float16(Float32(x)::Float32) | ||
Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x)) | ||
|
||
@pure function Rational{T}(x::AbstractIrrational) where T<:Integer | ||
# XXX this may change `DEFAULT_PRECISION`, thus not effect free | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. horribly, the return value also includes novel memory allocation and depends on the global runtime state of the DEFAULT_PRECISION |
||
@assume_effects :total function Rational{T}(x::AbstractIrrational) where T<:Integer | ||
o = precision(BigFloat) | ||
p = 256 | ||
while true | ||
|
@@ -64,7 +65,7 @@ Complex{T}(x::AbstractIrrational) where {T<:Real} = Complex{T}(T(x)) | |
end | ||
Rational{BigInt}(x::AbstractIrrational) = throw(ArgumentError("Cannot convert an AbstractIrrational to a Rational{BigInt}: use rationalize(BigInt, x) instead")) | ||
|
||
@pure function (t::Type{T})(x::AbstractIrrational, r::RoundingMode) where T<:Union{Float32,Float64} | ||
@assume_effects :total function (t::Type{T})(x::AbstractIrrational, r::RoundingMode) where T<:Union{Float32,Float64} | ||
setprecision(BigFloat, 256) do | ||
T(BigFloat(x)::BigFloat, r) | ||
end | ||
|
@@ -106,11 +107,11 @@ end | |
<=(x::AbstractFloat, y::AbstractIrrational) = x < y | ||
|
||
# Irrational vs Rational | ||
@pure function rationalize(::Type{T}, x::AbstractIrrational; tol::Real=0) where T | ||
@assume_effects :total function rationalize(::Type{T}, x::AbstractIrrational; tol::Real=0) where T | ||
return rationalize(T, big(x), tol=tol) | ||
end | ||
@pure function lessrational(rx::Rational{<:Integer}, x::AbstractIrrational) | ||
# an @pure version of `<` for determining if the rationalization of | ||
@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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we delete
_pure_meta
now?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and
@pure
?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's technically internal so we should be able to do so, but I suspect there's a decent amount of package code that uses it, so we should probably at least go through some kind of deprecation process. Or maybe just make PRs to affected packages to not use it if
VERSION
≥ v"1.9".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also be sneaky and make it a no-op and then see if people actually notice...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is anyone using it correctly? I don't think @vtjnash has added uses to packages, and as far as I understand, he is the only person allowed to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://juliahub.com/ui/Search?q=pure&type=symbols&t=macro
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now all of these internal macros are no-op when used outside of Base ;)