HaxeFoundation / haxe-evolution

Repository for maintaining proposal for changes to the Haxe programming language
111 stars 58 forks source link

New function type syntax #23

Closed nadako closed 6 years ago

nadako commented 7 years ago
(name:String, ?age:Int) -> Void

Rendered version

RealyUniqueName commented 7 years ago

Great addition to Haxe syntax. Currently i have to write typedefs with docblocks for function signatures in public API. If a proper error messages from compiler and a tool for automatic conversion can be provided then i'd vote for removing old syntax completely in next major release.

nadako commented 7 years ago

Maybe during the transitional period the code should give a hint to the compiler about its awareness of the new syntax?

That's an interesting idea. I think it could be nicely implemented in the parser similar to conditional compilation (#syntax new_function_type or something), but whether we actually want that is what I'm really not sure about. Personally I think I dislike this because it looks quite ugly and I'd love my fresh Haxe code to be clean :) Alternatively, we could have per-directory settings, similar to how import.hx works, but again, I'm not sure it's worth it...

nadako commented 7 years ago

That might be an option!

nadako commented 7 years ago

If anyone wants to play with the syntax, I hacked up a proof-of-concept implementation in a branch here: https://github.com/nadako/haxe/tree/new_function_type_syntax (not PR quality, but I think it at least parses correctly).

FrancisBourre commented 7 years ago

I really like it, and more than that, it was one of our need (in my company). So readable for a system that relies on method signature injection. Thanks for it.

kobi2187 commented 7 years ago

Can this also handle (or pave the way) for first class (in syntax) tuple support? which is also a way to return multiple arguments.

nadako commented 7 years ago

let's not lose the focus here. tuples are different topic. i can see tho that if we'll have unnamed arguments, there's gonna be ambiguity with popular tuple type syntax, e.g. (Int,Int)->Void is it a single-tuple-arg function or two-int-args function?

nadako commented 7 years ago

(Int,Int)->Void - two ints ((Int,Int))->Void - one tuple

This is inconsistent if we want to support Int->Int for single-arg function types (which we kind of want to, because that's what we have in arrow func expression syntax).

nadako commented 7 years ago

It's not about single-element tuples, but single-argument-type without parenthesis:

E.g. if (in the new syntax) we allow Arg->Ret to mean (Arg)->Ret (just like x->x means (x)->x in arrow funcs), then we have a problem with (T,T)->Ret: is this a single-tuple-arg function or two-arg function? We should avoid this kind of confusion.

nadako commented 7 years ago

but not for the tuples

This inconsistency is what I'm having issue with. And I think it's going to be a PITA for the parser as well (since we parse <type> <arrow> <type> so it's gonna be parsed as a tuple argument, which we'd have to decompose to several arguments...

ncannasse commented 7 years ago

I think we should allow optional names right from the start.

As the spec says, this cause a behavior change with this:

(T) -> A -> B

Which was parsed as T -> A -> B and would now be T -> (A -> B)

Maybe we should require the parenthesizes around functional return types ( A -> B in our case), thus making (T) -> A -> B invalid syntax, to be fixed by writing (T) -> (A-> B), which is actually less misleading wrt the new proposed syntax.

Of course that would also apply to several args types : (a : Int, b : Int) -> (Int -> Bool)

delahee commented 6 years ago

I need those labelled argument badly because node js externs are quite ard to write without them :) Dynamic-> Dynamic -> ( Dynamic -> Dynamic ) are not very expressive and typing them is sometime mistaking one parameter for another.

Thanks !

RealyUniqueName commented 6 years ago

@delahee Looks like labeled arguments are already merged: https://github.com/HaxeFoundation/haxe/pull/6428 This HXE is about changing entire function type syntax.

Gama11 commented 6 years ago

@RealyUniqueName That was reverted again: https://github.com/HaxeFoundation/haxe/commit/84615edc27d651f7bf3ba0acad20db7e6f19036b

RealyUniqueName commented 6 years ago

Cruel world!

nadako commented 6 years ago

@delahee These externs full of Dynamic don't sound good :) Anyway if it really has to be Dynamic (which I doubt), you could have typedef SomeDescriptiveType = Dynamic and then use that type for arguments.

(that of course doesn't mean that we don't need named arguments, it's just how I deal with situations like yours).

nadako commented 6 years ago

@ncannasse But (T)->A->B is valid syntax right now, so it's technically a breaking change (although I don't think there's a lot of code that uses parentheses for a single type like that, so it should be fine).

I haven't worked on this proposal for quite a while, so I'll have to re-read and update it, will try to incrorporate your suggestion, thanks!

nadako commented 6 years ago

So I looked into this again and I think it can work with unnamed arguments indeed.

Here's what I came up with in my prototype implementation:

(A)->B->C is valid syntax and is parsed like before. In general, (A) is parsed like before as CTParent and can be further chained using ->. This is so because we don't want to break things like (Int -> Int) -> Int -> Int.

Supported new syntax:

For single argument, (a:A)->B is parsed as new syntax while (A)->B, A->B are parsed as old syntax, there's no difference in the end.

(A,B)->C->D and (a:A)->B->C is a syntax error (unexpected -> at the second arrow), so mixing two function syntaxes like that is not allowed. If the desired behaviour is a functional return type, parentheses can be used, e.g. (A,B)->(C->D).

I'll update the proposal and prepare an implementation PR with tests for further review.

nadako commented 6 years ago

Alright, I updated the proposal to reflect latest discussions and prototyping. It now supports unnamed arguments without introducing inconsistency or ambiguity (I think).

I'm going to work on polishing the implementation and prepare a PR for the Haxe repo. Meanwhile, I would like the core team to vote (or at least express their opinions) soon, otherwise I'll just merge everything and walk away :)

nadako commented 6 years ago

Implementation PR ready https://github.com/HaxeFoundation/haxe/pull/6645

ncannasse commented 6 years ago

Good for me

Simn commented 6 years ago

Good for me too

nadako commented 6 years ago

Do we also want to emit a warning for old syntax usage (maybe with a -D switch)?

ncannasse commented 6 years ago

No warning, both syntaxes are accepted.

On Wed, Oct 11, 2017 at 3:11 PM, Dan Korostelev notifications@github.com wrote:

Do we also want to emit a warning for old syntax usage (maybe with a -D switch)?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HaxeFoundation/haxe-evolution/pull/23#issuecomment-335804549, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-bwDuMGsXNamvT0JXhtvC9HesZ5Ys0ks5srL6YgaJpZM4OOvbV .

nadako commented 6 years ago

What about type printing for error messages? Should it use old or new syntax?

Simn commented 6 years ago

IMO we should steer towards the new syntax. The old currying syntax really doesn't make much sense.

nadako commented 6 years ago

Agreed.

ncannasse commented 6 years ago

For printing I'm ok to have the new syntax only. We need to keep the old syntax available for backward compatibility. And no warning by default since it would force users to have their code compatible with haxe4 only or use #if / #else which is ugly. We can have a warning with -D if we want to, but I'm not sure it's useful for now.

On Wed, Oct 11, 2017 at 3:20 PM, Dan Korostelev notifications@github.com wrote:

Agreed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/HaxeFoundation/haxe-evolution/pull/23#issuecomment-335806871, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-bwFwF-BdI1ib9OCON1GezzQueTSabks5srMCYgaJpZM4OOvbV .

RealyUniqueName commented 6 years ago

Maybe update "Rendered version" link so it points to an accepted version?