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

deprecate @pure and make it just alias to @assume_effects :foldable #48682

Merged
merged 1 commit into from
Feb 16, 2023

Conversation

aviatesk
Copy link
Sponsor Member

This PR deprecates the @pure macro and removes all the internals that relate to the macro. It completes and replaces #48588.

@aviatesk aviatesk force-pushed the avi/remove-pure-take2 branch 2 times, most recently from 6f4909d to c285a8b Compare February 15, 2023 08:04
@giordano giordano added deprecation This change introduces or involves a deprecation compiler:effects effect analysis labels Feb 15, 2023
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 15, 2023

Please try to preserve and add Co-authored-by tags

@aviatesk aviatesk force-pushed the avi/remove-pure-take2 branch 2 times, most recently from 3abc31e to 3b1ba4d Compare February 15, 2023 17:20
@aviatesk
Copy link
Sponsor Member Author

aviatesk commented Feb 15, 2023

Whoops, I missed that I lost the tags during rebase. Thanks.

@aviatesk
Copy link
Sponsor Member Author

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk
Copy link
Sponsor Member Author

No regressions. This should be ready to go.

Co-authored-by: Oscar Smith <oscardssmith@users.noreply.github.com>
Co-authored-by: Valentin Churavy <vchuravy@users.noreply.github.com>
@aviatesk aviatesk merged commit 2ce9060 into master Feb 16, 2023
@aviatesk aviatesk deleted the avi/remove-pure-take2 branch February 16, 2023 16:14
@KristofferC
Copy link
Sponsor Member

-1 for the hard deprecation here. It is an internal that changes to another internal. Just put it in deprecated.jl without a deprecation and be done with it. No need to spam gazillions of warnings to normal users.

@oscardssmith
Copy link
Member

oscardssmith commented Feb 16, 2023

the counterpoint is that 90% of uses of @pure were broken anyway. We added the deprecation early enough in the release that I'm hoping the result is that all the advanced people who run on master go around and fix all of the broken uses of @pure

@KristofferC
Copy link
Sponsor Member

How is that a counter point? Whose life is better now when we get a bazillion warnings from precompile for any package install?

I'm hoping the result is that all the advanced people who run on master go around and fix all of the broken uses of @pure

No one is stopping anyone from doing that. Just don't spam my terminal with pages of warnings.

@maleadt
Copy link
Member

maleadt commented Feb 18, 2023

This also broke Compat.jl 3.x, which quite a few packages still use.
https://github.com/JuliaLang/Compat.jl/blob/v3.46.0/src/Compat.jl#L282
IMO removals like this should have been preceded by a PkgEval run, and fixes to packages.

@martinholters
Copy link
Member

Would that be solved by making the if in Compat a @static if? We could do that in a patch release of the v3 branch, I guess.

@maleadt
Copy link
Member

maleadt commented Feb 18, 2023

Yeah, I think that should work.

@StefanKarpinski
Copy link
Sponsor Member

With the new definition, isn't it more likely that uses of @pure are now correct?

@oscardssmith
Copy link
Member

They would be, but most uses of @pure were straight up broken. Eg @pure f() = error(). Almost all uses of @pure were pre 1.0 and are neither correct nor needed.

@aviatesk
Copy link
Sponsor Member Author

Base.@assume_effects :foldable f() = error() is valid :p I don't know much about @pure usages in the wild, but I guess they are not all correct even given the new definition.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:effects effect analysis deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants