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

Fix @deprecate with the new where syntax #22034

Merged
merged 2 commits into from
May 25, 2017

Conversation

fredrikekre
Copy link
Member

@fredrikekre fredrikekre commented May 23, 2017

Copy link
Sponsor Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Presumably you've tested this with an actual deprecation using the where syntax and it works? I would suggest adding a test, but we'll start using this as soon as it's merged, which will test it effectively enough.

@fredrikekre
Copy link
Member Author

Actually, it does not quite work for outer constructors (which I forgot about and posted #22033 ). I have an update for that incoming.

@fredrikekre
Copy link
Member Author

Okay, so this now enables signatures with where:

bar(x) = x;
foo(x::T) where T = 1;
@deprecate foo(x::T) where T bar(x);

julia> foo(1)
WARNING: foo(x::T) where T is deprecated, use bar(x) instead.
[...]

and outer constructors:

struct A{T} end
A{T}(x::S) where {T, S} = 1;
@deprecate A{T}(x::S) where {T, S} A{S}();

julia> A{Int}(1.)
WARNING: A{T}(x::S) where {T, S} is deprecated, use A{S}() instead.
[...]

I might as well add some tests, in what file should they go? test/misc.jl seems to be the best match?

@fredrikekre
Copy link
Member Author

On another note, which confused me while fixing this:

julia> f2(x) = 2; f4(x) = 4;

julia> @deprecate f1(x) f2(x);

julia> @deprecate f3(x) f4(x);

julia> f1(1)
WARNING: f1(x) is deprecated, use f2(x) instead.
Stacktrace:
 [1] depwarn(::String, ::Symbol) at ./deprecated.jl:64
 [2] f1(::Int64) at ./deprecated.jl:51
 [3] eval(::Module, ::Any) at ./boot.jl:235
 [4] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./REPL.jl:66
 [5] macro expansion at ./REPL.jl:97 [inlined]
 [6] (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:73
while loading no file, in expression starting on line 0
2

julia> f3(1) # why does this not warn?
4

The call to f3 here should warn? Anyhow, the behaviour is the same pre and post this PR...

@fredrikekre
Copy link
Member Author

I disabled the tests, such that #22043 does not block this PR. The tests pass locally one by one.

@kshyatt kshyatt added deprecation This change introduces or involves a deprecation types and dispatch Types, subtyping and method dispatch labels May 25, 2017
@KristofferC KristofferC merged commit 215adb2 into JuliaLang:master May 25, 2017
@tkelman
Copy link
Contributor

tkelman commented May 25, 2017

Can't these be @test_broken ?

@fredrikekre
Copy link
Member Author

There is not @test_warn_broken though. On another note, when these are activated we should make sure that Base.JLOptions().depwarn is turned on before running that testset.

@fredrikekre fredrikekre deleted the fe/deprecate-where branch May 25, 2017 11:39
@tkelman
Copy link
Contributor

tkelman commented May 25, 2017

tests for deprecations should probably be done in a separate process where we can be sure to run it with each possible value of the depwarn flag and check that the behavior is right for each value

tkelman pushed a commit that referenced this pull request Jun 3, 2017
* fix at-deprecate with where syntax

* tests for at-deprecate

(cherry picked from commit 215adb2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation This change introduces or involves a deprecation types and dispatch Types, subtyping and method dispatch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants