Conversation
tsnobip
changed the title
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
| 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".
| 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 👍 / 👎.
| | 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 👍 / 👎.
3 participants
fixes #8311