This used to work, but here's the error I'm getting. (Crossposted from https://github.com/mauro3/SimpleTraits.jl/issues/38):
julia> using SimpleTraits
julia> abstract type AbFoo end
julia> struct Foo <: AbFoo
x::Int
end
julia> @traitdef IsDirected{G<:AbFoo}
julia> @traitimpl IsDirected{G} <- is_directed(G)
julia> is_directed(x...) = error("nope")
is_directed (generic function with 1 method)
julia> is_directed(::Foo) = true
is_directed (generic function with 2 methods)
julia> is_directed(::Type{Foo}) = true
is_directed (generic function with 3 methods)
julia> is_directed(Foo)
true
julia> is_directed(AbFoo)
ERROR: nope
Stacktrace:
[1] is_directed(::Type{T} where T) at ./REPL[6]:1
julia> @traitfn f(g::::IsDirected) = g.x
f (generic function with 2 methods)
julia> f(Foo(10))
ERROR: MethodError: no method matching is_directed(::Type{Foo})
The applicable method may be too new: running in world age 21602, while current world is 21607.
Closest candidates are:
is_directed(::Type{Foo}) at REPL[8]:1 (method too new to be called from this world context.)
is_directed(::Any...) at REPL[6]:1 (method too new to be called from this world context.)
is_directed(::Foo) at REPL[7]:1 (method too new to be called from this world context.)
Stacktrace:
[1] trait(...) at /Users/seth/.julia/v0.6/SimpleTraits/src/SimpleTraits.jl:186
[2] f(::Foo) at /Users/seth/.julia/v0.6/SimpleTraits/src/SimpleTraits.jl:319
julia> is_directed(Foo)
true
Julia is correct - the SinpleTraits package is using generated functions incorrectly (to simulate dispatch).
@vtjnash um, wait a sec. We're using SimpleTraits extensively in LightGraphs now. This is the first I've heard that anything's "incorrect" about them. If this is the case, why is the package still registered?
Also, what changed? This used to work (in fact, it was how I prototyped our use of SimpleTraits for LightGraphs not 2 months ago).
cc @StefanKarpinski for a sanity check here.
Generated function cannot call methods defined after them while generating the function body.
@yuyichao I'm just a user of SimpleTraits and don't understand the implications of this. Does this mean that all the work we've done getting LightGraphs ready for 0.6 and beyond - along with our trait interface - has been for nothing?
Again, this was working less than 2 months ago.
I'm also just expand on what invalid thing it is doing and I have no idea what's all the work you've done to get things working so it's impossible for me to comment on that.
FWIW, one invalid use of generated function is precisely added two months ago https://github.com/mauro3/SimpleTraits.jl/commit/9393f4adacbd7ec2d720a51befa6cd44b46f65fd !!
@yuyichao I used base traits as examples when I was working out how to make things work for LightGraphs.
What's the fix for this? I do not relish having to rearchitect LightGraphs due to what appears to this stupid user to be a regression. (That is, something that was working as expected and as documented now isn't, and no code except for the underlying language has changed.)
WRT documenting the restrictions of @generated
The restriction of @generated
is already documented.......................
It wouldn't have helped me, since I am not using @generated
anywhere in my code and wouldn't have known that I was indirectly using it.
It wouldn't have helped me
Sure, but what would help you is to report this to the package instead. It seems that it's just using generated functions when there's no need to.
@yuyichao I originally opened the issue there, but opened it here as well because it appears to only affect the REPL right now.
Sure, but what would help you is to report this to the package instead. It seems that it's just using generated functions when there's no need to.
What should it do instead?
What should it do instead?
Use normal functions?
but opened it here as well because it appears to only affect the REPL right now.
Also documented.
@yuyichao fair enough. I looked under ?@generated
but should instead have looked under
https://docs.julialang.org/en/latest/manual/metaprogramming.html#Generated-functions-1
I have trouble seeing how the code in
https://github.com/mauro3/SimpleTraits.jl/commit/9393f4adacbd7ec2d720a51befa6cd44b46f65fd
violates:
Generated functions must not have any side-effects or examine any non-constant global state (including, for example, IO, locks, or non-local dictionaries). In other words, they must be completely pure. Due to an implementation limitation, this also means that they currently cannot define a closure or untyped generator.
Which I assume is the restrictions you are mentioning as being documented?
It would be good to understand this.
I looked under ?@generated but should instead have looked under
Right, the issue (#19300 ) is just about doc string. And I do think copying over part of the doc would be a good start.
https://github.com/mauro3/SimpleTraits.jl/commit/9393f4adacbd7ec2d720a51befa6cd44b46f65fd actually shouldn't cause an error like this but it will cause #265-like bug. The other use of @generated
function is bad though and can easily cause issues like this.
There's very detailed doc of the restriction it's violating slightly below the part you referenced. Starting from
It is also important to see how
@generated
functions interact with method redefinition. Following the principle that a correct@generated
function must not observe any mutable state or cause any mutation of global state, we see the following behavior. Observe that the generated function cannot call any method that was not defined prior to the definition of the generated function itself.
to the summary
Calling any function that is defined after the body of the generated function. This condition is relaxed for incrementally-loaded precompiled modules to allow calling any function in the module.
which also explained the difference between a (precompiled) package and REPL (or with precompilation disabled).
OK, basic question here: does the problem go away if I use @traitimpl
without a function (that is, stop using @traitimpl IsDirected{G} <- is_directed(G)
and instead just explicitly call the types out via @traitimpl IsDirected{DiGraph}
)?
I think that's fine. The problem seems to be that @traitimpl
on a function essentially does
@generated SimpleTraits.trait{F}(::Type{IsDirected{F}}) = is_directed(F) ? :(IsDirected{F}) : :(Not{IsDirected{F}})
This generated function requires that is_directed
is defined before the generated function is created (and all of the dispatches it uses?)
The answer is to make this just a function (i.e. a runtime check), define all is_directed
dispatches before the generated function, or directly set the traits. Did I get that correct @yuyichao ?
@sbromberger Yes, if you use @traitimpl
without a function things should work.
This generated function requires that is_directed is defined before the generated function is created (and all of the dispatches it uses?)
On julia 0.6 this generated function will only see methods that were defined before this generated function gets defined (or run for the first time?). So defining the function first, then defining the generated function and then adding methods to the original function will not work.
The answer is to make this just a function (i.e. a runtime check), define all is_directed dispatches before the generated function, or directly set the traits. Did I get that correct @yuyichao ?
Yes, all of these will work. If you want to use the first option, you will have to implement the SimpleTraits.trait
method by hand, and you will no longer have a holy trait, so things will be much slower. So while these work, I don't see how we can get back programmed traits that end up dispatching as fast as holy traits...
Is this a case where julia didn't follow the normal deprecation rules? This feature worked on julia 0.5, and the documentation that explained that this use of generated functions is not supported was only added after julia 0.5 was released. My understanding was always that if a feature gets removed from julia, there would be one release cycle where things would still work but throw a deprecation warning.
Yes, all of these will work. If you want to use the first option, you will have to implement the SimpleTraits.trait method by hand, and you will no longer have a holy trait, so things will be much slower. So while these work, I don't see how we can get back programmed traits that end up dispatching as fast as holy traits...
This is going to be a far reaching issue...
I've gotta say, this was not the right way to have handled this. From my perspective, I was simply using a package that just broke without warning due to a change in the language. The breakage threatened to obviate all the work I did in getting a new version of my package ready for Julia v0.6, and there was initially no acknowledgement that this was even a problem.
As it turns out, there's an acceptable workaround (that will require a few more hours of coding to effect), but I'm really nervous that something else that changes in Julia will break a feature upon which we depend, and we will have no notice of that happening until our users start complaining. That is not a good way to maintain software.
On the plus side, lots of people jumped in to offer opinions and help diagnose the issue, which led to the discovery of a workaround. I am grateful for that.
(edited to add): the fact that compiled code works but REPL code doesn't is as big a problem as the breakage, IMO. This means that one cannot rely on prototyping in the REPL to test code that may be deployed in a module, and this significantly weakens one of Julia's core strengths.
The answer is to make this just a function (i.e. a runtime check)
being a function != being runtime. I submitted a PR to SimpleTraits to remove the invalid @generated
function and just replace it with a compile-time check.
I submitted a PR to SimpleTypes to remove the invalid @generated function and just replace it with a compile-time check.
@vtjnash - thank you. Do you have any thoughts on how this might impact performance (if at all)?
If your trait functions are inferable, it'll have no impact.
Sorry - does "inferable" mean "type-stable", or is there another meaning?
Yes, "type-stable" is usually a short-hand for "feasible for inference to compute a concrete leaf type given the concrete leaf types of the arguments"
If your trait functions are inferable, it'll have no impact.
Just for those that aren't following the discussion over at mauro3/SimpleTraits.jl#39, this is not correct. As of right now it looks like julia 0.6 introduced a breaking change without the normal deprecation phase out that we can't work around and get the same performance that we had on julia 0.5.
As of right now it looks like julia 0.6 introduced a breaking change without the normal deprecation phase
It is well known that fixing #265 is a major breaking change.
It is well known that fixing #265 is a major breaking change.
Well known to whom? I think the existence of this issue disproves that assertion.
It is well known that fixing #265 is a major breaking change.
It might be well known to the core dev group, but I have not seen any communication to the wider community about this. Don't assume all package developers are following every issue and PR here in the julia repo...
Don't assume all package developers are following every issue and PR here in the julia repo...
and even if we were, you can't assume that we will understand the impact of #265 on dependent packages on which we rely.
The breakage has been discussed multiple time on the issue tracker and being clearly documented in the doc. There's no deprecations since it's impossible. It of course won't help for anyone that doesn't read the doc.
I'm done.
you can't assume that we will understand the impact of #265 on dependent packages on which we rely.
You don't need to. This only means that the package is broken. Fixing that package should restore anything depending on it.
Fixing that package should restore anything depending on it.
It looks like that might not be doable, at least not in a way that preserves the performance we had on julia 0.5.
the documentation that explained that this use of generated functions is not supported
Yes. I've often tried to warn people not to use generated functions to lie to the compiler. When it is brought to my attention, I usually try to open an issue (https://github.com/JuliaArrays/StaticArrays.jl/issues/95) or PR (https://github.com/mauro3/SimpleTraits.jl/pull/39#pullrequestreview-32363677) to explain why the attempted code is invalid / unsound and that marking functions as @generated
in order to disable correctness is not generally a good idea (https://github.com/JuliaLang/julia/issues/21244).
When SimpleTraits merges my PR, it should fix your issue.
I think depending on undefined behavior has to be evaluated case-by-case; you should feel free to keep doing it as long as it works for you (#21244 is a case where it is _not_ working), but being aware that you might hit issues like this one.
Unfortunately I have to advise caution in using packages like Requires.jl or SimpleTraits.jl that do excessively magical things. In this case I'm hopeful that we can come up with a hack to keep SimpleTraits working. I do think we should try to keep it working, with the same level of performance, if at all possible, even if it means continuing to use code that's not strictly kosher.
SimpleTraits.jl will work just fine with @vtjnash 's change as long as the trait implication function is pure (both in functionality and performance). If workaround is still needed for "edge-cases", I posted how to do it in the comment
https://github.com/mauro3/SimpleTraits.jl/pull/39#issuecomment-293633329
along with the bad things that can happen. We can probably just add that to the README and call it a day.
With this SimpleTraits.jl isn't very magical anymore, and in fact its macros will now enforce correctness. Requires.jl on the other hand, I am not sure if that could ever be made fully compatible with precompilation.
Thanks @ChrisRackauckas , that's great.
One of the things that bothered me about world age errors was my impression that they were undocumented. It turns out my perception was due to a bug in our documentation search: navigate to https://docs.julialang.org/en/latest/, type "world age" into the search box and hit return, you'll see that it claims there are no hits. (Or at least, that's what happens for me.)
But in preparing to tackle changes to Reactive.jl, I happened to stumble across https://docs.julialang.org/en/latest/manual/methods.html#Redefining-Methods-1, which is rather helpful.
https://docs.julialang.org/en/latest/manual/methods.html#Redefining-Methods-1
I believe this page shows up when searching for world
which is how I usually find a link to it. It does come with many other results so you may need to go through them if you don't know the one you are looking for..........................................
Yes, that's the same page I linked to. There are lots of hits to "world" (mostly because of the utmost importance of "Hello, world" in programming :smile:) that come up before it, and unless you know that page exists it's not hard to give up trying the hits before you get to that one. "age" is even worse, of course.
The real problem is the claim that there are no hits to "world+age"; in a world used to the efficiency and accuracy of Google searches, that lie is so convincing that I never looked harder before today.
The real problem is the claim that there are no hits to "world+age";
Replace the +
with in the search box. This is the same reason searching macro names with
@
returns no result..................
When I do that, it very helpfully reinserts the + that I must have forgotten to include.
Actually I mean after the search result show up, edit the search box to be "world age" without pressing enter, the results should show up.... (It'll disappear again and search for "world+age" if you press enter................................)
Neat. I'll use that in the future. But again, the problem isn't so much that there is no path to finding that page, but that the first path that you have to try (because it doesn't search until you press enter at least once) claims it doesn't exist. For many people that will truncate their effort to find it.
That said, thanks for filing that issue.
Jumping in late here (I've been on vacation), but given I was in this situation (sometime earlier, with StaticArrays) I'd like to add that I really appreciate the improvements to the compiler, even though it has been a lot of (sometimes stressful) work to integrate with the #265 changes.
OTOH it seems to me to be difficult to understand/predict what constitutes "undefined behaviour that I discovered and happens to work" as opposed to "behaviour that I discovered and is actually officially defined/supported", especially for features that don't (didn't) have a wide range of users or documentation. This isn't a criticism - I don't think anyone can journal every development thought or predict every (ab)use case. But there are clearly two viewpoints from hardworking compiler authors and hardworking package authors that (very) occasionally clash, which unfortunately is what we saw here (and I know full well that (ab)using generated functions for trait-based dispatch is just so damn tempting :) ).
Most helpful comment
SimpleTraits.jl will work just fine with @vtjnash 's change as long as the trait implication function is pure (both in functionality and performance). If workaround is still needed for "edge-cases", I posted how to do it in the comment
https://github.com/mauro3/SimpleTraits.jl/pull/39#issuecomment-293633329
along with the bad things that can happen. We can probably just add that to the README and call it a day.
With this SimpleTraits.jl isn't very magical anymore, and in fact its macros will now enforce correctness. Requires.jl on the other hand, I am not sure if that could ever be made fully compatible with precompilation.