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

effect overrides: Add notaskstate & refactor slightly #45448

Merged
merged 2 commits into from
May 27, 2022

Conversation

Keno
Copy link
Member

@Keno Keno commented May 25, 2022

This adds an effect override for :notaskstate and shifts the
:total, meta-effect around slightly to prepare for any potential
future expansion. In particular :total now means the maximum
possible set of effects and should likely include any future additions.
The assume_effects macro learned to do negation, so we can now write
things like :total :!nothrow, which will be helpful writing tests
that care about one effect in particular, but are otherwise total.

The previous :total_may_throw, was renamed :foldable and includes
the effects required to allow compile-time constant folding. At this
point I don't anticipate the introduction of additional effects that
would affect constant-foldability, but if such an effect were introduced,
it would be added to :foldable. Note however, that :foldable
does not include :notaskstate (though as noted in the docstring,
because of the strong requirements of :consistent and :effect_free,
it is implied that anything annotated :notaskstate may be DCEd
and thus :notaskstate is implied). Nevertheless, :notaskstate is
not included in the :foldable override and future effect additions
may further separate :foldable from :total.

@Keno Keno requested a review from aviatesk May 25, 2022 04:56
Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

The :total_may_throw setting is backported to v1.8 and so this can be breaking if we don't backport it.

base/expr.jl Outdated
Comment on lines 619 to 632
val = true
org_setting = setting
while true
if isa(setting, Expr) && setting.head === :call
if setting.args[1] == :(!)
val = !val
setting = setting.args[2]
end
elseif isa(setting, QuoteNode) && isa(setting.value, Symbol)
setting = setting.value
else
break
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
val = true
org_setting = setting
while true
if isa(setting, Expr) && setting.head === :call
if setting.args[1] == :(!)
val = !val
setting = setting.args[2]
end
elseif isa(setting, QuoteNode) && isa(setting.value, Symbol)
setting = setting.value
else
break
end
end
org_setting = setting
setting, val = compute_setting(setting)

with

function compute_setting(@nospecialize(setting), val::Bool=true)
    if isexpr(setting, :call) && settings.args[1] === :(!)
        return compute_setting(setting.args[2], !val)
    elseif isa(setting, QuoteNode)
        return compute_setting(setting.value, val)
    else
        return (setting, val)
    end
end

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure. I'm assuming nobody will build exprs big enough that this bites us :)

base/expr.jl Outdated Show resolved Hide resolved
base/expr.jl Outdated Show resolved Hide resolved
@Keno
Copy link
Member Author

Keno commented May 25, 2022

The :total_may_throw setting is backported to v1.8 and so this can be breaking if we don't backport it.

I'll put up a PR to rename it to :foldable.

@Keno
Copy link
Member Author

Keno commented May 25, 2022

Hmm, I don't see it on 1.8 (https://github.com/JuliaLang/julia/blob/release-1.8/base/expr.jl#L538-L539). Is it pending in backport still?

@Keno Keno force-pushed the kf/notaskstateoverride branch from 7d85fe1 to 3ca623b Compare May 25, 2022 20:56
@aviatesk
Copy link
Member

Hmm, I don't see it on 1.8 (https://github.com/JuliaLang/julia/blob/release-1.8/base/expr.jl#L538-L539). Is it pending in backport still?

It is still on the backport branch:

elseif setting === :total_may_throw

This adds an effect override for `:notaskstate` and shifts the
`:total`, meta-effect around slightly to prepare for any potential
future expansion. In particular `:total` now means the maximum
possible set of effects and should likely include any future additions.
The assume_effects macro learned to do negation, so we can now write
things like `:total :!nothrow`, which will be helpful writing tests
that care about one effect in particular, but are otherwise total.

The previous `:total_may_throw`, was renamed `:foldable` and includes
the effects required to allow compile-time constant folding. At this
point I don't anticipate the introduction of additional effects that
would affect constant-foldability, but if such an effect were introduced,
it would be added to `:foldable`. Note however, that `:foldable`
does not include `:notaskstate` (though as noted in the docstring,
because of the strong requirements of `:consistent` and `:effect_free`,
it is implied that anything annotated `:notaskstate` may be DCEd
and thus `:notaskstate` is implied). Nevertheless, `:notaskstate` is
not included in the `:foldable` override and future effect additions
may further separate `:foldable` from `:total`.
@Keno Keno force-pushed the kf/notaskstateoverride branch from 3ca623b to e0fc83f Compare May 26, 2022 02:56
@Keno
Copy link
Member Author

Keno commented May 26, 2022

It is still on the backport branch:

Ok, then no packages could have possibly started using it, so we can just rename it to :foldable on the release branch.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

LGTM.

@Keno Keno merged commit 8e30135 into master May 27, 2022
@Keno Keno deleted the kf/notaskstateoverride branch May 27, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants