Add locations-check test #844
Conversation
| File "input.ml", line 1, characters 33-36: | ||
| 1 | let[@react.component] make ~foo ~bar = | ||
| ^^^ | ||
| Error: invalid output from ppx, core type overlaps with core type at location: |
They do. I just had to modify one location in the ppx tests (seems not harmful). |
Hm just realized this change was already in #850. I forgot to merge |
|
|
||
| $ reason-react-ppx -check -locations-check input.ml | ||
| File "input.ml", line 2, characters 3-6: | ||
| 2 | (div |
There was a problem hiding this comment.
is this expected? What exactly is this test testing if these are errors?
There was a problem hiding this comment.
or are these things we should put on our TODO list to fix?
There was a problem hiding this comment.
Yes, this should go in a TODO list to fix. The idea behind it is that if the ppx creates the AST in a way that this tests pass, we should never have any more problems with editor not finding locations, getting confused about types on hover, etc, as the tree would be consistent.
There was a problem hiding this comment.
Rather than having to write "manual" tests in a per-case basis like in #842.
I found out today that ppxlib has some
-check -locations-checkflag that one can use to make sure a ppx generates an AST with valid locations ("valid" in the sense of following the Merlin rules).I thought this could serve as an alternative to tests like the one @anmonteiro adds in #842 and the many other merlin-based tests that we have.
Rather than building the test based on what merlin returns, we can just make sure the tree is well formed. But it turns out the check doesn't successfully pass (even in 5.1), because there are too many overlaps.
Maybe we can keep the test as is for now and over time solve all issues until we make sure it passed this check. @davesnx @anmonteiro wdyt?