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

Fix label evaluation order #12720

Merged
merged 4 commits into from
Nov 9, 2023
Merged

Conversation

garrigue
Copy link
Contributor

@garrigue garrigue commented Nov 6, 2023

Alternative to #10653, preserving the delaying of partial applications of optional arguments to avoid code blow up.

@garrigue
Copy link
Contributor Author

garrigue commented Nov 6, 2023

I again checked the sizes for lablgtk, but in this case they even seem to slightly decrease compared to 5.1.0.

Copy link
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

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

This PR fixes two bugs: a major one about delaying evaluation of optional arguments, and a minor one about evaluation order being inconsistent with normal applications.

But I still think the handling of optional arguments followed by missing arguments is wrong: the reference test should produce the output from #10653, not the one from this PR. Recovering efficient partial applications should only be done later, when information about the function's actual arity is known (typically in the middle-end).

I would be willing to approve this if the test contains a comment explaining that the produced result is not yet correct. Then we could work on replacing the current optimisation by a more correct one in another PR.

@garrigue
Copy link
Contributor Author

garrigue commented Nov 8, 2023

@lthls As I commented in #10653, I beg to differ here.
The point is that if we follow the convention that any block of optional parameters must be followed by a non-optional one, without side-effects between them, then you wouldn't be able to see the difference. And n-ary abstractions make sure that the code computing the defaults is indeed not inserted between them.
I agree that all this should be properly written down, but I think the behavior here is correct, i.e. undistinguishable from #10653 for all useful purposes.

@garrigue
Copy link
Contributor Author

garrigue commented Nov 8, 2023

By the way, I just discovered something strange about n-ary functions.

let f ?(a=print_endline"a") ?(b=print_endline"b") = function c -> c

is a function of 3 arguments, and the defaults are computed after receiving c, but

let f ?(a=print_endline"a") ?(b=print_endline"b") = fun c -> c

is a function of two arguments returning a function, and the defaults are computed after receiving b. The difference seems strange to me. Is it really useful ?

@xavierleroy
Copy link
Contributor

This looks like the "syntactic arity" convention (https://github.com/ocaml/RFCs/blob/master/rfcs/n_ary_functions.md) that was recently implemented. Like all conventions, it can look strange sometimes, but I think that knowing (syntactically) how the compiler is going to handle a curried function definition is useful. I don't know if syntactic arity can help with the issue discussed here and in #10653, as I haven't been able to follow the discussion.

Copy link
Contributor

@lthls lthls left a comment

Choose a reason for hiding this comment

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

I think we should go ahead and merge this. We'll change the reference output of the test later if we fix the other bug.

@garrigue
Copy link
Contributor Author

garrigue commented Nov 8, 2023

@lthls Thanks for the approval. I intend to add some comments explaining what this code does before merging (it took me a while to remember the details, as it was written 20 years ago). The behavior may be open to debate, but it is the intended one.

@xavierleroy Syntactic arity alone does not solve the problem, but if we combine it with the requirement that any n-ary abstraction must be terminated by a non-optional argument, which is the case in any reasonable code (except combinators used to build functions by combining sets of optional arguments), then there is no discrepancy with #10653, since side-effects are then guaranteed to only occur after a non-optional argument. That is why I suggested introducing a warning for that.
Note that the incriminating example in #10653 already disables another warning checking for the presence of a non-labeled parameter after optional ones, so it may not even be needed to add a new warning.

@garrigue garrigue merged commit efd7322 into ocaml:trunk Nov 9, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants