Understand what the problem with IsIterator might be

[davidanthoff]

@yuyichao made this comment in a different issue:

9393f4a actually shouldn't cause an error like this but it will cause #265-like bug.

I don't understand that comment, can you please elaborate, @yuyichao?

[yuyichao]

Defining start method will not cause the trait to be recompiled so if you define a iteration protocol the trait may not see it.

[vtjnash]

We should be able to make method_exists (and applicable) inferable now, which would make this inferable, and allowing removing the @generated without causing a runtime performance impact.

If you want to make a PR, inference would just need a new tfunc (following the examples at
https://github.com/JuliaLang/julia/blob/4e698cb55248b8191c4d68f9c18b656fecf48da4/base/inference.jl#L1659, which returns Const(true) or returns Const(false) (as appropriate), and adds a backedge to the method table (similar to https://github.com/JuliaLang/julia/blob/4e698cb55248b8191c4d68f9c18b656fecf48da4/base/inference.jl#L1393-L1396). I can help with this more if you're interested.

[davidanthoff]

Defining start method will not cause the trait to be recompiled so if you define a iteration protocol the trait may not see it.

Is that essentially the following story? If I call istrait(IsIterator{X}) on a type X for which no start method is defined, and after that istrait call I actually define the start method, then any future call to istrait(IsIterator{X}) will still return false? I'm aware of that issue, but I think we can live with that, certainly for my use cases that is not a problem. But on the flip side, if I define the type and the start method without a call to the istrait method in between, everything should be good, right?

We should be able to make method_exists (and applicable) inferable now, which would make this inferable, and allowing removing the @generated without causing a runtime performance impact.

If you want to make a PR, inference would just need a new tfunc (following the examples at
https://github.com/JuliaLang/julia/blob/4e698cb55248b8191c4d68f9c18b656fecf48da4/base/inference.jl#L1659, which returns Const(true) or returns Const(false) (as appropriate), and adds a backedge to the method table (similar to https://github.com/JuliaLang/julia/blob/4e698cb55248b8191c4d68f9c18b656fecf48da4/base/inference.jl#L1393-L1396). I can help with this more if you're interested.

I won't be able to make that PR, that is beyond my understanding of julia. But would we gain anything from that? From my tests it seems that the IsIterator trait actually doesn't suffer from the problems discussed in the other issues and works just fine on julia 0.6?

[yuyichao]

It'll make interactive development harder (which is basically all what #265 is about, I'm not aware of any cases in package code that's directly affected by #265).

The gain would be making this code not break in the future. Arguing that something "seems" to work and "doesn't suffer from the problems" is basically what causing the other issue in the first place. I don't think there's immediate plan to make changes that breaks this but it's not impossible.

[vtjnash]

But on the flip side, if I define the type and the start method without a call to the istrait method in between, everything should be good, right?

Which answer would you like to hear? Like in #39, you are depending on the compiler not being able to notice.

then any future call to istrait(IsIterator{X}) will still return false

nah, that's much too simplistic. The issue is that future calls will probably be non-deterministic between returning true, false, causing random memory corruption, or a hard-abort of the program with SIGILL / SEGV (if you're lucky).

[davidanthoff]

So I guess the best resolution for this issue would be to find someone who can make the changes to base that @vtjnash suggested, and then just change the trait method to:

SimpleTraits.trait{X}(::Type{IsIterator{X}}) = method_exists(start, Tuple{X}) ? IsIterator{X} : Not{IsIterator{X}}

My understanding from the comments above is that this would essentially solve the problem? I actually don't understand why, but I trust that @vtjnash has a better grasp of these things than I do ;)

The other question is what we do until someone has made these changes to julia... In terms of timeline, @vtjnash do you see any change of this making it into julia 0.6? I hear your dire warnings about non-deterministic behaviors and aborts etc., but then I have used this trait every day for two months now and not once has any of that happened. So either I was really, really lucky, or maybe this is more something you expect to stop working in the future?

For my own use of this, we could move away from the generated function right now already. My use-case is not really performance critical, so if this leads to what @mauro3 calls dynamic dispatch until we have the changes in julia, I could live with that.

[mauro3]

@vtjnash #40 (comment) is excellent news. I might have a go at this, but unlikely before May, and likely needing a lot of help.

There is a tfunc addition for applicable: https://github.com/JuliaLang/julia/blob/4e698cb55248b8191c4d68f9c18b656fecf48da4/base/inference.jl#L555 . Is that related?

[goretkin]

I tried to follow this discussion and the discussion over at #39 and I'm currently out of my depth. Is this issue still an issue?

And is it fine to get around the issue by explicitly/manually defining a type to have the trait Isiterator? i.e.

@traitimpl IsIterator{MyType}

and just make sure that I do that only if there's also iterate(::MyType)?

[mauro3]

Yep, I should really remove the generated function for IsIterator, it angers the gods. But, yes if you do it manually then it will be fine for that type. Otherwise, we should just do runtime dispatch until the time hasmethod is inferable.

Anyone against runtime dispatch for IsIterator?

[mauro3]

There are not going to be more answers...