Represent optional parameters with defaults structurally instead of by name sniffing - #8580
Merged
Conversation
cristianoc
force-pushed
the
structural-optional-params
branch
from
August 25, 2026 12:04
7dfce46 to
45e154a
Compare
![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.
Reviewed commit: 7dfce46e10
ℹ️ 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
cristianoc
force-pushed
the
structural-optional-params
branch
from
August 25, 2026 12:08
45e154a to
cf9f04c
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8580 +/- ##
=======================================
Coverage 75.93% 75.93%
=======================================
Files 474 474
Lines 62857 62842 -15
=======================================
- Hits 47729 47718 -11
+ Misses 15128 15124 -4
🚀 New features to boost your workflow:
|
cristianoc
force-pushed
the
structural-optional-params
branch
from
August 25, 2026 18:55
cf9f04c to
070c955
Compare
rescript
@rescript/belt
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
…y name sniffing The desugaring of an optional parameter with a default (~x=3) binds the option-carrying parameter to a synthetic *opt_<label>* variable, and that fact was recovered downstream by pattern-matching on the variable name in four places across three compiler layers: typecore's is_fake_let (matching the pre-n-ary name "*opt*" exactly, so it had silently rotted into dead code), matching's *sth* let-elimination peephole, lam_convert's rename_optional_parameters (which also had to re-recognize the whole compiled body shape to rename the parameter to xOpt for JS output), and gentype's *opt prefix check. Represent the fact structurally instead: - Typedtree.function_param gains an fp_has_default field. - type_function replaces the carrier ident right after typing: fp_param becomes a fresh ident named <label>Opt, substituted at the carrier's only two occurrences (the parameter pattern's binder and the synthetic match's scrutinee), both nodes the desugaring itself generated. The unspellable name still exists during typing, where names must be impossible to capture or shadow, but dies before the typedtree leaves the function. The Lambda IR is born with the final parameter name and no residual binding. - lam_convert's rename_optional_parameters and is_opt_param_name are deleted; the Lfunction case is a plain conversion. - matching's *sth* peephole is generalized to eliminate any alias binding of the form let v = arg in v, with no name test. - gentype uses fp_has_default instead of sniffing the ident name. - is_fake_let now keys on the #optional_arg_default attribute the desugaring plants, restoring its intended behavior: an unused defaulted parameter warns as an unused parameter (27), not an unused let (26). - The write-only #default attribute is removed, along with its dead parsetree consumer. Visible improvements: parameter names in emitted JS now consistently derive from the label, including cases the old shape-match silently missed and leaked mangled names for (mario_game.mjs's make$2 had $staropt_id$star as a parameter; it is now idOpt). Signed-Off-By: Cristiano Calcagno <ccrisccris@gmail.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PCtQiaDijUqA2fujQXvKUw
cristianoc
force-pushed
the
structural-optional-params
branch
from
August 25, 2026 19:22
070c955 to
ab19aea
Compare
|
Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8580 |
cknitt
approved these changes
Aug 26, 2026
cknitt
left a comment
cknitt
left a comment
Member
There was a problem hiding this comment.
Nice improvement to the JS output! Tested successfully in a large project.
| #### :house: Internal | ||
|
|
||
| - Give nominal variants one canonical runtime layout: compute their JavaScript representation once after typing each declaration, replace positional constructor tags with semantic runtime descriptors, and make construction, matching, coercion, printing, diagnostics, and GenType consume the stored representation instead of reinterpreting annotations. Pattern matching keeps occurrence-specific plans local without adding another Lambda or Lam expression form. https://github.com/rescript-lang/rescript/pull/8579 | ||
| - Represent optional parameters with defaults structurally instead of recovering them downstream by pattern-matching on the synthetic `*opt_<label>*`/`*sth*` variable names: `Typedtree.function_param` gains an `fp_has_default` field, the type checker gives the compiled parameter its final `<label>Opt` ident before the typedtree leaves the function, and the name-sniffing consumers (`lam_convert`'s shape-matching rename pass, `matching`'s `*sth*` peephole, gentype's prefix check, and typecore's dead `is_fake_let`/`#default` vestiges) are deleted or generalized. Parameter names in emitted JS now consistently derive from the label, including cases the old shape-match silently missed and leaked mangled names for (`$staropt_id$star` → `idOpt`), and an unused defaulted parameter warns as an unused parameter (27) rather than an unused let binding (26). The CMT magic number is bumped to `Caml1999T025`. https://github.com/rescript-lang/rescript/pull/8580 |
Member
There was a problem hiding this comment.
This changelog entry is a bit lengthy, could be trimmed down a bit.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The desugaring of an optional parameter with a default (
~x=3) binds the option-carrying parameter to a synthetic*opt_<label>*variable, and that fact was recovered downstream by pattern-matching on the variable name in four places across three compiler layers:typecore'sis_fake_letmatched the pre-n-ary name*opt*exactly, so it had silently rotted into dead code when the naming changed — nothing failed, the behavior just disappeared.matchinghad a peephole keyed on the name*sth*.lam_convert'srename_optional_parameterssniffed the*optprefix and re-recognized the entire compiled body shape (LletofLifthenelse(Pis_not_none, …)) to rename the parameter toxOptfor JS output.*optprefix to show the label in TS output.What this PR does
Represents the fact structurally and deletes all the name sniffing:
Typedtree.function_paramgains anfp_has_defaultfield.type_functionreplaces the carrier ident right after typing:fp_parambecomes a fresh ident named<label>Opt, substituted at the carrier's only two occurrences (the parameter pattern's binder and the synthetic match's scrutinee) — both nodes the desugaring itself generated a few lines earlier. The unspellable name still exists during typing, where names must be impossible to capture or shadow, but dies before the typedtree leaves the function. The Lambda IR is born with the final parameter name and no residual binding, solam_convertperforms no work for this feature (a step toward Lambda and Lam converging).lam_convert'srename_optional_parameters/is_opt_param_nameare deleted; theLfunctioncase is a plain conversion.matching's peephole is generalized to eliminate any alias binding of the formlet v = arg in v, with no name test.fp_has_defaultinstead of sniffing the ident name.is_fake_letnow keys on the#optional_arg_defaultattribute the desugaring plants, restoring its intended behavior: an unused defaulted parameter warns as an unused parameter (27), not an unused let (26).#defaultattribute is removed, along with its dead parsetree consumer.Visible improvements
Parameter names in emitted JS now consistently derive from the label, including cases the old shape-match silently missed and leaked mangled names for —
mario_game.mjs'smake$2had$staropt_id$staras a parameter name; it is nowidOpt.~label as name=defaultparameters are now named after the label (bb_offOpt) rather than the inner binder.Tests
make test(ounit, build tests, super_errors, runtime),make test-gentype,make test-analysis,make test-tools,make test-syntaxall pass;make checkformatclean. Stdlib and Belt recompile with zero JS output diffs; the only snapshot change is themario_game.mjsimprovement above.🤖 Generated with Claude Code
https://claude.ai/code/session_01PCtQiaDijUqA2fujQXvKUw