Open in StackBlitz

npm i https://pkg.pr.new/rescript@8317

npm i https://pkg.pr.new/@rescript/darwin-arm64@8317

npm i https://pkg.pr.new/@rescript/darwin-x64@8317

npm i https://pkg.pr.new/@rescript/linux-arm64@8317

npm i https://pkg.pr.new/@rescript/linux-x64@8317

npm i https://pkg.pr.new/@rescript/runtime@8317

npm i https://pkg.pr.new/@rescript/win32-x64@8317

commit: 5e37735

@codex review

@codex review

@codex review

@codex review

@codex review

@codex review

@codex review

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@codex review

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@codex review

@codex review

@codex review

@codex review

@codex review

@codex review

I asked Codex (locally) if it noticed anything else, here is the output (I think the one about ast0 can be ignored):

Yes. The biggest one I’d flag is in compiler/core/js_pass_record_rest.ml.

materialize_fields rewrites later reads of source.field to the destructured local binding introduced for object rest:

let {name, ...restConfig as rest} = source
// later source.name can become name

That is only valid if source cannot be reassigned or observed through mutation between the destructure and the read. The pass rewrites the whole remaining tail block, including nested function bodies, so cases like this look risky:

let mutable source = {name: "a", version: "1", debug: false}
let {name: _, ...restConfig as rest} = source
source = {name: "b", version: "2", debug: true}
source.name

If the JS pass replaces the final source.name with the destructured name, it returns the old value. Even worse with a closure created after destructuring and called after reassignment. I’d expect either no rewrite, or a much narrower rewrite that stops at writes/reassignments and probably does not descend into nested functions.

Other questionable points:

• compiler/common/pattern_printer.ml drops _rest, as discussed. Mostly diagnostic-only, but rest-only patterns can become odd empty-record diagnostics.
• compiler/ml/parmatch.ml also drops _rest in Conv.conv, so exhaustiveness counterexamples cannot show record-rest shape. Probably acceptable, but it reinforces that diagnostics are lossy.
• compiler/core/j.ml adds Object_rest_param, but the PR mostly seems to avoid emitting it and instead moves destructuring into the body for directive-sensitive cases. That may be dead or premature AST surface unless another pass produces it.
• compiler/ml/ast_mapper_to0.ml / ast_mapper_from0.ml encode record rest through a synthetic res.record_rest attribute when converting to/from the old parsetree. That is a brittle bridge: malformed or user/PPX-created attributes could be silently ignored or collide with real metadata unless this attribute namespace is clearly reserved.
• Tooling support is uneven: completion/signature-help mostly ignore _rest, while hints/semantic tokens handle it. That’s not a compiler correctness issue, but the feature may feel half-supported in editor workflows.

If I were reviewing the PR, I’d lead with the js_pass_record_rest rewrite across mutation/closures. That one looks like the highest chance of a real behavioral regression.

Yes. The biggest one I’d flag is in compiler/core/js_pass_record_rest.ml.

materialize_fields rewrites later reads of source.field to the destructured local binding introduced for object rest:

let {name, ...restConfig as rest} = source
// later source.name can become name

That is only valid if source cannot be reassigned or observed through mutation between the destructure and the read. The pass rewrites the whole remaining tail block, including nested function bodies, so cases like this look risky:

let mutable source = {name: "a", version: "1", debug: false}
let {name: _, ...restConfig as rest} = source
source = {name: "b", version: "2", debug: true}
source.name

@cknitt codex has been warning me multiple times about this issue but always with invented rescript syntax and semantics, typically let mutable is not a thing in rescript and if you use a ref instead I can't reproduce any issue in this case. Could you try to come up with any reproducible issue in the dev playground.

I'll take a look at the other issues you've raised.

UPDATE: I've addressed the other issues.

Could you try to come up with any reproducible issue in the dev playground

Didn't manage to come up with anything either.

UPDATE: I've addressed the other issues.

Thanks! 👍