Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@

#### :nail_care: Polish

- Consolidate record mutation output into a single spread object literal. https://github.com/rescript-lang/rescript/pull/8473
- Improve default argument type mismatch errors. https://github.com/rescript-lang/rescript/pull/8389
- Resolve workspace dependencies in editor analysis. https://github.com/rescript-lang/rescript/pull/8392
- Build system: Add OpenTelemetry tracing support for cli commands. https://github.com/rescript-lang/rescript/pull/8370
Expand Down
6 changes: 5 additions & 1 deletion compiler/core/js_analyzer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,11 @@ let rec no_side_effect_expression_desc (x : J.expression_desc) =
*)
Ext_list.for_all xs no_side_effect
| Optional_block (x, _) -> no_side_effect x
| Object (_, kvs) -> Ext_list.for_all_snd kvs no_side_effect
| Object (dup, kvs) ->
(match dup with
| Some e -> no_side_effect e
| None -> true)
&& Ext_list.for_all_snd kvs no_side_effect
| String_append (a, b) | Seq (a, b) -> no_side_effect a && no_side_effect b
| Length (e, _) | Caml_block_tag (e, _) | Typeof e -> no_side_effect e
| Bin (op, a, b) -> op <> Eq && no_side_effect a && no_side_effect b
Expand Down
23 changes: 23 additions & 0 deletions compiler/core/js_dump.ml
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,29 @@ and expression_desc cxt ~(level : int) f x : cxt =
| [tag; ({expression_desc = J.Var _} as spread_props)] ->
(* All the props are spread *)
print_jsx cxt ~level ~spread_props f fn_name tag []
| [tag; {expression_desc = J.Object (Some spread, props)}] ->
(* Spread props with overrides emitted as a single object literal:
{...base, x: 1}
*)
let fields =
List.filter_map
(fun (n, x) ->
match n with
| Js_op.Lit name -> Some (name, x)
| Symbol_name -> None)
props
in
print_jsx cxt ~level ~spread_props:spread f fn_name tag fields
| [tag; {expression_desc = J.Object (Some spread, props)}; key] ->
let fields =
List.filter_map
(fun (n, x) ->
match n with
| Js_op.Lit name -> Some (name, x)
| Symbol_name -> None)
props
in
print_jsx cxt ~level ~spread_props:spread ~key f fn_name tag fields
| _ ->
(* This should not happen, we fallback to the general case *)
expression_desc cxt ~level f
Expand Down
72 changes: 64 additions & 8 deletions compiler/core/lam_compile.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1834,6 +1834,59 @@ let compile output_prefix =
in
Js_output.output_of_block_and_expression lambda_cxt.continuation args_code
exp
and collect_dup_overrides (copy_id : Ident.t) (lam : Lam.t)
(acc : (Lam_compat.set_field_dbg_info * Lam.t) list) :
(Lam_compat.set_field_dbg_info * Lam.t) list option =
match lam with
| Lsequence
( Lprim
{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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve field update evaluation order

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 👍 / 👎.

| Lvar id' when Ident.same id' copy_id -> Some acc
| _ -> None
and try_compile_record_spread (lambda_cxt : Lam_compile_context.t)
(id : Ident.t) (arg : Lam.t) (body : Lam.t) : Js_output.t option =
match arg with
| Lprim {primitive = Pduprecord; args = [init]; _} -> (
match collect_dup_overrides id body [] with
| None -> None
| Some overrides ->
let need_value_cxt =
{lambda_cxt with continuation = NeedValue Not_tail}
in
let init_output = compile_lambda need_value_cxt init in
let init_val =
match init_output.value with
| Some v -> v
| None -> assert false
in
let blocks, props =
List.fold_left
(fun (blocks, props)
((fld_info : Lam_compat.set_field_dbg_info), value_lam) ->
let val_output = compile_lambda need_value_cxt value_lam in
let val_val =
match val_output.value with
| Some v -> v
| None -> assert false
in
let name =
match fld_info with
| Fld_record_set name
| Fld_record_inline_set name
| Fld_record_extension_set name ->
name
in
(blocks @ val_output.block, (Js_op.Lit name, val_val) :: props))
(init_output.block, []) (List.rev overrides)
in
Some
(Js_output.output_of_block_and_expression lambda_cxt.continuation
blocks
(E.obj ~dup:init_val props)))
| _ -> None
and compile_lambda (lambda_cxt : Lam_compile_context.t) (cur_lam : Lam.t) :
Js_output.t =
match cur_lam with
Expand All @@ -1859,14 +1912,17 @@ let compile output_prefix =
}
body)))
| Lapply appinfo -> compile_apply appinfo lambda_cxt
| Llet (let_kind, id, arg, body) ->
(* Order matters.. see comment below in [Lletrec] *)
let args_code =
compile_lambda
{lambda_cxt with continuation = Declare (let_kind, id)}
arg
in
Js_output.append_output args_code (compile_lambda lambda_cxt body)
| Llet (let_kind, id, arg, body) -> (
match try_compile_record_spread lambda_cxt id arg body with
| Some output -> output
| None ->
(* Order matters.. see comment below in [Lletrec] *)
let args_code =
compile_lambda
{lambda_cxt with continuation = Declare (let_kind, id)}
arg
in
Js_output.append_output args_code (compile_lambda lambda_cxt body))
| Lletrec (id_args, body) ->
(* There is a bug in our current design,
it requires compile args first (register that some objects are jsidentifiers)
Expand Down
20 changes: 6 additions & 14 deletions tests/tests/src/jsx_preserve_test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -54,19 +54,15 @@ let baseProps = {
title: "foo"
};

let newrecord = {...baseProps};

let _unary_element_with_spread_props = <input
{...newrecord}
{...baseProps}
type={"text"}
/>;

let newrecord$1 = {...baseProps};

let _container_with_spread_props = <div
{...newrecord$1}
title={"barry"}
{...baseProps}
className={"barry"}
title={"barry"}
>
{"Hello, world!"}
<input
Expand All @@ -83,21 +79,17 @@ let baseChildren = [
</span>
];

let newrecord$2 = {...baseProps};

let _unary_element_with_spread_props_keyed = <input
key={"barry-key"}
{...newrecord$2}
{...baseProps}
type={"text"}
/>;

let newrecord$3 = {...baseProps};

let _container_with_spread_props_keyed = <div
key={"barry-key"}
{...newrecord$3}
title={"barry"}
{...baseProps}
className={"barry"}
title={"barry"}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The order changed because it now follows the declaration order rather than the source order.

>
{"Hello, world!"}
<input
Expand Down
37 changes: 21 additions & 16 deletions tests/tests/src/large_record_duplication_test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,10 @@ let A0 = /* @__PURE__ */Primitive_exceptions.create("Large_record_duplication_te

Mocha.describe("Large_record_duplication_test", () => {
Mocha.test("large record spread operation", () => {
let f0 = x => {
let newrecord = {...x};
newrecord.x0 = 1;
return newrecord;
};
let f0 = x => ({
...x,
x0: 1
});
let result = f0(v0);
Test_utils.eq("File \"large_record_duplication_test.res\", line 143, characters 7-14", result.x0, 1);
Test_utils.eq("File \"large_record_duplication_test.res\", line 144, characters 7-14", result.x1, 9);
Expand Down Expand Up @@ -86,10 +85,12 @@ Mocha.describe("Large_record_duplication_test", () => {
let f1 = x => {
if (typeof x !== "object") {
return "A1";
} else {
return {
...x,
x0: 1
};
}
let newrecord = {...x};
newrecord.x0 = 1;
return newrecord;
};
Test_utils.eq("File \"large_record_duplication_test.res\", line 201, characters 7-14", get_x0(f1({
TAG: "A0",
Expand Down Expand Up @@ -125,12 +126,14 @@ Mocha.describe("Large_record_duplication_test", () => {
}
};
let f2 = x => {
if (x.TAG !== "A0") {
if (x.TAG === "A0") {
return {
...x,
x0: 1
};
} else {
return x;
}
let newrecord = {...x};
newrecord.x0 = 1;
return newrecord;
};
Test_utils.eq("File \"large_record_duplication_test.res\", line 243, characters 7-14", get_x0(f2({
TAG: "A0",
Expand Down Expand Up @@ -161,12 +164,14 @@ Mocha.describe("Large_record_duplication_test", () => {
});
Mocha.test("exception extension with large record", () => {
let f3 = x => {
if (x.RE_EXN_ID !== A0) {
if (x.RE_EXN_ID === A0) {
return {
...x,
x0: 1
};
} else {
return x;
}
let newrecord = {...x};
newrecord.x0 = 1;
return newrecord;
};
let get_x0 = x => {
if (x.RE_EXN_ID === A0) {
Expand Down
7 changes: 4 additions & 3 deletions tests/tests/src/record_regression.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,10 @@ let newrecord$3 = {...v1};
newrecord$3.y1 = 22;

function h11(v1) {
let newrecord = {...v1};
newrecord.y1 = 22;
return newrecord;
return {
...v1,
y1: 22
};
}

let po = {
Expand Down
35 changes: 20 additions & 15 deletions tests/tests/src/stdlib/intl/Stdlib_Intl_DateTimeFormatTest.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -46,33 +46,38 @@ let formatter$1 = new Intl.DateTimeFormat(undefined, options);

console.log(formatter$1.format(new Date(Date.now())));

let newrecord = {...options};

let formatter$2 = new Intl.DateTimeFormat(undefined, (newrecord.timeZoneName = "long", newrecord));
let formatter$2 = new Intl.DateTimeFormat(undefined, {
...options,
timeZoneName: "long"
});

console.log(formatter$2.format(new Date(Date.now())));

let newrecord$1 = {...options};

let formatter$3 = new Intl.DateTimeFormat(undefined, (newrecord$1.timeZoneName = "longOffset", newrecord$1));
let formatter$3 = new Intl.DateTimeFormat(undefined, {
...options,
timeZoneName: "longOffset"
});

console.log(formatter$3.format(new Date(Date.now())));

let newrecord$2 = {...options};

let formatter$4 = new Intl.DateTimeFormat(undefined, (newrecord$2.timeZoneName = "short", newrecord$2));
let formatter$4 = new Intl.DateTimeFormat(undefined, {
...options,
timeZoneName: "short"
});

console.log(formatter$4.format(new Date(Date.now())));

let newrecord$3 = {...options};

let formatter$5 = new Intl.DateTimeFormat(undefined, (newrecord$3.timeZoneName = "shortGeneric", newrecord$3));
let formatter$5 = new Intl.DateTimeFormat(undefined, {
...options,
timeZoneName: "shortGeneric"
});

console.log(formatter$5.format(new Date(Date.now())));

let newrecord$4 = {...options};

let formatter$6 = new Intl.DateTimeFormat(undefined, (newrecord$4.timeZoneName = "shortOffset", newrecord$4));
let formatter$6 = new Intl.DateTimeFormat(undefined, {
...options,
timeZoneName: "shortOffset"
});

console.log(formatter$6.format(new Date(Date.now())));

Expand Down
Loading