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

Assume effects #783

Merged
merged 8 commits into from
Nov 18, 2022
Merged

Assume effects #783

merged 8 commits into from
Nov 18, 2022

Conversation

Tokazama
Copy link
Contributor

@Tokazama Tokazama commented Nov 1, 2022

Fixes #769

This allows packages with a 1.6 compat to replace Base.@pure with Base.@assume_effects :total similar to what's been done in base. All other effects are ignored

@Keno
Copy link
Member

Keno commented Nov 1, 2022

@pure is stronger than @assume_effects. I don't think this is a good idea.

@codecov
Copy link

codecov bot commented Nov 1, 2022

Codecov Report

Merging #783 (b525933) into master (6e1b40c) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #783      +/-   ##
==========================================
+ Coverage   91.63%   91.69%   +0.06%     
==========================================
  Files           2        2              
  Lines         251      253       +2     
==========================================
+ Hits          230      232       +2     
  Misses         21       21              
Impacted Files Coverage Δ
src/Compat.jl 92.24% <100.00%> (+0.06%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 1, 2022

@pure is stronger than @assume_effects. I don't think this is a good idea.

Sorry, I thought this was how all uses of Base.@pure were managed in Base. What's the appropriate way to transition code that uses Base.@pure?

@Keno
Copy link
Member

Keno commented Nov 1, 2022

It should be generally safe to replace calls to @pure, by @assume_effects :total, but not the other way around. The issue is that with this, you'll write @assume_effects :total, but get @pure semantics on older versions of julia, which is not the same.

@Keno
Copy link
Member

Keno commented Nov 1, 2022

More generally, though, packages should revisit every use of @pure and select the minimum set of effect assumptions that they need (preferably none).

`@pure` => `@assume_effects :total` but `@assume_effects :total` =/=> `@pure` so just test
macro hygiene
@Tokazama
Copy link
Contributor Author

Tokazama commented Nov 1, 2022

I removed the mapping to @pure so there aren't any added assumptions on 1.6.
I guess there could be an extended help section for Base.@pure detailing how to make the transition but a quick search on JuliaHub reveals that most packages that still use it are maintained by one particular individual. If I recall correctly, previous attempts to dissuade their use of @pure were met with quite a bit of resistance.

src/Compat.jl Outdated Show resolved Hide resolved
@martinholters
Copy link
Member

Needs a README entry.

Tokazama and others added 2 commits November 8, 2022 07:52
src/Compat.jl Outdated Show resolved Hide resolved
Tokazama and others added 2 commits November 8, 2022 08:18
Copy link
Member

@martinholters martinholters left a comment

Choose a reason for hiding this comment

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

LGTM, but we should get #784 in first and then test on top of that to verify it's working as intended on latest Julia.

That said, I wonder how helpful this is given it cannot be used as a replacement for @pure without regressions on older Julia. Still better than nothing, though.

@Keno any more thoughts on this?

@martinholters
Copy link
Member

Can you rebase on (or merge) current master to (hopefully) fix the test failures on nightly?

@martinholters martinholters merged commit abce98e into JuliaLang:master Nov 18, 2022
@Tokazama Tokazama deleted the assume_effects branch November 18, 2022 11:03
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.

compat for @assume_effects
3 participants