Skip to content

Prevent "method overwritten" warning on julia 0.5 #5

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

Merged
merged 1 commit into from
Aug 2, 2016
Merged

Prevent "method overwritten" warning on julia 0.5 #5

merged 1 commit into from
Aug 2, 2016

Conversation

timholy
Copy link
Collaborator

@timholy timholy commented Jul 29, 2016

On julia 0.5, using trait functions generates a method-overwritten warning. This occurs because the trait-dispatching function

myfunction(SimpleTraits.trait(arg), arg)

gets generated for both Tr{X} and !Tr{X}. This PR tries to fix that, by anticipating that traits come in pairs: when the opposite member of the pair arrives, it avoids generating the trait-dispatching function. But kudos to you and your tests: it turns out to be important to do this only once, because if you want to redefine a trait function you need to also redefine the dispatcher (because of julia 265).

I thought I had come up with a way where this could still cause trouble:

julia> using SimpleTraits

julia> @traitdef Tr{X}

julia> @traitimpl Tr{Int}

julia> @traitfn f{X; Tr{X}}(x::X) = 1
f (generic function with 2 methods)

julia> f(100)
1

julia> # note we only defined 1 of 2; now redefine both, calling after the first to force-compile

julia> @traitfn f{X; !Tr{X}}(x::X) = 2
f (generic function with 3 methods)

julia> f(1.0)
2

julia> @traitfn f{X; Tr{X}}(x::X) = 3
WARNING: Method definition f(#X<:Any) in module Main at /home/tim/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:160 overwritten at /home/tim/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:160.
WARNING: Method definition f(Type{Main.Tr{#X<:Any}}, #X<:Any) in module Main at /home/tim/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:154 overwritten at /home/tim/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:154.
f (generic function with 3 methods)

julia> f(100)
3

julia> f(1.0)
2

Counter to my expectation, this still worked properly, so I think (somewhat surprisingly 😄) that this is actually OK.

@@ -119,6 +119,8 @@ end
# @traitfn f{X,Y; Tr1{X,Y}}(x::X,y::Y) = ...
# @traitfn f{X,Y; !Tr1{X,Y}}(x::X,y::Y) = ... # which is just sugar for:
# @traitfn f{X,Y; Not{Tr1{X,Y}}}(x::X,y::Y) = ...
let dispatch_cache = Set() # to ensure that the trait-dispatch function is defined only once per pair
global traitfn
Copy link
Owner

Choose a reason for hiding this comment

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

I'd indent here.

@mauro3
Copy link
Owner

mauro3 commented Aug 2, 2016

Thanks for tackling this! These problematic cases still give warnings (but works otherwise):

julia> @traitfn g{X; Tr{X}}(x::X) = 1
g (generic function with 2 methods)

julia> @traitfn g{X; !Tr{X}}(::X) = 2
WARNING: Method definition g(#X<:Any) in module Main at /home/mauro/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:160 overwritten at /home/mauro/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:160.
g (generic function with 3 methods)

julia> g(5)
1

julia> g(5.0)
2

julia> @traitfn h{X; Tr{X}}(x::X) = 1
h (generic function with 2 methods)

julia> @traitfn h{Y; !Tr{Y}}(x::Y) = 2
WARNING: Method definition h(#Y<:Any) in module Main at /home/mauro/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:160 overwritten at /home/mauro/.julia/v0.5/SimpleTraits/src/SimpleTraits.jl:160.
h (generic function with 3 methods)

julia> h(4)
1

julia> h(4.0)
2

Ideally one would query the method table to check for the existence of the trait-dispatch function, then add some extra storage to allow over-writing. However, method_exists is not suited for this task (and hand-coding it is tough, this is where a lot of the complexity of Traits.jl is). Maybe after the type-system overhaul this becomes easy enough.

So, I'll merge this and we can revisit this at some point in the future, unless you got a better idea.

@mauro3
Copy link
Owner

mauro3 commented Aug 2, 2016

I just saw: JuliaLang/julia#17618 Would that fix the overwritten warning?

@timholy
Copy link
Collaborator Author

timholy commented Aug 2, 2016

Yeah, I don't think this can handle renaming the types. Checking the method table isn't ideal either: what if you were intending to redefine the method? That's why I only allow this to prevent redefinition for the opposite member of the pair.

I just saw: JuliaLang/julia#17618 Would that fix the overwritten warning?

As far as I can tell, no. That seems to be about type redefinition, not method overwriting.

I added indentation of the let block.

@mauro3
Copy link
Owner

mauro3 commented Aug 2, 2016

Yes, the dispatch_cache would still be needed with the method_exists, but instead the method itself could be used as the key.

@mauro3 mauro3 merged commit 69efea4 into mauro3:master Aug 2, 2016
@timholy timholy deleted the pull-request/c16d389b branch August 2, 2016 12:40
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.

2 participants