Conversation
tsnobip
changed the title
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
compiler/ml/ast_mapper_to0.ml
Outdated
| construct ~loc ~attrs (map_loc sub l) (map_opt (sub.pat sub) p) | ||
| | Ppat_variant (l, p) -> variant ~loc ~attrs l (map_opt (sub.pat sub) p) | ||
| | Ppat_record (lpl, cf) -> | ||
| | Ppat_record (lpl, cf, _rest) -> |
There was a problem hiding this comment.
I'm not sure about what should be done here
There was a problem hiding this comment.
good question: it goes to ast0 and then back during ppx, so any use of a ppx would discard this at the moment, and change the code
normally, one tries to encode additional ast information in special annotations, so they can be recovered on the way back (from_0)
|
@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.
Reviewed commit: 224576540e
ℹ️ 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".
compiler/ml/matching.ml
Outdated
| match pat.pat_desc with | ||
| | Tpat_record (_, _, Some rest) -> |
There was a problem hiding this comment.
inject_record_rest_binding only handles cases where the top-level pattern is Tpat_record (_, _, Some rest). When a record-rest appears inside another pattern shape (for example a tuple, constructor, or-or pattern), no Precord_spread_new binding is emitted even though typing has already introduced rest as a bound identifier. This causes incorrect code generation for valid nested destructuring forms because the rest variable is referenced without being initialized from the matched record.
Useful? React with 👍 / 👎.
| | Ppat_record (pl, _, _rest) -> | ||
| List.iter | ||
| (fun {lid = lbl; x = p} -> | ||
| add bv lbl; |
There was a problem hiding this comment.
The dependency walker now matches Ppat_record (pl, _, _rest) but never visits _rest. As a result, module/type paths used only in record-rest annotations (e.g. ...M.t as rest) are omitted from dependency collection, so changing those modules may not trigger recompilation or proper dependency invalidation.
Useful? React with 👍 / 👎.
compiler/syntax/src/res_core.ml
Outdated
| | PatRest rest_pat -> (fields, flag, Some rest_pat) | ||
| | PatUnderscore -> (fields, flag, rest)) |
There was a problem hiding this comment.
parse_record_pattern stores rest as Some rest_pat every time it sees PatRest and does not check whether a previous rest was already parsed. This lets patterns with multiple rest clauses be accepted while silently discarding one clause, which is ambiguous and can mask user mistakes instead of producing a syntax error.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f835e3f28e
ℹ️ 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".
| let rest_field_names = | ||
| List.map | ||
| (fun (l : Types.label_declaration) -> Ident.name l.ld_id) | ||
| rest_labels |
There was a problem hiding this comment.
The new rest-pattern validation only compares field names (rest_field_names) and never unifies the rest type’s field types with the source record’s field types. That lets incompatible rest annotations type-check (for example, source b: string with rest type b: int), so rest.b is typed incorrectly and can break soundness at runtime.
Useful? React with 👍 / 👎.
compiler/ml/matching.ml
Outdated
| simplify ((p :: patl, bind Alias id arg action) :: rem) | ||
| | Tpat_record ([], _) -> (omega :: patl, action) :: simplify rem | ||
| | Tpat_record (lbls, closed) -> | ||
| | Tpat_record ([], _, _rest) -> (omega :: patl, action) :: simplify rem |
There was a problem hiding this comment.
The Tpat_record ([], _, _rest) fast path drops the rest binder entirely, so a pattern like {...Sub.t as rest} is simplified to _ without generating the rest binding. If such a pattern is accepted by parsing/type-checking, code that uses rest in the branch can compile to incorrect/unbound behavior.
Useful? React with 👍 / 👎.
| let excluded_str = String.concat ", " excluded in | ||
| let code = Printf.sprintf "(({%s, ...__rest}) => __rest)" excluded_str in |
There was a problem hiding this comment.
The primitive emitter interpolates excluded names directly into a JS destructuring binding ("(({%s, ...__rest}) => __rest)"). This assumes every key is a valid identifier; however custom tag names come from string attributes and can contain non-identifier characters, which would generate invalid JavaScript.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da71513258
ℹ️ 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".
| let rest_decl = | ||
| match rest_decl.type_kind with | ||
| | Type_record _ -> instance_declaration rest_decl | ||
| | _ -> |
There was a problem hiding this comment.
This branch accepts any Type_record as a rest target but never checks type_private/label privacy before constructing the rest value. Because record-rest emits a fresh object, code outside a module can now manufacture values of a private record type (for example ...M.t as rest where M.t is private) from another compatible record, which breaks type abstraction guarantees and any invariants private construction was meant to enforce.
Useful? React with 👍 / 👎.
analysis/src/ProcessCmt.ml
Outdated
| | Tpat_record (items, _, _rest) -> | ||
| items |> List.iter (fun (_, _, p, _) -> handlePattern [] p) |
There was a problem hiding this comment.
The Tpat_record branch only traverses explicit field subpatterns and ignores the new rest binding payload, so ...Type as rest variables are not recorded as declarations in CMT processing. In practice this drops editor/indexing features for those bindings (definition lookup, rename, dead-value tracking) even though the variable is valid and in scope.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a9b5dd3d
ℹ️ 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".
| let explicit_fields = | ||
| List.map (fun (_, label, _, _) -> label.lbl_name) lbl_pat_list |
There was a problem hiding this comment.
explicit_fields is built from label.lbl_name, but Precord_spread_new later treats those strings as literal JavaScript property keys; this breaks record-rest semantics for fields renamed with @as(...). In patterns like {foo, ...Rest.t as rest}, a renamed runtime key (e.g. "foo-bar") is not excluded, so rest still contains foo at runtime, and if Rest.t uses a different rename the generated field access can read undefined. The exclusion list should use the same attribute-based field-name resolution used by normal record field compilation.
Useful? React with 👍 / 👎.
|
|
||
| let optional_attr = (Location.mknoloc "res.optional", Parsetree.PStr []) | ||
| let optional_attr0 = (Location.mknoloc "res.optional", PStr []) | ||
| let record_rest_attr_name = "res.record_rest" |
There was a problem hiding this comment.
This commit modifies parsetree0.ml, but /workspace/rescript/AGENTS.md explicitly says “Never modify parsetree0.ml” because PPX compatibility depends on that frozen v0 layer. Adding new record-rest helpers here risks breaking downstream assumptions tied to the frozen module contract; the record-rest transport logic should live outside parsetree0.ml (for example in mapper-side utilities) to preserve that compatibility guarantee.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@cristianoc what do you think? I guess the modification of parsetree0 is OK as long as it doesn't change the types, right?
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73a9b5dd3d
ℹ️ 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".
| let explicit_fields = | ||
| List.map (fun (_, label, _, _) -> label.lbl_name) lbl_pat_list |
There was a problem hiding this comment.
The record-rest exclusion list is built from label.lbl_name, which is the source field name, not the emitted JS property name for fields annotated with @as(...). In that case, Precord_spread_new excludes the wrong key (for example, excluding component instead of runtime key Component), so the rest object can still contain explicitly matched fields and no longer matches the annotated rest record shape. This should use each label’s runtime name (the same mapping used by record codegen/coercion) when computing excluded and compared field names.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 792d7084c5
ℹ️ 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".
|
|
||
| let optional_attr = (Location.mknoloc "res.optional", Parsetree.PStr []) | ||
| let optional_attr0 = (Location.mknoloc "res.optional", PStr []) | ||
| let record_rest_attr_name = "res.record_rest" |
There was a problem hiding this comment.
The repo-level guideline in /workspace/rescript/AGENTS.md explicitly says never to modify parsetree0.ml because PPX compatibility depends on that v0 surface staying frozen; introducing record-rest support by adding new parsetree0 helpers violates that constraint and risks downstream PPX breakage, so this should be implemented only via the mapping layers (ast_mapper_from0.ml/ast_mapper_to0.ml) without editing parsetree0.ml.
Useful? React with 👍 / 👎.
| let rest_decl = | ||
| match rest_decl.type_kind with | ||
| | Type_record _ -> instance_declaration rest_decl | ||
| | _ -> | ||
| raise |
There was a problem hiding this comment.
This branch rejects any rest type whose declaration kind is not directly Type_record, so a manifest alias like type sub = source (where source is a record) is incorrectly rejected with Record_rest_not_record; the source side already uses concrete type extraction, so rest types should also be expanded/resolved before enforcing record-kind checks.
Useful? React with 👍 / 👎.
3 participants
fixes #8311