Support record type spreads in inline records #7859
Conversation
compiler/ml/record_type_spread.ml
Outdated
| | _ -> None) | ||
| else [] | ||
|
|
||
| let expand_labels_with_type_spreads ?(return_none_on_failure = false) |
There was a problem hiding this comment.
This is just extracted for reuse, since it's now used in 2 places.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
zth
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes gaps with record type spreads in inline records by improving the parsing and type checking logic to properly handle spread operations in constructor definitions.
- Removes parsing restrictions that prevented record spreads in inline records
- Adds proper type expansion for record spreads in constructor arguments
- Introduces heuristic-based disambiguation between record and object type spreads
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/tests/src/record_type_spread.res | Test cases for record type spreads in variant constructors |
| tests/tests/src/record_type_spread.mjs | Generated JavaScript for record spread tests |
| tests/tests/src/object_type_spreads.res | Test cases demonstrating object vs record spread disambiguation |
| tests/tests/src/object_type_spreads.mjs | Generated JavaScript for object spread tests |
| tests/syntax_tests/data/parsing/errors/typexpr/expected/objectSpread.res.txt | Updated parser error expectations removing old restrictions |
| tests/syntax_tests/data/parsing/errors/other/expected/spread.res.txt | Updated parser output for spread syntax |
| compiler/syntax/src/res_core.ml | Core parser changes for handling spreads in constructor arguments |
| compiler/ml/typedecl.ml | Type declaration logic updates for record spread expansion |
| compiler/ml/record_type_spread.ml | New utility function for expanding record spreads |
| CHANGELOG.md | Changelog entry for the bug fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Nice! Somewhat related issue: #6822 |
Was able to fix that in the latest commit as well. Note for us for later though - we really need to first class the "ambigious spread that needs to be resolved in the type checker" concept soon though. This is all pretty fragile. |
|
@codex check changes to the type checker: |
For now, I can only help with PRs you've created. |
OK will do offline and report this. |
|
[CODEX] Automated review from Codex CLI. Summary of type-checker changes per checklist:
If helpful, I can run the full test suite on this branch and report results. |
![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 suggestions.
Reply with @codex fix comments to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
compiler/ml/typedecl.ml
Outdated
| | Pcstr_tuple l -> | ||
| let l = List.map (transl_simple_type env closed) l in | ||
| (Types.Cstr_tuple (List.map (fun t -> t.ctyp_type) l), Cstr_tuple l) | ||
| | Pcstr_record l -> | ||
| | Pcstr_record l -> ( | ||
| let lbls, lbls' = transl_labels env closed l in | ||
| (Types.Cstr_record lbls', Cstr_record lbls) | ||
| let expanded = | ||
| Record_type_spread.expand_labels_with_type_spreads env lbls lbls' | ||
| in | ||
| match expanded with | ||
| | Some (lbls, lbls') -> (Types.Cstr_record lbls', Cstr_record lbls) | ||
| | None -> ( | ||
| match l with | ||
| | [{pld_name = {txt = "..."}; pld_type = spread_typ; _}] -> | ||
| (* Ambiguous `{...t}`: if only spread present and it doesn't resolve to a | ||
| record type, treat it as an object-typed tuple argument. *) | ||
| let obj_ty = | ||
| Ast_helper.Typ.object_ ~loc:spread_typ.ptyp_loc | ||
| [Parsetree.Oinherit spread_typ] | ||
| Asttypes.Closed | ||
| in | ||
| let cty = transl_simple_type env closed obj_ty in | ||
| (Types.Cstr_tuple [cty.ctyp_type], Cstr_tuple [cty]) | ||
| | _ -> (Types.Cstr_record lbls', Cstr_record lbls))) |
There was a problem hiding this comment.
[P1] Reject object spreads mixed with record fields in constructor records
When an inline record constructor contains a spread that resolves to an object type and any additional record fields (e.g. type obj = {"name": string}; type t = V({...obj, label: string})), expand_labels_with_type_spreads returns None, but this branch falls through to the _ -> (Types.Cstr_record lbls', Cstr_record lbls) case. Unlike the type-declaration path, no error is raised and the inline record is left with a synthetic "..." field rather than expanding or reinterpreting the argument as an object. This allows object and record fields to be mixed silently, contradicting the commit goal of forbidding such combinations and producing a malformed record type whose spread fields are ignored. The constructor handling should mirror the declaration logic: either raise Object_spread_with_record_field or reinterpret a single spread as an object tuple to keep semantics consistent.
Useful? React with 👍 / 👎.
|
[CODEX] Minimal repro for constructor record + object spread mix-up Repro code (save as tmp/repro_inline_ctor_mix.res): type obj = {"name": string}
type t = V({...obj, label: string})Commands: dune build
node scripts/copyExes.js --compiler
node ./cli/bsc.js -dparsetree tmp/repro_inline_ctor_mix.res
node ./cli/bsc.js tmp/repro_inline_ctor_mix.resObserved:
|
|
[CODEX] Constructor record/object spread mix: repro + validated fix Repro (inline record constructor mixing object spread with record field): type obj = {"name": string}
type t = V({...obj, label: string})Before (on PR branch): compiles without error, allowing mix and leaving a synthetic "..." field in the constructor record args. Suggested change (constructor args should mirror type-decl behavior): (* compiler/ml/typedecl.ml :: transl_constructor_arguments, Pcstr_record branch *)
(* If expand_labels_with_type_spreads returns None: *)
match l with
| [{pld_name = {txt = "..."}; pld_type = spread_typ; _}] ->
(* reinterpret single `{...t}` as object tuple arg *)
| _ ->
(* raise Object_spread_with_record_field on first non-spread field *)Validation (locally applied minimal change, rebuilt, re-ran repro with This keeps the existing special-case for a single ambiguous |
|
Fixed in f5f2f84 |
zth
force-pushed
the
inline-record-type-spreads
branch
from
f5f2f84 to
699b429
Compare
4 participants
This fixes a few gaps with record spreads in inline records:
\"..."fields. They're now expandedCloses #6822