-
Notifications
You must be signed in to change notification settings - Fork 445
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
Fix parsing of partial application with pipe #6325
Fix parsing of partial application with pipe #6325
Conversation
| DotDotDot when args <> [] -> | ||
| DotDotDot -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason for this restriction beyond the fact that f(...)
can simple be reduced to f
(if not piped)? Is it safe to remove it?
(ReactEvent.Mouse.t -> unit, [ `Has_arity1 ]) function$ as 'callback | ||
let partialPipe = x |.u ((f ())[@res.partial ][@res.uapp ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the ()
. Try an example and see if it type checks correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, it shouldn't be there. But if I remove it, the above end up as:
let partialPipe = x |.u f
Seems the whole apply
expression is replaced by just the function identifier, which makes sense in itself of course, but that also means the attributes are dropped. Might not be so easy to fix this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes looks like fixing this one is more complicated and might need to involve the processing of the partial attribute later in the compiler.
E.g. conceptually I guess the attribute belongs to |.u
in that example. Or perhaps putting it on f
is fine.
Guess it does not matter much -- just some representation picked up deeper down in the compiler.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Not sure if this is an appropriate fix, but if it is I also need to fix printing.
Fixes #6321