Consolidate large record mutation output into a single spread object literal#8473
Consolidate large record mutation output into a single spread object literal#8473cometkim wants to merge 3 commits into
Conversation
…teral
So it emits `{...x, a: 1}` instead of `let newrecord = {...x}; newrecord.a = 1;`.
![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: 55281545d1
ℹ️ 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".
| {primitive = Psetfield (_, fld_info); args = [Lvar id'; value]; _}, | ||
| rest ) | ||
| when Ident.same id' copy_id -> | ||
| collect_dup_overrides copy_id rest ((fld_info, value) :: acc) |
There was a problem hiding this comment.
When a record update overrides more than one field and the override expressions have side effects, this collector reverses the Lsequence order: each Psetfield is consed onto acc, and the fold below compiles that reversed list. The previous output executed the Psetfield chain in order, so cases like two logging/counter updates now run in the opposite order. Collect in order, or reverse before compiling values, before emitting the object literal.
Useful? React with 👍 / 👎.
| Some | ||
| (Js_output.output_of_block_and_expression lambda_cxt.continuation | ||
| blocks | ||
| (E.obj ~dup:init_val (List.rev props)))) |
There was a problem hiding this comment.
When the optimized record update is used only for effects and the override values are pure, this emits a single Object(Some spread, props) value; Js_analyzer.no_side_effect_expression currently treats Object(_, kvs) as pure by checking only kvs, so append_output/output_as_block can drop {...getRecord(), x: 1} entirely and skip the side effects of getRecord(). The old temp-plus-assignment path kept the spread initializer live because the temp was used by the assignment.
Useful? React with 👍 / 👎.
| title={"barry"} | ||
| {...baseProps} | ||
| className={"barry"} | ||
| title={"barry"} |
There was a problem hiding this comment.
The order changed because it now follows the declaration order rather than the source order.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8473 +/- ##
=======================================
Coverage 74.42% 74.42%
=======================================
Files 451 451
Lines 61459 61497 +38
=======================================
+ Hits 45743 45772 +29
- Misses 15716 15725 +9
🚀 New features to boost your workflow:
|
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
1 participant
Follow-up to #7043
To avoid excessive variations in the Lambda types, I added a kind of pattern matching for record mutation (Pduprecord + Psetfield chain).
But it is not a very efficient implementation; it might be better to just change the IR.