ppx: add test for inference regression in keys

[jchavarri]

This test shows some regression where the type checker is not able to infer the type of a prop for a value which type has been inferred before in a key prop.

The compilation fails with:

File "test/React__test.re", line 605, characters 26-34:
605 |           <img src=author.imageUrl />
                                ^^^^^^^^
Error: Unbound record field imageUrl
make: *** [Makefile:12: build] Error 1

When before, it would infer that author has type Author.t, as it is namespaced in line 603:

reason-react/test/React__test.re

Line 603 in c9acf14

<tr key={author.Author.name}>

I bisected the test, and this used to work before #714 was merged (tested with commit 2b8819b and it builds fine)

[anmonteiro]

Check the difference in generated code.

Before:

let render author =
  ReactDOMRe.createDOMElementVariadic "tr"
    ~props:(ReactDOMRe.domProps ~key:author.Author.name ())
    [|
      ReactDOMRe.createDOMElementVariadic "td"
        [|
          ReactDOMRe.createDOMElementVariadic "img"
            ~props:(ReactDOMRe.domProps ~src:author.imageUrl ())
            [||];
        |];
    |]

After:

let render author =
  ReactDOM.jsxKeyed "tr"
    ((ReactDOM.domProps [@merlin.hide])
       ~children:
         (ReactDOM.jsx "td"
            ((ReactDOM.domProps [@merlin.hide])
               ~children:
                 (ReactDOM.jsx "img"
                    ((ReactDOM.domProps [@merlin.hide]) ~src:author.imageUrl ()))
               ()))
       ())
    ~key:author.Author.name ()

I tried this too, but it doesn't fix it:

let render author =
  ReactDOM.jsxKeyed ~key:author.Author.name "tr"
    ((ReactDOM.domProps [@merlin.hide])
       ~children:
         (ReactDOM.jsx "td"
            ((ReactDOM.domProps [@merlin.hide])
               ~children:
                 (ReactDOM.jsx "img"
                    ((ReactDOM.domProps [@merlin.hide]) ~src:author.imageUrl ()))
               ()))
       ())
    ()

[jchavarri]

Thanks for sharing the ppx expanded.

With React jsx-runtime API, key is the last argument in this function:

reason-react/src/ReactDOM.re

Line 2139 in c9acf14

external jsxKeyed: (string, domProps, string) => React.element = "jsx";

(side note: should it be a key labeled arg?)

So, because what matters for inference is the ordering of arguments at the definition, there is not much we can do. Here is a simpler example, note how the call to foo succeeds while foo2 fails:

let foo = (~bar, ~baz) => bar ++ baz;
let foo2 = (~baz, ~bar) => bar ++ baz;
let _ = author => foo(~bar=author.Author.name, ~baz=author.imageUrl);
let _ = author => foo2(~bar=author.Author.name, ~baz=author.imageUrl);
                                                            ^^^^^^^^
Error: Unbound record field imageUrl

Funny enough (but understandably) this works fine:

let render = author =>
  <tr key={author.name}>
    <td>
      <img src=author.Author.imageUrl />
    </td>
  </tr>;

The only solution I can think of is to redefine the args order by doing something like:

[@bs.module "react/jsx-runtime"]
external jsxKeyed: (string, domProps, string) => React.element = "jsx";
let jsxKeyed(element, key, props) = jsxKeyed(element, props, key);

But I personally don't think it's worth it. Wdyt?

[jchavarri]

went ahead and implemented the suggestion above in 8425467.

[anmonteiro]

That’s a good idea, we don’t need the let though. We should be able to keep it an external, or didn’t that work?
Key needs to be labeled and optional to fix the case in #750

[jchavarri]

We should be able to keep it an external, or didn’t that work?

I don't know how to have both things with a single external:

• keep the arguments in the order that React expects (key after props)
• make the key come before props for type inference

Do you have any ideas?

[anmonteiro]

Ah you’re right, of course. I think this creates an extra function call for every JSX call though, which I don’t think is desirable.

[anmonteiro]

@jchavarri I pushed a different version of your fix that generates:

let render author =
  let reason_react_ppx_key_arg___x = author.Author.name in
  ReactDOM.jsxKeyed ~key:reason_react_ppx_key_arg___x "tr"
    (((ReactDOM.domProps)[@merlin.hide ])
       ~children:(ReactDOM.jsx "td"
                    (((ReactDOM.domProps)[@merlin.hide ])
                       ~children:(ReactDOM.jsx "img"
                                    (((ReactDOM.domProps)[@merlin.hide ])
                                       ~src:(author.imageUrl) ())) ())) ())
    ()

we introduce a let binding that matches the expected flow of type inference. What do you think?

[jchavarri]

Your approach looks good as well! It is very creative :) I left a comment about locations that I think should be tackled before merging.

In general, all other things equal, I tend to prefer solving issues elsewhere than into a ppx, just because it has so much inherent complexity... but it is not a strong opinion, happy to go with this solution.

Do you know which one would be more optimal in terms of resulting size of the generated JavaScript code? I understand the extra let in the bindings is just added once in the bindings, but the ppx will add an extra let for every usage of key?

[jchavarri]

This kind of breaks the rules for "well-formed" locations, because the apply will have a larger range (parentExpLoc) than the parent let expression (which has loc). I am afraid that our for now limited tests are not catching a potential regression here.

I think we should at least rename loc to callerLoc or something else, to prevent this kind of issues in the future.

[davesnx]

The rename make sense, but I don't understand the issue here. I belive the location is pointed to the right place no?

(li ~key:e.path [@JSX]) (* <-- parentExpLoc *)

let reason_react_ppx_key_arg___x = e.path in
ReactDOM.jsxKeyed ~key:reason_react_ppx_key_arg___x "li" (* <-- parentExpLoc *)

[davesnx]

After pressing "Send", I realised that the let reason_react_ppx_key_arg___x use the location of the identifier, and this is the reason why when you hover a lowercase component it says "string"

I wonder now, what would be the right location for this case.

[jchavarri]

In my experience, it doesn't matter that much which is the right location for each node. As long as they are well formed (parents have larger scope than children), things kind of work. One might need to hide some nodes to prioritize others, but the fundamental first step is that the ranges are well formed. When that fails, merlin will fail in most cases.

[jchavarri]

I updated this branch with latest main, which included some merlin tests for key prop, and they show there is a regression in this PR where many locations in key are lost (check ppx/test/keys.t/run.t), which confirms the suspicion of my previous comment.