merlin and RLS broken for JSX3 components

[sgrove]

https://youtu.be/Hf6hEqHqpG4 shows the issues for ReasonReact JSX3 + GraphQL, but this is also the case even for simple components:

open React;
open! ReasonReact;

type state = {
  count: int,
  show: bool,
};

type action =
  | Click(int)
  | Toggle;

let initialState = {count: 0, show: true};

[@react.component]
let make = (~greeting) => {
  let (state, dispatch) =
    useReducer(
      (state, action) =>
        switch (action) {
        | _ => state
        | Click(points) => {...state, count: state.count + points}
        | Toggle => {...state, show: !state.show}
        },
      initialState,
    );

  let message =
    "You've clicked this " ++ string_of_int(state.count) ++ " times(s)";

  <div>
    <button onClick={_ => dispatch(Click(10))}> {string(message)} </button>
    <button onClick={_ => dispatch(Toggle)}>
      {string("Toggle greeting")}
    </button>
    {state.show ? string(greeting) : null}
  </div>;
};

The issue for this smaller component show up in weird places. Asking for the type under lots of locations works, but asking for the type of points under | Click(points) => expands the selection to the whole function (everything after let make = . Same thing for checking dispatch in let (state, dispatch), though state works fine.

Similarly for {state.show ? string(greeting) : null}, these tokens work: state, string, null and these expand the selection to the full component show (in state.show), greeting

[sgrove]

I've confirmed this is also an issue with vim with reason master, so it indeed seems to be the [@react.component] ppx swallowing up location information.

[rickyvetter]

Commented in #editorsupport on discord but copying here for better searchibility:

Quite tough to correctly recover in this case. I’ve tried a few different things and nothing works very well. I’m not confident that I have the right APIs to construct it correctly. I essentially need to create two different locations for things - one for error reporting and one for merlin as the tooling treats them differently. Right now it’s optimized for error reporting and I can tweak it to get the tooling but then the error reporting doesn’t have any location info or has incorrect info.

I hesitate to "blame" ocaml for this but i think it would need to change at this level. take a look at this simplified example:

[@react.component]
let make = (~name) => ...;

gens

[@bs.obj]
external makeProps: (~name: 'name, ~key: string=?, unit) => {. "name": 'name} = "";

let make = (Props) => {
  let name = Props##name;
  ...
};

the trick is finding the correct location for let name - today we replace it with the location of ~name - which means if it goes unused or there's a type error with usage the warning/usage points to the location which exists in your actual code.

This is confusing for merlin (rightfully!) because let name, Props##name (and sometimes ~name) in the bs.obj actually all need to point to the same location to make all possible variations of name related errors show up correctly. There are circular references in go to definition and type at location that it cannot reasonably be expected to parse (and would never have to parse in normal situations).

The alternative is to mint new locations for each piece of code - I've done this and merlin seems to handle it pretty well actually - but then error messages get printed with null locations, which is even less acceptable to me. I think there are 2 potential solutions and 1 workaround.

1. merlin needs to be able to ignore locations entirely to be able to generate type information. I don't know enough about merlin to know if this is feasible, but I guess not.
2. ocaml needs to add double locations to every node in the ast - one for error reporting and one for tracing values linearly through a codebase.

possible workaround: with the introduction of records as objects we could instead have:

type props('name) = {name: 'name};
[@bs.objRec]
external makeProps: (~name: 'name, ~key: string=?, unit) => props('name) = "";

let make = ({name}) => ...;

which reduces the number of places that point to the original name and may be possible to simplify enough to satisfy error reporting and merlin. I'll be experimenting with this over the holidays and am ~70% optimistic for success.

[jchavarri]

@rickyvetter thanks for the extensive explanation!

There seems to be some attributes in Merlin that could allow to control in a precise way how it interprets the resulting AST: https://github.com/ocaml/merlin/blob/master/doc/dev/PROTOCOL.md#locations-in-ppx-rewriters

I have never user them but I have seen merlin.loc in action in jsoo. Maybe @let-def or @hhugo can confirm if these attributes could enable a solution for this ppx without changing reason-react API.

[sgrove]

This is much better since #4119, but I still hit some issues in some parts of the code.

One example is this, where I can't ask for the type of .networkPending:

[davesnx]

I'm not sure if it's still relevant, but meanwhile, locations are a little broken between the Reason parser, and the JSX ppx... This still happens.