-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
(method too new to be called from this world context.) #21356
Comments
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 mauro3/SimpleTraits.jl@9393f4a !! |
@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.) |
#19300 would be nice to have. |
The restriction of |
It wouldn't have helped me, since I am not using |
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. |
What should it do instead? |
Use normal functions?
Also documented. |
@yuyichao fair enough. I looked under I have trouble seeing how the code in mauro3/SimpleTraits.jl@9393f4a violates:
Which I assume is the restrictions you are mentioning as being documented? It would be good to understand this. |
Right, the issue (#19300 ) is just about doc string. And I do think copying over part of the doc would be a good start. mauro3/SimpleTraits.jl@9393f4a actually shouldn't cause an error like this but it will cause #265-like bug. The other use of There's very detailed doc of the restriction it's violating slightly below the part you referenced. Starting from
to the summary
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 |
I think that's fine. The problem seems to be that @generated SimpleTraits.trait{F}(::Type{IsDirected{F}}) = is_directed(F) ? :(IsDirected{F}) : :(Not{IsDirected{F}}) This generated function requires that The answer is to make this just a function (i.e. a runtime check), define all |
@sbromberger Yes, if you use
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.
Yes, all of these will work. If you want to use the first option, you will have to implement the 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. |
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. |
being a function != being runtime. I submitted a PR to SimpleTraits to remove the invalid |
@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" |
Well known to whom? I think the existence of this issue disproves that assertion. |
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... |
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 don't need to. This only means that the package is broken. 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. |
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 (JuliaArrays/StaticArrays.jl#95) or PR (mauro3/SimpleTraits.jl#39 (review)) to explain why the attempted code is invalid / unsound and that marking functions as 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 mauro3/SimpleTraits.jl#39 (comment) 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. |
I believe this page shows up when searching 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 😄) 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. |
Replace the |
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................................) |
Ref #20828 |
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 :) ). |
Aside, here all the related issues: - #39 - #40 - JuliaLang/julia#21356
Aside, here all the related issues: - #39 - #40 - JuliaLang/julia#21356
Aside, here all the related issues: - #39 - #40 - JuliaLang/julia#21356
This used to work, but here's the error I'm getting. (Crossposted from mauro3/SimpleTraits.jl#38):
The text was updated successfully, but these errors were encountered: