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

assorted subtyping tests #20627

Merged
merged 1 commit into from
Apr 9, 2021
Merged

Conversation

felixrehren
Copy link
Contributor

@felixrehren felixrehren commented Feb 16, 2017

c.f. master list #19998
tests for closed issues #2552 #8625 #8915 #11407
and #12814 #16922 #17943 #18892 (#19159 #19413) #18985 #19041

[Edit: misunderstood method overwriting as in #18892, thanks yuyichao for correction, now has a test]

@felixrehren felixrehren mentioned this pull request Feb 16, 2017
53 tasks
@tkelman
Copy link
Contributor

tkelman commented Feb 16, 2017

these should use the new syntax

@yuyichao
Copy link
Contributor

Why should it error? You defined two methods that overrides each other and both of them matches the signature you are calling.

@felixrehren
Copy link
Contributor Author

felixrehren commented Feb 16, 2017

@tkelman edit: got it, I think. incoming

@ararslan ararslan added test This change adds or pertains to unit tests domain:types and dispatch Types, subtyping and method dispatch labels Feb 16, 2017
@felixrehren
Copy link
Contributor Author

I don't understand the error message

  LoadError: syntax: 
  ERROR: deprecated syntax "inner constructor D8915(...) around /tmp/julia/share/julia/test/subtype.jl:932".
  Use "D8915{T}(...) where T" instead.

and can't work out from NEWS.md what the new syntax should be :/

test/subtype.jl Outdated
f19041{K}(x::A19041{K}) = deepcopy(x)
@test f19041(A19041(B19041)) == A19041{B19041}()

# #19985
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

@yuyichao
Copy link
Contributor

and can't work out from NEWS.md what the new syntax should be :/

Have you tried doing exactly what the deprecation warning says?

@StefanKarpinski
Copy link
Sponsor Member

Have you tried doing exactly what the deprecation warning says?

It's on the second line of the error message in case you missed it (I did on first reading).

test/subtype.jl Outdated
struct B12814{N, T} <: A12814{N, T}
x::NTuple{N, T}
end
Base.call{T <: A12814, X <: Array}(::Type{T}, x::X) = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

call overloading in this style is deprecated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that when I tried this is in 0.5, but couldn't figure out the transformation (and the warning had disappeared in 0.6?). How should this be spelled now?

Copy link
Contributor

Choose a reason for hiding this comment

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

You should follow the deprecation warning on 0.5.

(::Type{T}){T<:A12814,X<:Array}(x::X) = 1

@felixrehren
Copy link
Contributor Author

felixrehren commented Feb 17, 2017

Thanks all for the help!
@yuyichao @StefanKarpinski I tried to follow the depreciation warning, but I don't understand it (what inner constructor?), and my attempts to fix it haven't worked. I would be grateful for any further pointers / a fix!

@yuyichao
Copy link
Contributor

yuyichao commented Feb 17, 2017

(what inner constructor?)

The meaning of inner constructor should be very unambiguous: http:https://docs.julialang.org/en/latest/manual/constructors.html#Inner-Constructor-Methods-1

and my attempts to fix it haven't worked.

What didn't work? The depwarn shouldn't be printed anymore?

@felixrehren
Copy link
Contributor Author

felixrehren commented Feb 18, 2017

Writing D8915{T}(a) where T = 1 instead of D8915(a) = 1 didn't work: the depwarn is gone, replaced by

LoadError: syntax: "1" is not a valid function argument name
  while loading /tmp/julia/share/julia/test/subtype.jl, in expression starting on line 935

and

Got an exception of type LoadError outside of a @test
ERROR: LoadError: type UnionAll has no field parameters

on two of the Travis builds.

@yuyichao
Copy link
Contributor

Please check the line number. That's not the same line at all.

https://github.com/JuliaLang/julia/pull/20627/files#diff-4cf37a18308090507d83e9d96dd01692R935 is wrong.

@tkelman
Copy link
Contributor

tkelman commented Feb 18, 2017

I don't think that deprecation message is all that obvious, fwiw

@yuyichao
Copy link
Contributor

What would be a more obvious one?

@tkelman
Copy link
Contributor

tkelman commented Feb 18, 2017

printing the actual contents of the line/signature that caused the warning instead of a (...) abbreviation of it

@yuyichao
Copy link
Contributor

The abbreviated part is not important since it does not need to be changed at all. It should be ok to print it if it's short but when it's longer it'll make it much harder to spot the actual change that needs to be made.

@tkelman
Copy link
Contributor

tkelman commented Feb 18, 2017

it is important if it makes it easier to identify what actually needs changing. right now it's not very clear, like when there are multiple inner constructors

@yuyichao
Copy link
Contributor

What about multiple inner constructors? If none of them have type parameters they need to be changed in exactly the same way and are printed as such in the deprecation warning?

@tkelman
Copy link
Contributor

tkelman commented Feb 18, 2017

then you should get the warning once per constructor, otherwise fixing one will only change the line number of the warning (if you're lucky) so you don't get much feedback that you partially fixed it

@yuyichao
Copy link
Contributor

then you should get the warning once per constructor

You do?

julia> struct A{T}
           A() = 1
           A(a) = 2
       end

WARNING: deprecated syntax "inner constructor A(...) around REPL[1]:2".
Use "A{T}(...) where T" instead.

WARNING: deprecated syntax "inner constructor A(...) around REPL[1]:3".
Use "A{T}(...) where T" instead.

@yuyichao
Copy link
Contributor

Just noticed that the inner constructor with type parameters is not printed correctly.

julia> struct C{T}
           C{T2}(a::T2) = T2
       end

WARNING: deprecated syntax "inner constructor C(...) around REPL[3]:2".
Use "C{T}(...) where T" instead.

Should say

WARNING: deprecated syntax "inner constructor C{T2}(...) around REPL[3]:2".
Use "C{T}(...) where {T,T2}" instead.

Though this gets a lot trickier for

struct D{T}
    D{T}(::T) = 1
end

which should probably suggest sth like D{_}(::T) where {_,T} = T.

@felixrehren
Copy link
Contributor Author

What should the correct version of lines 930 - 935 be?

@yuyichao
Copy link
Contributor

mutable struct D8915{T <: Union{Float32,Float64}}
    D8915{T}(a) where T = 1
    D8915{T}(a::Int) where T = 2
end
@test D8915{Float64}(1) == 2

@tkelman
Copy link
Contributor

tkelman commented Feb 18, 2017

preferably with curly braces where {T} for a bit nicer readability

@felixrehren
Copy link
Contributor Author

Is there a problem with remote exceptions? Not sure what to make of this message in the logs, seems to be a different remote worker that failed in each case

Error in testset subtype:
Error During Test
  Test threw an exception of type RemoteException
  Expression: subtype
  RemoteException(1, CapturedException(CompositeException(Any[CapturedException(ErrorException("Error deserializing a remote exception from worker 8\nRemote(original) exception of type Base.Test.TestSetException\nRemote stacktrace : "), Any[(fini...

@yuyichao
Copy link
Contributor

Yes there are some issues with test result serialization so it doesn't actually show the error.

OTOH, please run the tests locally first before pushing.

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2017

bump, needs rebase

@pabloferz
Copy link
Contributor

Rebased (hope you don't mind)

@tkelman
Copy link
Contributor

tkelman commented Mar 2, 2017

likely needs "fixing whatever was broken about the tests" too in addition to just a rebase, so this'll probably fail at the moment

@felixrehren
Copy link
Contributor Author

Thanks for rebase @pabloferz! ❤️ Fixed tests, builds build. LGTM?

@Keno
Copy link
Member

Keno commented Jul 22, 2017

Bump @JeffBezanson

@Keno
Copy link
Member

Keno commented May 9, 2019

Bump @JeffBezanson are any of these still relevant?

@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Apr 8, 2021
@vtjnash vtjnash merged commit 21eb1b6 into JuliaLang:master Apr 9, 2021
@simeonschaub simeonschaub removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:types and dispatch Types, subtyping and method dispatch test This change adds or pertains to unit tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants