Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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
4 changes: 4 additions & 0 deletions compiler/core/js_pass_flatten_and_mark_dead.ml
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,10 @@ let subst_map (substitution : J.expression Hash_ident.t) =
turn a runtime crash into compile time crash : )
*)
match Ext_list.nth_opt ls (Int32.to_int i) with
(* 7432: prevent optimization in JSX preserve mode *)
| Some {expression_desc = J.Var (Id {name = "make"})}
Copy link
Member

Choose a reason for hiding this comment

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

This check could in theory affect each make function right?
Could we add some sort of check if that make returns a Jsx.element type? Just to further limit the scope when this kicks in.

Copy link
Collaborator

Choose a reason for hiding this comment

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

there's no type this down the compiler stack

when !Js_config.jsx_preserve ->
super.expression self x
| Some
({expression_desc = J.Var _ | Number _ | Str _ | Undefined _} as
x) ->
Expand Down
9 changes: 9 additions & 0 deletions compiler/core/lam_pass_remove_alias.ml
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,15 @@ let simplify_alias (meta : Lam_stats.t) (lam : Lam.t) : Lam.t =
let rec simpl (lam : Lam.t) : Lam.t =
match lam with
| Lvar _ -> lam
(* 7432: prevent optimization in JSX preserve mode *)
| Lprim
{
primitive = Pjs_call {prim_name = "jsx" | "jsxs"} as primitive;
Copy link
Member

Choose a reason for hiding this comment

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

Same remark, very unlikely that jsx is not going to come from ReactRuntime call, if we can further tighten it that be great.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's plenty guarded against risk already

Copy link
Collaborator

Choose a reason for hiding this comment

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

why not the same check that preserve mode is on?
I guess it's kind of implicit, and only a super corner case would have the same name, but since it's so easy to add why not

Copy link
Member Author

Choose a reason for hiding this comment

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

The check for preserve mode is already there (below).

args = (Lprim {primitive = Pfield (_, _)} as field_arg) :: rest;
loc;
}
when !Js_config.jsx_preserve ->
Lam.prim ~primitive ~args:(field_arg :: Ext_list.map rest simpl) loc
| Lprim {primitive = Pfield (i, info) as primitive; args = [arg]; loc} -> (
(* ATTENTION:
Main use case, we should detect inline all immutable block .. *)
Expand Down
2 changes: 1 addition & 1 deletion tests/tests/src/async_jsx.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ let Foo = {

function Async_jsx$Bar(props) {
return <div>
<Async_jsx$Foo />
<Foo.make />
</div>;
}

Expand Down
62 changes: 53 additions & 9 deletions tests/tests/src/jsx_preserve_test.mjs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// Generated by ReScript, PLEASE EDIT WITH CARE

import * as React from "react";
import * as JsxRuntime from "react/jsx-runtime";

function Jsx_preserve_test$Icon(props) {
Expand All @@ -20,7 +21,7 @@ let _multiple_element_children = <div>
<h1>
{"Hello, world!"}
</h1>
<Jsx_preserve_test$Icon />
<Icon.make />
</div>;

let _single_element_fragment = <>
Expand Down Expand Up @@ -141,7 +142,7 @@ let B = {

let _external_component_with_children = <QueryClientProvider>
<strong />
<Jsx_preserve_test$B />
<B.make />
</QueryClientProvider>;

function Jsx_preserve_test$MyWeirdComponent(props) {
Expand All @@ -155,7 +156,7 @@ let MyWeirdComponent = {
make: Jsx_preserve_test$MyWeirdComponent
};

let _escaped_jsx_prop = <Jsx_preserve_test$MyWeirdComponent
let _escaped_jsx_prop = <MyWeirdComponent.make
MyWeirdProp={"bar"}
/>;

Expand Down Expand Up @@ -197,7 +198,7 @@ let ComponentWithOptionalProps = {
make: Jsx_preserve_test$ComponentWithOptionalProps
};

let _optional_props = <Jsx_preserve_test$ComponentWithOptionalProps
let _optional_props = <ComponentWithOptionalProps.make
i={1}
s={"test"}
element={<div />}
Expand All @@ -208,11 +209,9 @@ let _props_with_hyphen = <label
data-testid={"test"}
/>;

let React = {};

let _fragment = <Fragment>
let _fragment = <>
{"Hello, world!"}
</Fragment>;
</>;

let _youtube_iframe = <iframe
allow={"accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture; web-share"}
Expand All @@ -224,6 +223,47 @@ let _youtube_iframe = <iframe
referrerPolicy={"strict-origin-when-cross-origin"}
/>;

function make(_props) {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding the tests!

return null;
}

let X = {
make: make
};

<X.make />;

function Jsx_preserve_test$Y(props) {
return null;
}

let make$1 = React.memo(Jsx_preserve_test$Y);

let Y = {
x: 42,
make: make$1
};

<Y.make />;

let context = React.createContext(0);

let make$2 = context.Provider;

let ContextProvider = {
make: make$2
};

function Jsx_preserve_test(props) {
return <ContextProvider.make
value={42}
>
{props.children}
</ContextProvider.make>;
}

let make$3 = Jsx_preserve_test;

export {
Icon,
_single_element_child,
Expand All @@ -250,8 +290,12 @@ export {
ComponentWithOptionalProps,
_optional_props,
_props_with_hyphen,
React,
_fragment,
_youtube_iframe,
X,
Y,
context,
ContextProvider,
make$3 as make,
}
/* _single_element_child Not a pure module */
40 changes: 29 additions & 11 deletions tests/tests/src/jsx_preserve_test.res
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,6 @@ let _optional_props = <ComponentWithOptionalProps i=1 s="test" element={<div />}

let _props_with_hyphen = <label ariaLabel={"close sidebar"} dataTestId="test" />

module React = {
type component<'props> = Jsx.component<'props>
type element = Jsx.element

external jsx: (component<'props>, 'props) => element = "jsx"

type fragmentProps = {children?: element}

external jsxFragment: component<fragmentProps> = "Fragment"
}

let _fragment = <> {Jsx.string("Hello, world!")} </>

let _youtube_iframe =
Expand All @@ -153,3 +142,32 @@ let _youtube_iframe =
allowFullScreen={true}
>
</iframe>

module X = {
type props = {}
let make = (_props: props) => React.null
}

let _ = <X />

module Y = {
let x = 42

@react.component
let make = () => React.null

let make = React.memo(make)
}

let _ = <Y />

let context = React.createContext(0)

module ContextProvider = {
let make = React.Context.provider(context)
}

@react.component
let make = (~children) => {
<ContextProvider value=42> {children} </ContextProvider>
}
Loading