Add Parsetree-level Option stdlib optimizations (forEach/map/flatMap) #7918
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
There was a problem hiding this comment.
This is going to be unsafe: Option.x could be anything.
We won't know what module Option 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.
cknitt
force-pushed
the
options-stdlib-opt
branch
from
f1692ff to
4496782
Compare
cknitt
force-pushed
the
options-stdlib-opt
branch
from
4496782 to
50dbc3c
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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.
[P1] Guard rewrite against user-defined Option modules
The optimization currently rewrites any application whose head is syntactically Option.* or Stdlib_Option.* (see the Ldot (Lident ("Option" | "Stdlib_Option"), fname) pattern). Because this runs before name resolution, it will also trigger for locally defined modules named Option or Stdlib_Option. If a project defines its own module shadowing these names (for example a custom Option.map with different semantics), the pass will silently replace the call with a match on Some/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 👍 / 👎.
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.