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

New function type syntax #23

Merged
merged 6 commits into from Oct 11, 2017

Conversation

nadako
Copy link
Member

@nadako nadako commented Jul 5, 2017

(name:String, ?age:Int) -> Void

Rendered version

@RealyUniqueName
Copy link
Member

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
Copy link
Member Author

nadako commented Jul 6, 2017

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
Copy link
Member Author

nadako commented Jul 6, 2017

That might be an option!

nadako added a commit to nadako/haxe that referenced this pull request Jul 6, 2017
@nadako
Copy link
Member Author

nadako commented Jul 6, 2017

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
Copy link

FrancisBourre commented Jul 7, 2017

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
Copy link

kobi2187 commented Jul 7, 2017

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
Copy link
Member Author

nadako commented Jul 7, 2017

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
Copy link
Member Author

nadako commented Jul 7, 2017

(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
Copy link
Member Author

nadako commented Jul 7, 2017

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
Copy link
Member Author

nadako commented Jul 7, 2017

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
Copy link
Member

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
Copy link

delahee commented Sep 26, 2017

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
Copy link
Member

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

@Gama11
Copy link
Member

Gama11 commented Sep 26, 2017

@RealyUniqueName That was reverted again: HaxeFoundation/haxe@84615ed

@RealyUniqueName
Copy link
Member

Cruel world!

@nadako
Copy link
Member Author

nadako commented Sep 26, 2017

@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
Copy link
Member Author

nadako commented Sep 26, 2017

@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
Copy link
Member Author

nadako commented Oct 5, 2017

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:

  • (A,B)->C
  • (a:A,B)->C
  • etc.

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
Copy link
Member Author

nadako commented Oct 5, 2017

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
Copy link
Member Author

nadako commented Oct 6, 2017

Implementation PR ready HaxeFoundation/haxe#6645

@ncannasse
Copy link
Member

Good for me

@Simn
Copy link
Member

Simn commented Oct 11, 2017

Good for me too

@nadako
Copy link
Member Author

nadako commented Oct 11, 2017

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

@ncannasse
Copy link
Member

ncannasse commented Oct 11, 2017 via email

@nadako
Copy link
Member Author

nadako commented Oct 11, 2017

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

@Simn
Copy link
Member

Simn commented Oct 11, 2017

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

@nadako
Copy link
Member Author

nadako commented Oct 11, 2017

Agreed.

@ncannasse
Copy link
Member

ncannasse commented Oct 11, 2017 via email

@nadako nadako merged commit 0b7a2dd into HaxeFoundation:master Oct 11, 2017
@nadako nadako deleted the new_function_type_syntax branch October 11, 2017 21:32
@RealyUniqueName
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants