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.

Sign up for GitHub

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

Add locations-check test #844

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Add locations-check test #844

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e01cd37
add locations-check test
jchavarri Jul 1, 2024
f3d05f2
update tests
jchavarri Aug 15, 2024
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
66 changes: 66 additions & 0 deletions ppx/test/locations-check.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
Test the preprocessed reason-react components have well-formed locations.
Uses https://github.com/ocaml-ppx/ppxlib/blob/44583fc14c3cc39ee6269ffd69f52146283f72c0/src/location_check.mli

With no annotations (ppx does nothing)

$ cat >input.ml <<EOF
> let make ~foo ~bar =
> (div
> ~children:[ React.string foo; bar |> string_of_int |> React.string ]
> ())
> EOF

$ reason-react-ppx -check -locations-check input.ml
let make ~foo ~bar =
div ~children:[React.string foo; (bar |> string_of_int) |> React.string] ()

With JSX annotation

$ cat >input.ml <<EOF
> let make ~foo ~bar =
> (div
> ~children:[ React.string foo; bar |> string_of_int |> React.string ]
> () [@JSX])
> EOF

$ reason-react-ppx -check -locations-check input.ml
File "input.ml", line 2, characters 3-6:
2 | (div
Copy link
Member

Choose a reason for hiding this comment

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

is this expected? What exactly is this test testing if these are errors?

Copy link
Member

Choose a reason for hiding this comment

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

or are these things we should put on our TODO list to fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rather than having to write "manual" tests in a per-case basis like in #842.

^^^
Error: invalid output from ppx, expression overlaps with expression at location:
File "input.ml", line 2, characters 2-96:
[1]

With @react.component annotation

$ cat >input.ml <<EOF
> let[@react.component] make ~foo ~bar =
> (div
> ~children:[ React.string foo; bar |> string_of_int |> React.string ]
> () [@JSX])
> EOF

$ reason-react-ppx -check -locations-check input.ml
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:
Copy link
Member

Choose a reason for hiding this comment

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

Woa

File "input.ml", line 1, characters 33-36:
[1]

Everything

$ cat >input.ml <<EOF
> let[@react.component] make ~foo ~bar =
> (div
> ~children:[ React.string foo; bar |> string_of_int |> React.string ]
> () [@JSX])
> EOF

$ reason-react-ppx -check -locations-check input.ml
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:
File "input.ml", line 1, characters 33-36:
[1]
Loading