- Notifications
You must be signed in to change notification settings - Fork471
Add Parsetree-level Option stdlib optimizations (forEach/map/flatMap)#7918
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pkg-pr-newbot commentedSep 22, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
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.
This is going to be unsafe: Option.x could be anything.
We won't know what moduleOption we're talking about until the typed tree.
Also, aliases will be resolved by then.
We already have code in typecore.ml (I think) that checks exactly that: is this id the path I expect from predefined types. (Or in this case I guess, from sodlib).
There's no big reason it should be much harder. Worst case one can type it, then figure out the optimisation, then go back to the untyped type tree, modify, and re-type.
That would be the emergency solution if it turns out to be too hard.
f1692ff to4496782Compare4496782 to50dbc3cCompare@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
| let transform (expr : Parsetree.expression) : Parsetree.expression = | ||
| match expr.pexp_desc with | ||
| | Pexp_apply | ||
| { | ||
| funct = | ||
| { | ||
| pexp_desc = | ||
| Pexp_ident | ||
| {txt = Ldot (Lident ("Option" | "Stdlib_Option"), fname)}; | ||
| }; | ||
| args = [(_, opt_expr); (_, func_expr)]; | ||
| } -> ( |
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.
[P1] Guard rewrite against user-definedOption modules
The optimization currently rewrites any application whose head is syntacticallyOption.* orStdlib_Option.* (see theLdot (Lident ("Option" | "Stdlib_Option"), fname) pattern). Because this runs before name resolution, it will also trigger for locally defined modules namedOption orStdlib_Option. If a project defines its own module shadowing these names (for example a customOption.map with different semantics), the pass will silently replace the call with a match onSome/None, miscompiling the user’s code. The transformation needs to confirm it is targeting the stdlib Option module—e.g. by running after type checking or by explicitly checking the module path after resolution—otherwise it is unsound.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading.Please reload this page.
This PR adds a compiler-side optimization for the most important functions from the standard library's
Optionmodule:Option.forEach/map/flatMap.Primitive_option.valFromOptionexactly as before, so the generated JavaScript stays identical. The trade-off is that we can only recognize calls spelled explicitly asOption.*orStdlib_Option.*; aliases or locally opened variants aren’t rewritten.tests/tests/src/option_stdlib_optimization_test.res. The first test in that file gives a good example of how much the compiler output for long pipe chains is simplified.