About Good Practices... (locations check related issues)

[Kakadu]

I read a manual recently, and I created some questions.

• AST nodes are requested to be well-nested WRT locations
• the locations of "sibling" AST nodes should not overlap

The second point sounds reasonable but over restrictive. I'm worried that ghost locations may damage error reporting in the generated code. Maybe banning overlap modulo being the same location will not damage merlin?

Also, I'm confused about how to debug issues when they come. Previously, I passed -dparsetree to compiler flags to see parsetree but now ppxlib fails earlier, and I don't know what to do. (Removing rewriter switches -check and -locations-check helps, is there a better solution?) I thought about copypasting the command with rewriter, but it looks like directories _build/.sandbox/40b4a257626a7d996349a54dd4dcc8b9 are silently deleted. (Don't remember they have been deleted before, but maybe it is gallucination)

$ (cd _build/.sandbox/40b4a257626a7d996349a54dd4dcc8b9/default && .ppx/3d91456aa8e7df8080cfbc1aaf103b6e/ppx.exe -check -locations-check --cookie 'library-name="test_repr"' -o lib/test_repr.pp.ml --impl lib/test_repr.ml -corrected-suffix .ppx-corrected -diff-cmd - -dump-ast)
File "lib/test_repr.ml", line 1, characters 19-31:
1 | let __1 : _ = REPR (fun _ -> 1)
                        ^^^^^^^^^^^^

This is the repo with my attempts, if anybody interested. https://github.com/Kakadu/ppxlib_check_demo There are two rewriters there. ppx_repr fails because equality of siblings' locations is forbidden. Why the demo about ppx_fresh fails? (I have no idea)

Minor stuff

• spelling

# let quoted_part = Quoter.quote quoter part_to_quote ;;
val quoted_payload : expression =

• "-check and -check-locations" should be "-check and -locations-check"

[NathanReb]

Thanks for taking interest in ppxlib and for reporting this!

I'm not quite sure I understand the problem you ran into. Is it that you're trying to write a ppx and it seems to break the location invariant?

Could you provide the steps to reproduce this with a minimal example?

Some elements to help you here, in case I understood correctly:

1. Since ppxlib.0.33.0 you are not required to pass -check in addition to -locations-check, you simply run the driver with -locations-check. This might help avoiding noise in your quest!
2. Indeed, for generated code, locations of siblings should be allowed to share the same locations, as this is what most ppx will do and have all the generated code point to the extension point location, or the type definition with [@@deriving ...] attached for instance. If it makes the location check fail, it might indeed be a bug!

[NathanReb]

I've taken a look at your repo. The failure with ppx_repr was expected, since the locations weren't marked as ghost anywhere, the location check failed because indeed the siblings had overlapping locations and weren't descendants of a ghost loc node.

If you set the location of the generated node to ghost, the error goes away.

I'm a bit more puzzled by the ppx_fresh example as it seems it's not doing anything in particular and therefore the error is a bit puzzling:

$ dune build @all
File "lib/test_fresh.ml", line 1, characters 23-24:
1 | let __1 : _ = fun _ -> 1
                           ^
Error: invalid output from ppx:
       this expression is built from a pattern whose location is outside of this node's.
Child pattern found at:
File "lib/test_fresh.ml", line 1, characters 18-19:

[Kakadu]

@NathanReb I looked again, and I seems to understand better what happening. I agree that pps_repr was bad, not it doesn't complain. It reports compilation error in generated code in decent locations, as I desired. (I was reading manual not carefully enough, my bad.)

About ppx_fresh I'm puzzled too. Which expert should we @ mention in this conversation?

[NathanReb]

I just started working on a better AST pretty-printing utility for ppxlib here: #517.

I'll try to use it to debug what's going on with ppx_fresh and get back to you!

[NathanReb]

I just fixed it here: Kakadu/ppxlib_check_demo#1

It was a simple bug and indeed the locations were off!

[NathanReb]

If you don't have any further questions, I think we can close this issue!

[Kakadu]

Hey, @NathanReb

I have a small trouble with rewriters that use let* in the code. In the repo I made a commit where I really don't rewrite anything...

open Ppxlib

let mapper =
  object (_self)
    inherit Ast_traverse.map as super
  end

let () = Ppxlib.Driver.register_transformation ~impl:mapper#structure "asdfasdf"

but for the following code

let () =
  let ( let* ) x f = f x in
  let* x = "asdf" in
  print_string x

I still get and error message about locations.

File "lib/test_fresh.ml", line 4, characters 2-16:
4 |   print_string x
      ^^^^^^^^^^^^^^
Error: invalid output from ppx, expression overlaps with binding operator at location:
File "lib/test_fresh.ml", line 3, characters 2-37:

It looks like it only happen for AST nodes Pexp_letop .... Is my rewriter wrong again?