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

Strange dispatch with @pure #20704

Closed
andyferris opened this issue Feb 21, 2017 · 22 comments
Closed

Strange dispatch with @pure #20704

andyferris opened this issue Feb 21, 2017 · 22 comments
Assignees
Labels
compiler:inference Type inference kind:bug Indicates an unexpected problem or unintended behavior

Comments

@andyferris
Copy link
Member

I was trying to understand the behavior of @pure (as well as @generated) and how they relate to the recent fix to #265, and came across this strange case.

I'm not sure why the following code doesn't result in a missing method error on foo on the last line:

julia> foo(::Int) = 1
foo (generic function with 1 method)

julia> Base.@pure bar(x::ANY) = foo(x)
bar (generic function with 1 method)

julia> bar(42)
1

julia> bar(42.0)
1

The error is (correctly) present on Julia v0.5.

@kshyatt kshyatt added the domain:types and dispatch Types, subtyping and method dispatch label Feb 21, 2017
@StefanKarpinski
Copy link
Sponsor Member

$100 says @vtjnash tells you this function isn't pure.

@andyferris
Copy link
Member Author

ROFL

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 21, 2017

Do I get that $100? Because that is indeed the correct answer. We inferred that if bar has a return value it must be 1. Since it is marked @pure, we can strengthen that say that its return value is always 1. Also, since it is marked @pure, we don't need to consider what might happen if the method table changes, so the compiler is free to disregard any operation that might invalidate that answer.

@yurivish
Copy link
Contributor

What does @pure mean, exactly?

@Keno
Copy link
Member

Keno commented Feb 21, 2017

@pure means I am @vtjnash and I certify that this function doesn't break any of the implicit assumptions I use in the compiler ;)

@andyferris
Copy link
Member Author

andyferris commented Feb 21, 2017

Thanks @vtjnash for the explanation of what the compiler has done. Please note that this behaviour doesn't occur without the ::ANY, and there were no method table changes.

Also contrast that with this situation:

julia> foo(::Int) = 1
foo (generic function with 1 method)

julia> foo(::Float64) = 2
foo (generic function with 2 methods)

julia> Base.@pure bar(x::ANY) = foo(x)
bar (generic function with 1 method)

julia> bar(42)
1

julia> bar(42.0)
2

julia> bar(true)
ERROR: MethodError: no method matching foo(::Bool)
Closest candidates are:
  foo(::Float64) at REPL[2]:1
  foo(::Int64) at REPL[1]:1
Stacktrace:
 [1] bar(::Any) at ./REPL[3]:1

To me, it seems undesirable that the different cases here have different results (in terms of sometimes throwing the MethodError and sometimes giving a spurious result). I'm pretty sure I can follow what Jameson is saying the compiler is doing with inference, the lack of dependency edges for @pure functions, and also that @pure is not exported for general usage - but despite all of that, it seems that @pure is currently fraught with dangers and complications which I hope that we don't expect everyday users to be happy with having to learn.

Also, since it is marked @pure, we don't need to consider what might happen if the method table changes, so the compiler is free to disregard any operation that might invalidate that answer.

I think this is specifically where I am hung-up (and probably disagree). Historical issues on github show that @pure was implemented for the purpose of constant propagation.

It seems of most utilitarian value if a @pure method is just a simple and plain method which inference is allowed to evaluate if it has constant inputs. To me, that doesn't imply anything distinct about dependency edges for @pure methods as opposed to functions which are @inlined.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 21, 2017

currently fraught with dangers and complications which I hope that we don't expect everyday users to be happy with having to learn.

Correct. I don't expect anyone to learn them (I certainly haven't :P). I am happy to tell people just to avoid it.

was implemented for the purpose of constant propagation.

It sometimes gets used for that, but I suspect that it's not really correct. It was implemented to slightly generalize some tfunctions so that Core.Inference could move out of Base. I hope someday to finally be rid of it. But it currently functions as a stop gap.

@andyferris
Copy link
Member Author

I am happy to tell people just to avoid it.

I hope someday to finally be rid of it. But it currently functions as a stop gap.

OK, I hadn't seen an opinion like this before - is this the consensus, then?

Again, I realise it was an unexported feature, but @pure has served as a very useful and powerful construct in v0.5 - for really nice constant (value) propagation, (slightly) more complex operations on types, and for creating API's that are a bit prettier (e.g. StaticArray uses it to allow users to type Size(3) instead of Size{(3,)}()). It would be a shame to lose its functionality and/or prior reliability without some replacement. Having a keyword which says "inference is free to evaluate this function when it encounters it at compile-time with constant inputs" seems incredibly useful (to me).

@c42f
Copy link
Member

c42f commented Feb 21, 2017

Oddities like this remind me of C-like UB - if you do the wrong thing "anything can happen, and frequently does". This seems at odds with the most julia semantics, where if the compiler can't figure it out at compile time you get the expected behavior, but at runtime.

Not that I mind if pure and generated go away, as long as there's a nice replacement for what we had in 0.5, which was, in my opinion quite pragmatic and fairly easy to understand: if you invoke a pure function on a new type before telling the compiler all the information about that type, you get the wrong thing. If you do it after providing the compiler with all the information, you get the right thing. It seems that the possible uses of pure in 0.6 have changed dramatically, perhaps to the extent that it should be called something else.

@andyferris
Copy link
Member Author

andyferris commented Feb 21, 2017

Oddities like this remind me of C-like UB

Right, I'd generally prefer that language authors work hard (go out of their way) to throw errors instead of allow users to type things which result in undefined behavior... (says someone who has never authored a language).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 21, 2017

where if the compiler can't figure it out at compile time you get the expected behavior, but at runtime

yes. That's exactly what the compiler does, if you don't mark something as "ignore this" (aka @pure). This was largely all hashed out in the original PR (#13555) and issue (#414)

If you do it after providing the compiler with all the information, you get the right thing

That was never what it did. The v0.6 semantics haven't changed, it's just gotten slightly better at benefitting from the information it has already computed.

throw errors instead of allow users to type things which result in undefined behavior

I'd be happy to replace Base.@pure with macro pure(f); error("function $f is not pure"); end. It would even be entirely correct 😛

I'm not convinced this particular optimization is particularly beneficial, so perhaps it should be disabled. It was introduced by Jeff in as a small piece of #16837.

@c42f
Copy link
Member

c42f commented Feb 21, 2017

Perhaps, then, @pure never meant what I thought it did. However, take this as a data point: I found the behavior of @pure in 0.5 easy to understand and useful in practice, regardless of the underlying semantic you actually intended when implementing it! In 0.6 it seems dramatically more difficult to understand and less practically useful.

@timholy
Copy link
Sponsor Member

timholy commented Feb 21, 2017

I've been (and still am) confused about the meaning of @pure too. @vtjnash, can you write up a short docstring and a longer explanation for devdocs? Given that other languages also have pure annotations, I don't think this feature is going away; given the subtleties and potential for abuse, it should be a high priority to make sure it's clearly documented.

@timholy
Copy link
Sponsor Member

timholy commented Feb 21, 2017

And as a good-faith exchange, I just added some documentation (#20712) that someone else indirectly requested (https://discourse.julialang.org/t/using-cartesianrange-flattening-before-collect/2150). I just wanted to make sure you didn't have any excuses for avoiding this task 😄.

@StefanKarpinski
Copy link
Sponsor Member

It seems completely wrong for @pure to imply that a function cannot throw an error. @vtjnash, what possible justification for that assumption is there aside from convenience? I've advocated over and over again for a very simple definition of the @pure annotation: that the compiler is free to call the function/method whenever and as many times as it wants (including zero), and cache the result. If the function throws an error at compile time, it should not, however, cause the compiler to crash (obviously), but the compiler is free to emit code that unconditionally throws the error object that was produced during compilation.

@JeffBezanson
Copy link
Sponsor Member

This was not the intent of #16837. The only intent of that change was to add a calling convention for returning a constant, for use when it is correct to do so.

@JeffBezanson JeffBezanson added kind:bug Indicates an unexpected problem or unintended behavior compiler:inference Type inference and removed domain:types and dispatch Types, subtyping and method dispatch labels Feb 21, 2017
@JeffBezanson
Copy link
Sponsor Member

JeffBezanson commented Feb 21, 2017

I'm pretty sure I have a handle on why this case occurs, but I'm having trouble understanding why certain other cases work correctly. Hoping @vtjnash can explain:

julia> Base.@pure foo(::Int) = 1

julia> bar(x::ANY) = foo(x)

julia> foo(1)
1

julia> bar(1.2)
ERROR: MethodError: no method matching foo(::Float64)

julia> code_typed(bar,(Any,))
1-element Array{Any,1}:
 CodeInfo(:(begin 
        return (Main.foo)(x)::Int64
    end))=>Int64

What I'm expecting is that typeinf_edge sees that foo uses the constant calling convention, so returns Const(1). Then inlineable sees that foo is marked pure and inferred to be Const, so calls inline_as_constant. Crucially, that case happens before we check whether the argument type is a subtype of the method signature in inlineable ((atype <: metharg)), so I'd expect the constant to get inlined in bar.

So why doesn't all that happen?

@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 21, 2017

So why doesn't all that happen?

It fails for me :P

Seems to be unstable though. Definitely though those two tests are reversed. I don't see how that bug is related to @pure either – It looks that one should be showing up regardless.

@JeffBezanson
Copy link
Sponsor Member

inlineable checks method.source.pure, which I believe means it is influenced by @pure. We should perhaps be checking pure on the specialization instead though.

@JeffBezanson JeffBezanson self-assigned this Feb 21, 2017
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 21, 2017

No, that makes sense to me. We check later for jlcall_api == 2. The specific request in this issue here is that we need to track whether e.typ came from calling apply_pure or was merely inferred to be the result. One option would be to just re-call apply_pure instead of using the Const object.

@JeffBezanson
Copy link
Sponsor Member

It goes a bit beyond that, since we also need to avoid setting jlcall_api == 2 for a function that might throw an error.

JeffBezanson added a commit that referenced this issue Feb 27, 2017
fix #20704, `pure` annotation should not skip method errors
@c42f
Copy link
Member

c42f commented Mar 1, 2017

Awesome, thanks guys for fixing this :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference kind:bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

9 participants