Rename pure to unsafe_pure and remove unnecessary/improper uses #27432

helgee · 2018-06-05T06:25:16Z

See dicussion here https://discourse.julialang.org/t/bug-in-doc-system-and-question-on-pure-in-docs-meaning-julia-1-0-is-close/11292

This should send a clear signal to beginners that this macro should be handled with care.

Since it is not exported, there is no deprecation and NEWS entry needed, right?

KristofferC · 2018-06-05T06:50:29Z

It would be kind to give a deprecation anyway since we know this is frequently used outside Base.

helgee · 2018-06-06T10:30:11Z

Added the deprecation. CI is super unhappy but I do not take responsibility for this 😜

FWIW: I experienced the same hangs after UUIDs as seen on AV locally on macOS.

vtjnash

It's nice to see that we're so close to deprecating @pure! But I see you missed the alternate spelling (@_pure_meta) used during bootstrap – should we make these consistent?

vtjnash · 2018-06-06T14:42:21Z

base/compiler/abstractinterpretation.jl

        return Const(length(argtypes[2].val))
    elseif length(argtypes) == 3 && isa(argtypes[2], Const) && isa(argtypes[3], Const) &&
            isa(argtypes[2].val, SimpleVector) && isa(argtypes[3].val, Int) && istopfunction(tm, f, :getindex)
-        # mark getindex(::SimpleVector, i::Int) as @pure
+        # mark getindex(::SimpleVector, i::Int) as @unsafe_pure


These two should probably say "as pure", since it's only indirectly connected to the @pure macro

vtjnash · 2018-06-06T14:43:58Z

doc/src/manual/types.md

@@ -1374,7 +1374,7 @@ for cases where you don't need a more elaborate hierarchy.
 julia> struct Val{x}
       end

-julia> Base.@pure Val(x) = Val{x}()
+julia> Base.@unsafe_pure Val(x) = Val{x}()


The @unsafe_pure annotation should be unnecessary (@inline should achieve the same results, but should also be unnecessary since this function is small)

Removing the annotation has the added benefit of getting @[unsafe_]pure out of the manual.

vtjnash · 2018-06-06T14:44:59Z

stdlib/LinearAlgebra/src/deprecated.jl

@@ -403,7 +403,7 @@ _RowVector_depstring() = string("`ConjRowVector` and `RowVector` have been depre
        "of the associated changes.")

 @inline check_types(::Type{T1}, ::AbstractVector{T2}) where {T1,T2} = check_types(T1, T2)
-@pure check_types(::Type{T1}, ::Type{T2}) where {T1,T2} = T1 === transpose_type(T2) ? nothing :
+@unsafe_pure check_types(::Type{T1}, ::Type{T2}) where {T1,T2} = T1 === transpose_type(T2) ? nothing :
    error("Element type mismatch. Tried to create a `RowVector{$T1}` from an `AbstractVector{$T2}`")


This isn't safe (or necessary) to mark as @pure (the transpose_type function may break the compiler if used from a function marked @pure)

vtjnash · 2018-06-06T14:51:46Z

base/broadcast.jl

@@ -135,7 +135,7 @@ BroadcastStyle(a::AbstractArrayStyle, ::Style{Tuple})    = a
 BroadcastStyle(::A, ::A) where A<:ArrayStyle             = A()
 BroadcastStyle(::ArrayStyle, ::ArrayStyle)               = Unknown()
 BroadcastStyle(::A, ::A) where A<:AbstractArrayStyle     = A()
-Base.@pure function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
+Base.@unsafe_pure function BroadcastStyle(a::A, b::B) where {A<:AbstractArrayStyle{M},B<:AbstractArrayStyle{N}} where {M,N}
    if Base.typename(A).wrapper == Base.typename(B).wrapper


This doesn't need to be marked unsafe_pure. Indeed, the inferable version is shorter:

if Base.typename(A) === Base.typename(B) return A(Val(max(M, N))) end return Unknown()

(the Val is a side-effect of likely-unnecessary complications in the constructor API above, and could also be deleted with a bit more work)

helgee · 2018-06-08T07:29:40Z

I also renamed the bootstrap version to keep it consistent and to give @vtjnash a convenient possibility to audit the remaining uses of @unsafe_pure/@_unsafe_pure_meta in Base 😉

vtjnash · 2018-07-19T18:57:13Z

base/deprecated.jl

@@ -1724,6 +1724,9 @@ end

 @deprecate atan2(y, x) atan(y, x)

+# PR #27432
+@eval @deprecate $(Symbol("@pure")) $(Symbol("@unsafe_pure")) false


I think we can keep @pure as an (non-deprecated) alias for @unsafe_pure too, to avoid breaking the package ecosystem. In the future, we'll weaken the meaning of @pure (probably to something like speculatable), but preferably, most package uses should be able to continue using @pure though that transition. That'll also let me audit Base – as you conveniently offered :) – to indicate which expect to still need the unsafe version.

mbauman · 2018-07-26T18:35:18Z

Thanks so much for the PR! I really appreciate the time and effort you put into this — and the discussion that this prompted moved the discussion forward. Jameson's comment (#27432 (comment)) is a great thought. Triage is in support of making Base.@pure do the safer thing folks expect, and then adding a new unsafe version is a great path forward. With all the new constant propagation and optimizer and IPO goodies in 0.7, the extra guarantees of the unsafe version are even more rarely needed.

helgee · 2018-07-26T19:51:31Z

How do I define an alias for a macro? Like this?

# Alias `foo` for `bar`
macro foo(expr)
    :(@bar $expr)
end

fredrikekre · 2018-07-26T19:54:55Z

@eval const $(Symbol("@foo")) = $(Symbol("@bar"))

helgee · 2018-07-26T20:27:00Z

I removed the deprecation and added the alias, as suggested. Should I also modify the recently added docstring for @pure, i.e. change it to @unsafe_pure?

helgee · 2018-07-29T13:16:20Z

Bump. Anything left to do?

helgee · 2018-07-31T18:44:56Z

I am not sure I understood the conclusion when this should get in. These changes are not really essential for 0.7/1.0 except for the docs change that removes @pure from the manual.

I could also open a separate PR which only changes the manual.

(OT: I suck at rebasing 😜)

StefanKarpinski · 2018-07-31T19:52:47Z

Jameson, can you comment on what should be done here for now?

helgee · 2018-08-07T10:51:21Z

I have opened a separate PR for removing @pure from the manual: #28496

KristofferC · 2022-05-10T12:00:07Z

#40097 is an updated version of this and it seems unlikely that we will go through the churn of renaming this.

helgee force-pushed the unsafe-pure branch from 29a8871 to 848baf6 Compare June 6, 2018 06:38

vtjnash approved these changes Jun 6, 2018

View reviewed changes

helgee changed the title ~~Rename pure to unsafe_pure~~ Rename pure to unsafe_pure and remove unnecessary/improper uses Jun 8, 2018

helgee force-pushed the unsafe-pure branch from 67d332f to 9f358a7 Compare June 12, 2018 09:12

StefanKarpinski added the triage This should be discussed on a triage call label Jul 16, 2018

vtjnash reviewed Jul 19, 2018

View reviewed changes

StefanKarpinski removed the triage This should be discussed on a triage call label Jul 26, 2018

helgee force-pushed the unsafe-pure branch from eff0ac7 to 3170a15 Compare July 31, 2018 14:11

vtjnash approved these changes Jul 31, 2018

View reviewed changes

helgee mentioned this pull request Aug 7, 2018

Remove at-pure from manual #28496

Merged

helgee added 7 commits August 7, 2018 18:06

Rename at-pure to at-unsafe_pure

47bca2e

Rename bootstrap version of unsafe_pure

cc0caaf

Remove unsafe_pure from Val

85887f7

Another at-pure had snuck in

d286cc8

Add alias and fix broken rebase

948f3d7

Update LinearAlgebra.jl

3a04e71

Update LinearAlgebra.jl

2255480

helgee force-pushed the unsafe-pure branch from a6e253a to 2255480 Compare August 7, 2018 17:14

helgee mentioned this pull request Mar 8, 2021

Write down what we know about Base.@pure #39954

Closed

KristofferC closed this May 10, 2022

helgee deleted the unsafe-pure branch May 10, 2022 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename pure to unsafe_pure and remove unnecessary/improper uses #27432

Rename pure to unsafe_pure and remove unnecessary/improper uses #27432

helgee commented Jun 5, 2018

KristofferC commented Jun 5, 2018

helgee commented Jun 6, 2018

vtjnash left a comment

vtjnash Jun 6, 2018

vtjnash Jun 6, 2018

helgee Jun 6, 2018

vtjnash Jun 6, 2018

vtjnash Jun 6, 2018

helgee commented Jun 8, 2018 •

edited

Loading

vtjnash Jul 19, 2018

mbauman commented Jul 26, 2018 •

edited

Loading

helgee commented Jul 26, 2018

fredrikekre commented Jul 26, 2018

helgee commented Jul 26, 2018

helgee commented Jul 29, 2018

helgee commented Jul 31, 2018

StefanKarpinski commented Jul 31, 2018

helgee commented Aug 7, 2018

KristofferC commented May 10, 2022

Rename pure to unsafe_pure and remove unnecessary/improper uses #27432

Rename pure to unsafe_pure and remove unnecessary/improper uses #27432

Conversation

helgee commented Jun 5, 2018

KristofferC commented Jun 5, 2018

helgee commented Jun 6, 2018

vtjnash left a comment

Choose a reason for hiding this comment

vtjnash Jun 6, 2018

Choose a reason for hiding this comment

vtjnash Jun 6, 2018

Choose a reason for hiding this comment

helgee Jun 6, 2018

Choose a reason for hiding this comment

vtjnash Jun 6, 2018

Choose a reason for hiding this comment

vtjnash Jun 6, 2018

Choose a reason for hiding this comment

helgee commented Jun 8, 2018 • edited Loading

vtjnash Jul 19, 2018

Choose a reason for hiding this comment

mbauman commented Jul 26, 2018 • edited Loading

helgee commented Jul 26, 2018

fredrikekre commented Jul 26, 2018

helgee commented Jul 26, 2018

helgee commented Jul 29, 2018

helgee commented Jul 31, 2018

StefanKarpinski commented Jul 31, 2018

helgee commented Aug 7, 2018

KristofferC commented May 10, 2022

helgee commented Jun 8, 2018 •

edited

Loading

mbauman commented Jul 26, 2018 •

edited

Loading