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

Support @deprecateing qualified function names #44394

Merged
merged 5 commits into from
May 27, 2022

Conversation

tkf
Copy link
Member

@tkf tkf commented Mar 1, 2022

This PR adds support for code like this

@deprecate Threads.Atomic() Threads.Atomic{Int}() false

#44388 needs to go first

@tkf tkf mentioned this pull request Mar 1, 2022
3 tasks
@tkf
Copy link
Member Author

tkf commented Mar 2, 2022

@fredrikekre Since you just reviewed #44388 (thanks!) and already tweaked this part of the code, do you mind reviewing this as well?

@fredrikekre
Copy link
Member

Is this just so you don't have to put the macro call in the Threads module? I guess it will only work if the name still exist in the module too?

module A
end
bar() = 1
@deprecate A.foo bar

@tkf
Copy link
Member Author

tkf commented Mar 2, 2022

Yeah, the callable object should already exist in that namespace. It'd fail otherwise.

One difference to the version before this PR is that we can emit the message with the name that would be more likely to be used in practice. For example, with this PR, I get

julia> @deprecate Threads.Atomic() Threads.Atomic{Int}() false

julia> Threads.Atomic()
┌ Warning: `Threads.Atomic()` is deprecated, use `Threads.Atomic{Int}()` instead.
│   caller = ip:0x0
└ @ Core :-1

while before this PR it would be

julia> import Base.Threads: Atomic
       @deprecate Atomic() Atomic{Int}() false;

julia> Threads.Atomic()
┌ Warning: `Atomic()` is deprecated, use `Atomic{Int}()` instead.
│   caller = ip:0x0
└ @ Core :-1

Notice that the warning message with this PR shows Threads.Atomic() while it is printed as Atomic() before this PR.

@fredrikekre
Copy link
Member

Okay, but if you would have put it in the correct module it would work even if the name didn't exist. It doesn't seem too much hassle to put deprecations in the module they belong? I don't have any strong feelings about the PR, just wondering whether it is useful enough since it will only work in pretty specific cases.

One difference to the version before this PR is that we can emit the message with the name that would be more likely to be used in practice.

But that already works (won't print the module on the old usage, but it displays what you should update too at least):

julia> @eval Base.Threads @deprecate Atomic() Threads.Atomic{Int}() false;

julia> Threads.Atomic()
┌ Warning: `Atomic()` is deprecated, use `Threads.Atomic{Int}()` instead.

@tkf
Copy link
Member Author

tkf commented Mar 2, 2022

I think it makes sense to be cautious if this PR were adding code to achieve what I wanted it for. But it actually just simply removes an artificial restriction that the function names have to be a symbol. For example, this PR also lets us use something like @deprecate (@f)() g() false (which could be useful for hiding the actual function name and/or auto-generating it).

@tkf tkf changed the title Support @deprecateing functions in other modules Support @deprecateing qualified function names Mar 2, 2022
@fredrikekre
Copy link
Member

Okay, that makes sense I guess. Does

@deprecate A.foo A.bar

also work? I didn't see any changes wrt that. Could probably be included in the first code path (if isa(old, Symbol)).

@tkf
Copy link
Member Author

tkf commented Mar 3, 2022

I just pass through all non-call expressions ATM. But it probably doesn't do what we want it to do

julia> struct Foo{T}
           value::T
       end

julia> (Foo{T} where {T <: Int})(x) = x  # this happens with `@deprecate Foo{T} where {T <: Int} ...`

julia> Foo{Int}(0)
Foo{Int64}(0)

julia> (Foo{T}(x) where {T <: Int}) = x

julia> Foo{Int}(0)
0

Maybe we need to restrict the non-call form to symbols and getfields?

@tkf
Copy link
Member Author

tkf commented Mar 4, 2022

Actually, the scenario I worried about was already detected in the current code. I just added a test to check it.

It's good to go on my end. Are there any other concerns?

@KristofferC KristofferC added the merge me PR is reviewed. Merge when all tests are passing label May 17, 2022
@vtjnash vtjnash merged commit ec7a39e into JuliaLang:master May 27, 2022
@giordano giordano removed the merge me PR is reviewed. Merge when all tests are passing label Jun 10, 2022
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.

5 participants