search

ocaml-ppx / ppxlib

Base library and tools for ppx rewriters
MIT License
244 stars 94 forks source link

Feature request: locations integrity check tool #505

Closed jchavarri closed 2 weeks ago

jchavarri commented 2 weeks ago

Merlin expects locations to be well-formed. The only reference to what "well-formed" means that I could find is here: https://github.com/ocaml/ocaml/pull/8987. Copying for posterity:

As it turns out, a non negligible portion of this complains is actually due to a ppx misbehaving! Here is what's happening: to answer any query, merlin starts by finding the ast node which corresponds to the position of the cursor in the user's buffer. For this to work well, two conditions must be respected:

  1. for any two AST nodes m and n, if n is a descendant of m, then n.loc is included in m.loc

  2. for any two AST nodes m and n, if they have the same "parent" then n.loc and m.loc must be disjoint; note: "parent" here is not necessarily the direct parent of the node, it is actually the closest ancestor whose location is not ghost (:ghost:)

I wonder if ppxlib could include a check_locations_integrity tool to check the locations of a given AST to make sure they comply with these rules. This tool would be useful for both ppxlib maintainers and the larger ecosystem of ppx authors alike.

As an example, right now in reason-react we have to analyze full trees of AST nodes doing this work manually, which is very time consuming.

See related: #503 and its fix #504 where a small subset of what this tool could look like is implemented.

NathanReb commented 2 weeks ago

I didn't think about it when reviewing #504 but we indeed have something like this in the codebase already, see location_check.mli.

It's disabled by default but you can enable it by passing -location-check to the driver invocation in which case it will perform the check on the fully transformed AST.

Let me know if that fits your needs!

I'll refactor the tests for your migration fix to use this instead!

jchavarri commented 2 weeks ago

😮 wow, TIL. Thanks!

NathanReb commented 1 week ago

For reference, to get this to work you need to pass both -check and -locations-check to the driver, passing simply the latter won't work. I'll look into fixing this!

jchavarri commented 1 week ago

I noticed :) thanks for the heads up!

NathanReb commented 1 week ago

I opened a PR with a fix: #506

I'll include this in the next release!