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

support argument names in function type (closes #4799) #6428

Merged
merged 1 commit into from
Jul 3, 2017
Merged

Conversation

nadako
Copy link
Member

@nadako nadako commented Jul 1, 2017

This PR adds support for specifying argument names in a function type with the following syntax:

arg1:Type1 -> arg2:Type2 -> Ret

This syntax is the most obvious choice as people are already familiar with it because Haxe prints function type with argument names like that.

The parsing part is super-easy and the change is tiny: we parse an identifier, then check if it's followed by a colon (:), then we parse a named argument, otherwise we continue parsing as a type path.

The AST representation takes the same approach as optional args: we add a CTNamed variant next to CTOptional and handle it similarly.

The typing part is the smallest change: when we loop through CTFunction arguments, we filter out CTNamed and extract name from it (again, similar to how we extract optional-ness CTOptional).


Here's why I believe adding this is a good idea, considering how non-invasive this change is:

Self-documenting code. Just compare these two:

typedef GetUserCallback = Int -> String -> Int -> Void;
// vs
typedef GetUserCallback = id:Int -> name:String -> age:Int -> Void;

Good signature hints for callbacks:

IDEs have the info to autogenerate a callback function/method with sensible argument names:

It's useful when we want to generate a function type from a macro. For example, latest node.js API have this util.promisify function that transform a callback-style function into a promise-returning one. With the information about argument names, we can provide a type-safe way of dealing with it:


(note how we transform stdout/stderr from argument names to promise object fields, and how we have the same command argument name for the generated function).


The only thing that's breaking about this change I think is the added ComplexType.TNamed constructor, which I think is fine, considering all the gained benefits and that we're aiming at Haxe 4 which already breaks macros in worse ways :)

nadako added a commit to vshaxe/hxparser that referenced this pull request Jul 2, 2017
@nadako
Copy link
Member Author

nadako commented Jul 3, 2017

If we merge this (I hope we will!), it might make sense to handle CTParent for function type args as well, so one could write var cb:(a:Int)->(b:String)->Void for better readability.

let p1 = snd n in
let p2 = snd t in
CTNamed (n,t),punion p1 p2
| [< s >] ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the parse_type_path2 go in this pattern?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make a difference? I called it outside of the pattern because we need to unpack n to n,p, but we might as well just use (fst n), (snd n). I'm not sure what's better performance-wise.

@Simn Simn merged commit b225e9f into development Jul 3, 2017
@nadako nadako deleted the ctnamed branch July 3, 2017 14:55
@nadako
Copy link
Member Author

nadako commented Jul 3, 2017

Thanks!

Any opinion regarding handling CTParent for named/optional argument types?

Right now the following will fail at Typeload.load_complex_type:

var cb:(?Int)->Void; // this one was failing before as well
var cb:(arg:Int)->Void;
var cb:(?arg:Int)->Void;
var cb:?(arg:Int)->Void;

Do we want to allow (some of) them, or parenthesed types are only meant to distinguish function types in function type arguments?

Parens for named args (e.g. var cb:(id:Int)->(name:String)->Void) would be an option, but I don't have any strong opinion on this.

@Simn
Copy link
Member

Simn commented Jul 3, 2017

I don't know... (arg:Int) -> Void looks like you could also write (arg1:Int, arg2:Int) -> Void. But you can't...

@nadako
Copy link
Member Author

nadako commented Jul 4, 2017

Unfortunately we had to revert this because of a parser regression found #6433. We'll have to take another approach here with a different unambigous syntax.

@back2dos
Copy link
Member

back2dos commented Jul 5, 2017

How about using the short lambda syntax for it (FWIW that's pretty much what TypeScript does)?

var add:(x:Int,y:Int)->Int = (x:Int,y:Int) -> x + y;

Alternatively use argument list + return type syntax:

var add:(x:Int,y:Int):Int = (x:Int,y:Int) -> x + y;

I have no idea how the Haxe parser works, but I think it should be as easy/hard to distinguish from TParent as distinguishing EObjectDecl from EBlock, i.e. on ( look ahead if there's an ident and a : next. If so, assume function type syntax and parse ,-separated ident : ComplexType tuples until hitting ) then expect -> or : for the alternative syntax and parse return type.

Personally I like the arrow form more, since it looks so close to the expression itself.

It also nests fine:

var stat:(name:String,callback:(error:Error, stat:Stat)->Void)->Void = js.Lib.require("fs").stat; 

As for representation (speaking in terms of the macro API to conceal my ignorance) I would just use FunctionArg (with null or empty name to support X->Y->Z syntax) for TFunction which also makes it more similar to haxe.macro.Type.TFun. This would make TOptional obsolete since optionality is encoded with the argument for functions and with @:optional for fields.

@nadako
Copy link
Member Author

nadako commented Jul 5, 2017

How about using the short lambda syntax for it

Yeah that's what we coming up with as well, I'm currently preparing an evolution PR because this is a larger change.

@nadako
Copy link
Member Author

nadako commented Jul 6, 2017

For anyone interested in this, there's an evolution proposal now: HaxeFoundation/haxe-evolution#23

Simn added a commit that referenced this pull request Sep 12, 2017
* Actually disable the tests

* [java/cs] Make sure that cast new NativeArray uses the right type parameters

Closes #5751

* Fix test

* [cs] Only generate casts on NativeArray if the type parameters are not the same

Fixes fast_cast define

* Fix #6375 (#6376)

* also check tempvar'd this field access when typing getter/setter (closes #6375)

* add test

* [java/cs] Make sure to collect the right type parameters on anonymous functions

Closes #5793

* [neko] fix Sys.programPath() in nekotools booted exe (close #5708)

* don't remove enumIndex optimizations because EnumFlags is super important

* [TravisCI] fix boolean logic for neko installation

* [TravisCI] Fix neko installation on OSX

* Treat object array sets as having gc references

* [appveyor] merge php tests with the others

* [php7] fix haxe.io.Input.readAll() with disabled analyzer optimizations (closes #6387)

* changelog

* [php7] fix accessing static methods of String class stored into a variable (#4667)

* [php7] replace `Syntax.binop(.=)` with more analyzer-friendly `= Syntax.binop(.)`

* fixed stacks not being correctly reset in case of not unify detection (close #6235)

* add native mysqli error message when throwing an exception (#6390)

* use one-based error format, add -D old-error-format to keep zero-based behavior (#6391)

* [lua] old string metatable index is a function, not a table. fixes #6377

* [lua] previous string __index can be a table or a function

* fix position tests (see #6391)

* fix import.hx diagnostics condition (closes #6382)

* deal with EDisplay in assignment patterns (closes #6381)

* small toplevel collector cleanup (is going to help with #6341)

* filter duplicate toplevel identifiers (closes #6341)

* [eval] catch Failure on all Process calls (closes #6333)

* [eval] implement missing Date.fromString formats (closes #6323)

* print usage to stdout instead of stderr (closes #6363)

* [make] try to fix build directory dependency problem

* [lexer] count braces in code strings (closes #6369)

* [dce] keep everything if a class extends an extern (closes #6168)

* [display] delay metadata check behind field typing (closes #6162)

* fix test

* [matcher] fix assignment pattern display differently (closes #6395)

* fix json range column positions (back to zero-based after nicolas change)

gotta look into eval position stuff and check there too

* [java] Initial support for treating Null<BasicType> as their actual Java counterparts

* [java] java.lang.Long is also a boxed type

* [java] Properly rewrite basic type parameters as their boxed counterparts

Closes #784
Related #3728

* [java/cs] Add test

Closes #2688

* [cs] Automatically set nativegen to structs, and enforce that they cannot be subclassed, nor contain a constructor with no arguments

Closes #5399

* [java/cs] Make sure to reset old values when resizing a map

Closes #5862

* [java/cs] Avoid boxing with Runtime.compare expressions on dynamic field access

Closes #4047

* [java/cs] Avoid boxing when comparing Null<> basic types

Closes #4048

* [cs] Add C# optional arguments on Haxe-defined functions that have optional arguments

Closes #4147

* [java/cs] Prepare the code for the abstract null type PR

* [java/cs] Cast field field accesses to type parameters to their implementation instead of going through reflection

Closes #3790

* [cpp] No FastIterator for cppia

* [TravisCI] fix "Please check that the PPA name or format is correct."

* add test (closes #6215)

* [hl] disable failing test

* [java] disable failing test

* [parser] don't make reification call positions span the entire expression (closes #6396)

* Revert "[cs] Add C# optional arguments on Haxe-defined functions that have optional arguments"

This reverts commit 261e73c.

* don't use global status var for newly created anon types (closes #6400)

* [lua] fix lazy function type handling for anon method binding (#6368)

* Make _build source directory on demand

* [Makefile] always generate version.ml

such that the version info is always up-to-date

* ignore haxelib run.n changes

* [macro] add haxe.macro.Expr.TypeDefinition.doc field and support printing it with haxe.macro.Printer (#6401)

* Change Null<T> to an abstract (#6380)

* use abstract instead of typedef for Null<T>

* make Null<T> a @:coreType (broken on C#/Java)

* fix infinite recursion

* fix flash

* fix overloads

* Do not dce fields which override extern fields (discussion in 23f94bf) (#6168)

* [eval] fix inconsistent error formatting (#6410)

see vshaxe/vshaxe#138

* [cs/java] add an expr filter that optimizes Type.createEmptyInstace when the type is known

this helps avoid redundant casting overhead on c# when type parameters are involved (e.g. `(Type.createInstance(C) : C<Int>)` becomes `new C<int>(EMPTY)`

* Changed is_extern_field to is_physical_field (#6404)

* changed is_extern_field to is_physical_field

* fix is_extern_field->is_physical_field change in genlua

* [js] reserve flattened paths for variable renaming (closes #6409)

* [inliner] only unify_min if we go from no-type to some type (closes #6406)

* [display] don't show Module.platform.hx in import completion (closes #6408)

* [java] disable failing test

* fix comments in php7.Syntax

* [macro] flush pass in typing-timer

Also make `cast_of_unify` go through typing-timer. Closes #6402

* [parser] fix binop position spanning (closes #6399)

* [display] respect `@:noCompletion` for toplevel (closes #6407)

* Use Meta.Pos instead of Meta.Custom ":pos" (#6419)

That way, it shows up in --help-metas.

* [parser] fix more positions (closes #6416) (closes #6417)

* [display] don't show non-static abstract fields via using (closes #6421)

* [typer] disallow accessing non-static abstract field statically (closes #5940)

* [display] do not generate property accessor calls if we are displaying the property (closes #6422)

* TIdent (#6424)

* add TIdent

* remove Meta.Unbound and break everything

* first replacement batch

* fix neko/hl

* fix java/cs

* fix cpp/php and maybe lua

* fix cppia and maybe fix lua (again)

* remove unused NoPatternMatching define

* support specifying platforms for defines (so they are mentioned in --help-defines)

* [dce] don't add @:directlyUsed to a class if it's used from within its methods (fixes #6425)

* add some platform tags to define info

* [gencommon] remove unused fix_tparam_access argument

* [lua] introduce @:luaDotMethod for using dot-style invocation on appropriate extern instances

* [cpp] Split CppGlobal to allow untyped identifiers as externs. Type the __trace function more strongly

* [cpp] Do not use GC memory for const enums

* support argument names in function type (closes #4799) (#6428)

* [lua] fix inconsistent function assignment (#6426)

* Revert "support argument names in function type (closes #4799) (#6428)"

This reverts commit b225e9f.

* [php7] implemented Lib.mail()

* [php7] php.Lib.rethrow()

* [php7] Global.require(), Global.include()

* [php7] implemented php.Lib.loadLib()

* [php7] implemented php.Lib.getClasses() (closes #6384)

* [lua] generate prototypes with plain field access, rather than through _hx_a(...)

* [lua] drop unused param

* [lua] ocp-indent

* [cpp] Use stack allocations to get class vtables

* [cpp] No need to mark class statics if there are none

* [cpp] Fix hasMarkFunc condition

* [lua] fix multireturn return type

* [TravisCI] move osx_image to matrix

* [eval] catch a Break (closes #6443)

* [eval] support extending special classes (closes #6446)

* [display] check field display on extern function fields (closes #6442)

* [typer] don't lose return expressions even if they error (closes #6445)

* fix test

* make -D old-error-format deal with display completion being sent in bytes instead of chars

* [js] apply var escaping rules to function arguments as well (fixes #6449)

* [TravisCI] use brew bundle to install brew formulae

It avoids getting "xxx already installed" error.

* [lua] use _hx_string_wrapper helper function instead of modifying string prototype

* [lua] wrap string operations so that metatable changes are not necessary

* (HL) Fixed non-blocking SSL Socket connect (#6452)

* added getThis() in jquery event, allow to easily get real this in a callback

* [lua] faster std Math min/max (#6455)

ternary expressions seem to be faster when not part of an inline

* Add jit option to cppia host

* Add alias 'Single' for cpp.Float32.  Closes HaxeFoundation/hxcpp#555

* Check for null object in Reflect.callMethod.  Closes HaxeFoundation/hxcpp#593

* [js] consider @:native type names when collecting reserved names for local vars (see #6448)

* [js] support unqualified static field access for @:native("") classes (see #6448)

* [js] consider statics of @:native("") classes when renaming local vars (closes #6448)

* Disable extra cppia test until I can debug

* oops, add #if

* [hl] fixed Type.getClass with interface/virtual input

* [js] Add compiler switch to generate enums as objects instead of arrays (#6350)

* add -D js_enums_as_objects switch

* add $hxEnums, change __enum__ to String

* whitespace

* resolve genjs.ml conflict

* add -D js-enums-as-objects to tests

* [lua] fix/simplify Rex extern

* [lua] one more Rex tweak

* fix haxe.web.Dispatch

* [cpp] Remove some scalar casts when not requires, and add some to reduce warnings.  Closes HaxeFoundation/hxcpp#485

* [jQuery] regenerate extern with added Event.getThis()

andyli/jQueryExternForHaxe@4a6e3d5

* [TravisCI] install a patched ptmap for Mac builds

* [TravisCI] use the hererocks in python3 in mac builds

* use abstract field resolution instead of "implements Dynamic" for haxe.xml.Fast

this actually makes more lightweight due to less fields/allocations

* move haxe.web.Dispatch to hx3compat

* move magic types support code into their own module

* [java/cs] Fix IntMap implementation when setting 0 keys

Reviewed the whole Map implementations, added a few more comments
Closes #6457

* Disable toString test on C++

* [java] Add `no_map_cache` checks on WeakMap, and reimplement its iterators as classes

* Do not deploy binaries if we're running a PR

This only happened on PRs that were made by HF members, but still
we certainly don't want to submit them as nightlies

* [swf] rewrite safe-casts to if in non-retval mode

This is stupid. But it fixes #6482.

* [inliner] object declarations are _not_ constants

closes #6480

* disable failing test (see #6482)

* [eval] fix FileSystem path patching (closes #6481)

* [js] output file:line for traces (closes #6467)

* update submodule

* Add copy() to DynamicAccess (#5863)

* [xml] mention haxe.rtti.XmlParser (closes #6451)

* [display] allow dots in paths (closes #6435)

* [display] don't show private types in toplevel display (closes #6434)

* remove unused MSub module kind

* [matcher] print deprecation messages for enum (fields) (closes #6485)

* [parser] patch EField positions in reification (closes #6423)

* fix gadt pattern matching bug, closes #6413

* add interface tests

* [display] show constructor signatures on `new` (closes #6275)

* more local #if java for TestInterface

* make `in` a Binop and remove `EIn` (closes #6224) (#6471)

* make `in` a Binop and remove `EIn` (closes #6224)

* use make_binop

* fix enumIndex in tests

* change OpIn precedence to be lessthan OpAssign(Op)

* [js] fix tracing (closes #6492)

* minor

* Update JsonParser.hx (#6493)

haxe.format.JsonParser.parse("{\"\"\"A\":\"B\"}") -- any strings (empty string for sample), that are not separated by commas.

* add test for #6493

* [js] some doc on Symbol, add ofObject utility method and the standard `iterator` static field.

* supposedly fix #6491

* temp fix for #6467

* [js] setup proper keyword list based on selected ES version

* use rev_file_hash instead of rev_hash_s for eval position reporting so that it reports original file instead of unique_full_path

this also fixes positions multibyte utf8 files because original files are stored in lexer cache

* haxe now uses 1-based columns, so no need to +1 that in the eval debugger

* Split the Json parse test from the lambda arrow test until it is fixed.  Add shorcut for cpp on linux

* Add some additional cppia tests

* [make] don't add -native to ocamldep in bytecode mode

* casually walking into Mordor

* more comments

* more comment

* [lua] fix various coroutine-related signatures

* [lua] if dynamic is not table, return empty array for Reflect.fields

* [lua] Table helper methods

* [lua] do not generate function wrapping for explicit nulls (#6484)

* Make cppia use new ast by default.

* Update the docs for @:expose (#6515)

It works for Lua as well, and for both classes and fields.

* Fix duplicated platform in @:phpGlobal docs (#6518)

* [lua] fix Compiler.includeFile() with position = Inline

`haxe.macro.Compiler.includeFile("test.lua", "inline");` made `__js__` appear in the Lua output, leading to a rather funny error:

>attempt to call global '__js__' (a nil value)

* more comments for handle_efield

* remove weird loop from handle_efields. tests still pass, let's see what travis has to say.

* revert 36ea42d. this actually does something for untyped...

* more comments for handle_efield, also move the [] case to the bottom of the match for easier reading

* more comments

* add comment about the weird loop now when I get it :)

* more comments, also move the inner `loop` function closer to its usage

* add a couple more comments to handle_efield, remove unused argument from check_module (as it uses sname)

* add common decl_flag type for parsing common type flags instead of returning tuples of class+enum flag and then mapping it awkwardly

* Update @:selfCall docs to include Lua (#6523)

* Add a VSCode settings.json (#6524)

It's common practice to commit .vscode settings (eventually, it might be good to have a tasks.json and a launch.json too).

The first two are necessary to work with the vscode-ocaml extension. The files.exclude pattern prevents always ending up in _build files instead of the actual source files by accident (for instance via Ctrl+P, which used to show both).
Note that F1 -> Clear Editor History might be necessary for Ctrl+P not to show _build files anymore since it shows recently opened files regardless of the files.exclude setting.

[skip ci]

* Remove spaces around "in" in printBinop (#6527)

* [lua] properly escape reserved keywords when used as prototype declarations (#6528)

* [lua] convert all abort directives to error, in order to emit normalized line number/column info (#6519)

* [lua] change "failwith" to "error" for consistent error reporting

* [lua] speed up some array methods

* Revert "[lua] speed up some array methods"

This reverts commit f11ed3c.

* fix OpIn precedence: it's actually the lowest one. closes #6531

* Date.fromTime expects milliseconds (#6530)

* [matcher] support `@:forward` (closes #6532)

* Update MainLoop.hx (doc) (#6537)

Typo on call event's doc

* fixed generic getFloat to use FPHelper

* [eval] push block scope even for single-element blocks (see #6543)

* [eval] add constructor for haxe.io.Bytes (see #6543)

* [eval] print position for field index errors (#6543)

* [eval] support setting Bytes.length (see #6543)

* [matcher] don't leak `_` extractor variable (closes #6542)

* [js] create directory on setCustomJSGenerator (closes #6539)

* more binary support for js : use dataview for FPHelper, added dataview based BytesBuffer

* [lua] spacing nit

* [lua] do a better job marking haxe modules and classes as local

* [lua] speed up array methods (take 2)

* [TravisCI] ptmap 2.0.2 is compatible with ocaml 4.05

* [lua] fix unshift for empty array

* [lua] fix jit detection in tests

* [lua] de-inline a lengthy filesystem method

* [lua] fix unshift again

* [lua] fix array.shift for lua 5.3

* [lua] revert back to old array.unshift behavior

* [lua] use lua alias instead of luajit on hererocks installation

* [doc] fix Sys.getChar documentation (closes #6139)

* [lua] small std tweaks

* [js] initially grow BytesBuffer

* [inliner] deal with Dynamic returns (closes #6555)

* [matcher] don't let extractors access bindings (closes #6550)

* [matcher] fix extractor pattern offsets (closes #6548)

* [php7] do not throw on Reflect.getProperty(o, field) if field does not exist (fixes #6659, fixes 6554)

* unit test for #6559

* [cpp] Respect no_debug define

* [matcher] don't forget switch type when optimizing single-case matches (closes #6561)

* [tests] dodge C# issue (see #6561)

* unit test for #6502 (closes #6502)

* fixed some specific cases with empty buffer

* changed underlying TLazy implementation + make it a bit more abstract (allow to know if it's already processed or not)

* bugfix

* - error when accessing type currently being typed in macro
- don't assume not-method fields are not lazy typed
- flush core context (enter in PTypeExpr mode) when accessing a TLazy or calling field.expr()

close #683
close #5456

* I should have wait indeed

* fixed warning

* [TravisCI] install hererocks with --user

* force classes to be build after returning from a build call (same as we do when we load a type) close #6505

* partially revert fix for #6505

* better fix for #6505, do not flush if we are in module build (causes assert in typeload)

* don't consider hidden timers for timer alignment

* [eval] fix debug setVariable

* [inliner] don't mess up non-terminal try/catch (closes #6562)

* CHANGES.txt

* merge waneck-changes from nightly-travis branch
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.

3 participants