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

add function f()::T return type declaration syntax #16432

Merged
merged 2 commits into from
Jun 1, 2016
Merged

Conversation

JeffBezanson
Copy link
Member

This implements #1090. Uses the same approach as other variables with declared types, i.e. return values are wrapped in convert(T, val)::T. I think we've waited long enough for this feature! :)

@tkelman tkelman added the needs docs Documentation for this change is required label May 18, 2016
@quinnj
Copy link
Member

quinnj commented May 18, 2016

Does this allow declaring a return type on a generic function definition (i.e. function foo::Int end) and if so, does it mean anything? (My guess is it's not allowed because it opens the rest of the can of worms in #1090 around monotonicity, interface definitions, dispatch on function types, etc.)

@StefanKarpinski
Copy link
Member

The conclusion we came to at some point in the #1090 discussion was that because the effect of that kind of type declaration is non-local, it should not implicitly call convert, but rather implicitly add type assertions so that adding a type annotation would cause an error rather than silently changing behavior.

@vtjnash
Copy link
Member

vtjnash commented May 18, 2016

What scope does it get evaluated in? It seems like it could use a few more tests to make sure odd cases get lowered correctly (e.g. f(T)::T = 1, f{T}(::T) = 1, and f()::T = (T=1))

but rather implicitly add type assertions

but that is not what it implemented here?

@StefanKarpinski
Copy link
Member

but that is not what it implemented here?

It is not – on this branch:

julia> function foo::Int end
ERROR: syntax: expected "(" in function definition
 in eval(::Module, ::Any) at ./boot.jl:226

@vtjnash
Copy link
Member

vtjnash commented May 18, 2016

right, but I mean it is also not what the visually similar function foo()::Int; 1; end (implemented here) does. It seems unfortunate if the same syntax means two slightly different things.

@JeffBezanson
Copy link
Member Author

Does this allow declaring a return type on a generic function definition (i.e. function foo::Int end)

No, that is not implemented here.

What scope does it get evaluated in?

I evaluate it inside the function, at the very top. function f(...)::T is basically converted to

function f(...)
    temp = T

and temp is used from then on.

@JeffBezanson
Copy link
Member Author

What manual chapter should describe this? My first thought was functions, but types and variable type declarations are not introduced until the later types chapter. Should I add it there instead?

@tkelman
Copy link
Contributor

tkelman commented May 18, 2016

I like the idea of putting it in types, along with descriptions of typeassert and dispatch syntax.

@StefanKarpinski
Copy link
Member

Right after dispatch seems like the right place to put it.

@quinnj
Copy link
Member

quinnj commented May 18, 2016

Thanks.

So the way I'm thinking about this is as a method-specific feature: for a specific method definition, I can declare a defined return type. This has benefits for self-documentation, code readability, and general "type cleanliness" in that we're essentially adding explicit type declarations for return types in a method.

Can I request one more change to go along with this PR in the spirit of the first 2 benefits (self-doc and readability)? Can we add the declared return type, if any, to the output of, for example, methods(func) and @which 1 + 1? I believe this would involve updating at least how we show a Method, but may involve more.

We probably also want to plan on doing a mass update of some kind to put return types on things in Base and possibly update our documentation style to use this syntax; i.e. I think we often use the foo(x::Bar) => Int64 form whereas we should now use foo(x::Bar)::Int64.

@JeffBezanson
Copy link
Member Author

I don't want to use the information in these declarations too extensively. If people expect to see it e.g. from methods(f), then if you omit a declaration it will look like your function is missing something, and there will be too much pressure to write a declaration every time.

This is implemented in the front-end by rewriting all return statements for you. After that, it doesn't exist any more. It would be a significant amount of work to plumb the info farther into the system. These types can also depend on function arguments, so it's not clear what to store per-Method.

However we could have showing Methods and/or the doc system call type inference, and print the return type if it's more interesting than Any. That would capture a superset of methods with declared return types. @doc can also look for this syntax and remember the return type expression.

@quinnj
Copy link
Member

quinnj commented May 18, 2016

Fair enough; as I thought more about that, I realized it would probably require more machinery than we probably want to commit to at this point.

@JeffBezanson JeffBezanson removed the needs docs Documentation for this change is required label May 18, 2016
@IainNZ
Copy link
Member

IainNZ commented May 18, 2016

Sorry if this is a dumb question, but does this support doing operations on types, something like

function my_op{T,S}(x::Vector{T}, y::Vector{S})::promotionof(T,S)

?

@hayd
Copy link
Member

hayd commented May 19, 2016

Is something like f()::Int "string" end a compile or run-time (when it tries to convert) error?

(Should be tested.)

@JeffBezanson
Copy link
Member Author

JeffBezanson commented May 19, 2016

does this support doing operations on types

Yes; the type expression is evaluated on entry to the function.

Is something like f()::Int "string" end a compile or run-time (when it tries to convert) error

Run-time; equivalent to running convert(Int, "string") when the function returns.

@KristofferC
Copy link
Member

Yes; the type expression is evaluated on entry to the function.

How is "on entry" defined, i.e what does:

function op(a)::(rand(1:2) == 1 ? Int : Float64)
    return 1.0
end

do? Pick one when the function is compiled a new one every time the function is called?

@yuyichao
Copy link
Contributor

It is defined here #16432 (comment)

Only one version will be compiled.

@StefanKarpinski
Copy link
Member

If you write

function f(args...)::<expr>
    # stuff
    return <r1>
    # futzing
    return <r2>
    # junk
    return <r3>
end

it is transformed into something like this:

function f(args...)
    R = <expr>
    # stuff
    return convert(R, <r1>)::R
    # futzing
    return convert(R, <r2>)::R
    # junk
    return convert(R, <r3>)::R
end

The return type expression <expr> is evaluated at runtime like everything else – if it is unpredictable, then it's unpredictable. But as for normal code, when the compiler can predict it, it can be evaluated a compile time.

@KristofferC
Copy link
Member

Thanks, I got it now :)

@omus
Copy link
Member

omus commented May 20, 2016

@JeffBezanson should the return type declaration be checked to ensure that it is a type?

julia> ex1{T}(x::T) :: (T,T) = x-1.0, x+1.0
ex1 (generic function with 1 method)

julia> ex1(2)
ERROR: MethodError: First argument to `convert` must be a Type, got (Int64,Int64)
 in ex1(::Int64) at ./null:0
 in eval(::Module, ::Any) at ./boot.jl:225
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

julia> ex2{T}(x::T) :: 2 = x-1.0
ex2 (generic function with 1 method)

julia> ex2(2)
ERROR: MethodError: First argument to `convert` must be a Type, got 2
 in ex2(::Int64) at ./null:0
 in eval(::Module, ::Any) at ./boot.jl:225
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

@yuyichao
Copy link
Contributor

This can only possibly be checked at runtime and it is effectly checked there already.

@omus
Copy link
Member

omus commented May 20, 2016

Hmm, I have to ask then: why does this throw an exception upon declaration?

julia> ex3{T}(x::(T,T)) = x
ERROR: TypeError: Tuple: in parameter, expected Type{T}, got Tuple{TypeVar,TypeVar}
 in eval(::Module, ::Any) at ./boot.jl:225
 in macro expansion at ./REPL.jl:92 [inlined]
 in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46

julia> ex4{T}(x::Tuple{T,T}) = x
ex4 (generic function with 1 method)

@yuyichao
Copy link
Contributor

yuyichao commented May 20, 2016

The argument types have to be evaluate at definition time (otherwise you'll have to execute arbitrary code during dispatch). It is certainly possible to add that constraint to return type declaration too but it would be less useful (or at least you can't call promote_type in it anymore)

@JeffBezanson
Copy link
Member Author

Because the signature "runs" as part of the method definition. You can't add a method at all unless you have a type to sort it under.

We could evaluate the return type at definition time too, requiring it to be a well-formed type. But (1) this would prevent uses like ::promote_type(T,S), (2) we have no need for it --- the system would take the constructed type and just throw it away.

@omus
Copy link
Member

omus commented May 20, 2016

Thanks @yuyichao and @JeffBezanson. Return types are more powerful than I realized.

@tkelman
Copy link
Contributor

tkelman commented May 21, 2016

Would you expect this to have an effect on memory consumption? AppVeyor has failed with memory issues in LLVM twice in a row now:
https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.1871/job/ob5hf5h4s870vbya
https://ci.appveyor.com/project/JuliaLang/julia/build/1.0.1886/job/mbg2cddhs68334j7

Maybe bad luck? edit: 3rd time was the charm apparently...

@JeffBezanson
Copy link
Member Author

Merge?

@vtjnash
Copy link
Member

vtjnash commented May 22, 2016

This seems to be using quite a bit more memory (est. 10% 5% from glancing at 3 6 appveyor logs)? Presumably that shouldn't have been the case since this isn't used right now. Perhaps that indicates there's something wrong with the lowering here?

@JeffBezanson
Copy link
Member Author

Seems worth looking into. I looked around, but so far I haven't found any examples that this lowers differently. However I did find a small problem; PR updated (I realized I should use meta instead of adding yet another node type that might need to be handled during lowering.)

@hayd
Copy link
Member

hayd commented May 24, 2016

Being greedy: I wonder if Void should be special cased https://groups.google.com/forum/#!topic/julia-users/4RVR8qQDrUg

function f!()::Void ...

@quinnj
Copy link
Member

quinnj commented May 30, 2016

Oh man, I thought this had been merged already! I coded up an entire new package using the syntax. Excited for this!

@quinnj
Copy link
Member

quinnj commented May 30, 2016

So I'm seeing an interesting interaction with docs with this change,

julia> "test doc Int"
       test1(x::Int) = 2x
test1

julia> "test doc Float64"
       test1(x::Float64) = 4x
test1

julia> "test doc Int"
       test2(x::Int)::Int = 2x
test2

julia> "test doc Float64"
       test2(x::Float64)::Float64 = 4x
WARNING: replacing docs for 'test2 :: Union{}'.
test2

note that for test1, there is no warning, but for some reason, there is one for test2 when the return types are added. I'm also not sure where test2 :: Union{} is coming from.
Maybe cc @MichaelHatherly

@MichaelHatherly
Copy link
Member

MichaelHatherly commented May 30, 2016

Probably just needs some minor updates to @doc to handle the new syntax I think. I'll build the branch later this evening to actually check though.

MichaelHatherly added a commit to MichaelHatherly/julia that referenced this pull request May 30, 2016
E.g.

    "..."
    f{T}(x::T)::T = x

Fixes problem mentioned in

    JuliaLang#16432 (comment)
@MichaelHatherly
Copy link
Member

Was a simple fix as far as I can tell: jb/rettype...MichaelHatherly:mh/fix-doc-rettype

Can either be cherry picked to this branch or I can open a separate PR, I don't mind which.

@JeffBezanson JeffBezanson merged commit a4c419b into master Jun 1, 2016
@yuyichao yuyichao deleted the jb/rettype branch June 1, 2016 00:14
@diegozea
Copy link
Contributor

diegozea commented Jun 1, 2016

This's awesome! :)

@MikeInnes
Copy link
Member

MikeInnes commented Jun 1, 2016

In the gitter we were discussing how to make it more convenient to return Void from a function, i.e. to completely ignore the function's return value:

pushone!(xs)::Void = push!(xs, 1) # currently an error

You can actually do this already by defining convert(::Type{Void}, x) = nothing but of course the change to weaker typing everywhere is unsatisfying.

One solution might be to have a return_convert function which can special-case Void and fall through to convert everywhere else. That would only affect return values and potentially generalises to other types as well, if that's useful.

Just a suggestion anyway.

@quinnj
Copy link
Member

quinnj commented Jun 7, 2016

Would it be terrible if we just had a definition of

convert(::Type{Void}, x) = nothing

?

Seems like that would allow what @MikeInnes said without needing an extra return_convert method.

@quinnj
Copy link
Member

quinnj commented Jun 7, 2016

Oh, I read @MikeInnes comment in email and it looks like he edited to include this exact suggestion. Sorry for the noise.

@stevengj
Copy link
Member

As discussed on the mailing list, this doesn't actually seem to be using convert(T, ret)::T as @JeffBezanson claimed at the top. It is apparently just using convert(T, ret) and not checking whether convert returns the claimed type.

Can we add the typeassert please? In addition to checking for buggy convert methods, this would also help type inference in cases where Julia can't figure out the return type of convert on its own.

@JeffBezanson
Copy link
Member Author

Not sure how that happened. Will fix.

@StefanKarpinski
Copy link
Member

x-ref: #18899

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.