Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better editor and merlin integration by improving ppx locations #748

Merged
merged 13 commits into from
Jul 18, 2023

Conversation

jchavarri
Copy link
Collaborator

@jchavarri jchavarri commented Jul 13, 2023

Fixes #429.

The fix is kind of similar to the one used in jsoo-react, but adapted to the current state of this ppx.

It is based on these ideas:

  • Avoid using Location.none, especially on intermediate ast nodes. Using Location.none completely disables merlin ability to find back an ast node for a given location
  • Try to build locations in a consistent way with the ast tree. for what "consistent" means, check this explanation by trefis
  • Hide subparts of the ast with merlin.hide (when we want to hide whole subtree) or loc_ghost (for just one node)
  • Use -dparsetree in tests to both check which locations are set to none or inconsistent, and see how changes in ppx affect the result

The PR adds some tests using -dparsetree to see how changes in the ppx affect locations in real code. Unfortunately I am not able to set up a test that involves merlin as well, so what I do for testing, besides looking at the output in the newly added ppx/test/output_locations.expected, is:
- Change reactjs_jsx_ppx.ml
- make build
- Hover over elements in test/React__test.re

This is a quite fast workflow. Edit: tests using merlin directly are being added in #749.

Here are some gifs to show comparison between main and this branch.

Case 1: Component in React__test.re

main This branch
case-1-ppx-locs-before case-1-ppx-locs-after

Case 2: The component reported in original issue #429

main This branch
case-2-ppx-locs-before case-2-ppx-locs-after

@jchavarri
Copy link
Collaborator Author

There was a comment by Ricky about some tradeoff between optimizing for good errors or for merlin integration in the original issue.

Unfortunately no examples of this tradeoff were provided, I have tested with unused props, go to definition in props, type errors in props, etc. and all the tests I did were successful, in all cases both Merlin and the compiler can figure things out fine.

@jchavarri jchavarri mentioned this pull request Jul 13, 2023
@jchavarri jchavarri requested a review from davesnx July 14, 2023 08:23
@jchavarri
Copy link
Collaborator Author

@anmonteiro @davesnx gonna go ahead and merge this, we can refine later on as we find more broken cases, now that there is some tests infrastructure available.

@jchavarri jchavarri merged commit 065548c into reasonml:main Jul 18, 2023
4 checks passed
@jchavarri jchavarri deleted the fix-ppx-locations branch July 18, 2023 08:36
jchavarri added a commit to jchavarri/reason-react that referenced this pull request Jul 18, 2023
* main:
  Better editor and merlin integration by improving ppx locations (reasonml#748)
  Update flake, add ocamlformat (reasonml#751)
davesnx added a commit to davesnx/opam-repository that referenced this pull request Sep 13, 2023
CHANGES:

* Migrate the reason-react PPX and library to the [new React JSX
transform](https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html)
  (@anmonteiro in [reasonml/reason-react#714](reasonml/reason-react#714))
* Add `suppressHydrationWarning` to supported props (@davesnx in
[reasonml/reason-react#721](reasonml/reason-react#721))
* Rename `reactjs-jsx-ppx` to `reason-react-ppx` ([@davesnx in reasonml/reason-react#732](reasonml/reason-react#732))
* Fix locations for lower and uppercase components so that merlin / editor
  integration can get type defs on hover ([@jchavarri in reasonml/reason-react#748](reasonml/reason-react#748))
* Refine types for `key` attributes ([@anmonteiro in reasonml/reason-react#750](reasonml/reason-react#750))
* Bump React depext to v17-18 reasonml/reason-react#777 ([@jchavarri in reasonml/reason-react#777](https://github.com/reasonml/reason-react/pull/777/files))
* Make optional props optional properly
 ([@davesnx in reasonml/reason-react#776](reasonml/reason-react@0a98c07))
davesnx added a commit to davesnx/opam-repository that referenced this pull request Sep 13, 2023
CHANGES:

* Migrate the reason-react PPX and library to the [new React JSX
transform](https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html)
  (@anmonteiro in [reasonml/reason-react#714](reasonml/reason-react#714))
* Add `suppressHydrationWarning` to supported props (@davesnx in
[reasonml/reason-react#721](reasonml/reason-react#721))
* Rename `reactjs-jsx-ppx` to `reason-react-ppx` ([@davesnx in reasonml/reason-react#732](reasonml/reason-react#732))
* Fix locations for lower and uppercase components so that merlin / editor
  integration can get type defs on hover ([@jchavarri in reasonml/reason-react#748](reasonml/reason-react#748))
* Refine types for `key` attributes ([@anmonteiro in reasonml/reason-react#750](reasonml/reason-react#750))
* Bump React depext to v17-18 reasonml/reason-react#777 ([@jchavarri in reasonml/reason-react#777](https://github.com/reasonml/reason-react/pull/777/files))
* Make optional props optional properly
 ([@davesnx in reasonml/reason-react#776](reasonml/reason-react@0a98c07))
davesnx added a commit to davesnx/opam-repository that referenced this pull request Sep 13, 2023
CHANGES:

* Migrate the reason-react PPX and library to the [new React JSX
transform](https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html)
  (@anmonteiro in [reasonml/reason-react#714](reasonml/reason-react#714))
* Add `suppressHydrationWarning` to supported props (@davesnx in
[reasonml/reason-react#721](reasonml/reason-react#721))
* Rename `reactjs-jsx-ppx` to `reason-react-ppx` ([@davesnx in reasonml/reason-react#732](reasonml/reason-react#732))
* Fix locations for lower and uppercase components so that merlin / editor
  integration can get type defs on hover ([@jchavarri in reasonml/reason-react#748](reasonml/reason-react#748))
* Refine types for `key` attributes ([@anmonteiro in reasonml/reason-react#750](reasonml/reason-react#750))
* Bump React depext to v17-18 reasonml/reason-react#777 ([@jchavarri in reasonml/reason-react#777](https://github.com/reasonml/reason-react/pull/777/files))
* Make optional props optional properly
 ([@davesnx in reasonml/reason-react#776](reasonml/reason-react@0a98c07))
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
CHANGES:

* Migrate the reason-react PPX and library to the [new React JSX
transform](https://legacy.reactjs.org/blog/2020/09/22/introducing-the-new-jsx-transform.html)
  (@anmonteiro in [reasonml/reason-react#714](reasonml/reason-react#714))
* Add `suppressHydrationWarning` to supported props (@davesnx in
[reasonml/reason-react#721](reasonml/reason-react#721))
* Rename `reactjs-jsx-ppx` to `reason-react-ppx` ([@davesnx in reasonml/reason-react#732](reasonml/reason-react#732))
* Fix locations for lower and uppercase components so that merlin / editor
  integration can get type defs on hover ([@jchavarri in reasonml/reason-react#748](reasonml/reason-react#748))
* Refine types for `key` attributes ([@anmonteiro in reasonml/reason-react#750](reasonml/reason-react#750))
* Bump React depext to v17-18 reasonml/reason-react#777 ([@jchavarri in reasonml/reason-react#777](https://github.com/reasonml/reason-react/pull/777/files))
* Make optional props optional properly
 ([@davesnx in reasonml/reason-react#776](reasonml/reason-react@0a98c07))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

merlin and RLS broken for JSX3 components
2 participants