Skip to content

Enforce evaluation order for generic applications in Closure#13882

Merged
lthls merged 2 commits into
ocaml:trunkfrom
lthls:closure-generic-application-order
Mar 28, 2025
Merged

Enforce evaluation order for generic applications in Closure#13882
lthls merged 2 commits into
ocaml:trunkfrom
lthls:closure-generic-application-order

Conversation

@lthls
Copy link
Copy Markdown
Contributor

@lthls lthls commented Mar 19, 2025

Fixes #13874, by enforcing evaluation order for function calls in Closure.

I have an alternative branch here which binds the arguments in Cmmgen instead. It is a bit simpler because it doesn't change much existing code, but I prefer this one because it allows some factorisation with the direct application case.

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 19, 2025

This kind of change is always delicate because we risk breaking user code by changing the evaluation order -- which they shouldn't rely on, but they do. On the other hand, if I understand correctly flambda already enforces this evaluation order, and the big large scary codebases that we shouldn't break presumably already tested that they work with flambda.

I'll put this change as a topic for discussion in our upcoming maintainer meeting.

@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Mar 20, 2025

It's changing the evaluation order of indirect calls with Closure to be the same as direct calls with Closure (which is also the same as all calls in bytecode and flambda). I don't think it is a very risky move, particularly since effects/coeffects on the function side are not very common, but there's no harm in discussing this at the meeting.

@gasche
Copy link
Copy Markdown
Member

gasche commented Mar 27, 2025

This needs a Changes entry I suppose?

@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Mar 28, 2025

Do you think I should mark it as a potentially breaking change (with * instead of -) ?

@nojb
Copy link
Copy Markdown
Contributor

nojb commented Mar 28, 2025

Do you think I should mark it as a potentially breaking change (with * instead of -) ?

Yes, I think so.

@lthls lthls force-pushed the closure-generic-application-order branch from 2268483 to ca89e30 Compare March 28, 2025 11:36
@lthls lthls merged commit 895f2ae into ocaml:trunk Mar 28, 2025
clementblaudeau pushed a commit to clementblaudeau/ocaml that referenced this pull request Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Evaluation order: ocamlopt difference on application of mutable field

3 participants