Match compilers dummy loc

[patricoferris]

A fix as per #532.

I tested reverse dependencies (I think most of them, for their latest version) and they mostly installed --with-test. Any that didn't seemed unrelated.

[NathanReb]

Looks good to me!

This probably needs a changelog entry!

[NathanReb]

Seems some tests need to be updated as well!

[NathanReb]

Also, rebasing it will reduce the diff as the quoter tests don't print out the whole AST anymore!

[patricoferris]

Turns out this actually changed in OCaml compiler versions, so this is not the proper fix.

• <= 4.07 it matches what we have, see for example https://github.com/ocaml/ocaml/blob/4.07/parsing/location.ml
• >= 4.08 it switched to used Lexing.dummy_pos

The fix might be fairly easy (using Cinaps) but the problem becomes all of the expect tests. And whilst we could in theory do the [%%expect_in] stuff I think our tests become less and less useful the longer they get and they're filled with none locations. It would be great to run Toploop.execute_phrase and then parse values and use Pp_ast on them... Any ideas @NathanReb ?

[NathanReb]

I guess we could use [%%ignore] here, similar to what I did for the Quoter tests:

let x = [%expr ...]
[%%ignore]

let () = Pp_ast.Default.expression x
[%%expect {|
...
|}]

Would that work for you?

[patricoferris]

It actually turns out I don't need that @NathanReb -- the files are only enabled for new enough compilers that it doesn't matter. But it is good to have that trick up my sleeve should we need it. Happy to make that change here to help future proof these tests (but I don't think it is likely for those bits to change too much).

[NathanReb]

If I understood the issue correctly, the important part is to agree with the compiler on what is Location.none right?

If so then we could probably simply stick to using the compiler's functions to build none locs.

This won't work when reading AST emitted by a different compiler (which we in theory support) but even that could be fixed by versioning the none loc.

As I'm writing this I wonder if the right fix isn't the following:

1. Stick to having our stable Location.none and choose a convention for it
2. Have a function that detects whether a location is a valid none loc from any version we support
3. Use the function from 2 instead of poly = in our codebase

What do you think?

[patricoferris]

After some discussion with @NathanReb this PR now does the following:

Location.none has been changed to match the compiler's Location.none as
of OCaml 4.08. This has been stable for some time. We also provide a
Location.is_none which checks if a location is equal to Location.none
for all versions of the compiler we currently support (i.e. >= 4.04).

The decision to change Location.none has been made to enable the code to
be simplified if we decide to change the lower bound of ppxlib to 4.08
in the future.

Location.is_none has been used to fix loc_of_attribute which would
sometimes miss attributes with none locations (like compiler inserted
ocaml.doc attributes). There is a test for this fix.

[patricoferris]

The current errors we're seeing on 4.04 and 4.05 are due to newer compiler versions checking of a line number is "valid" or not (i.e. > 0) and replacing it with 1 if not... https://github.com/ocaml/ocaml/blob/7a9d83047450e337ff31f3d73e5c2d9af66bd363/parsing/location.ml#L214

On the older compilers, the location is used directly, hence we are getting 0 with this new change. Possible solutions are to:

• manually change the line number location
• revert back to making Location.none match the compiler's Location.none

any thoughts @NathanReb ?

[NathanReb]

Given these are errors we produce ourselves and they point to an actual file, I guess we could directly have those errors point to line 1. Maybe we can factor this out in an internal Location.raise_error_in_filef: file: string -> ... kind of function.

How does that sound?

[NathanReb]

That being said, I'm not particularly against using the matching compiler's Location.none if you think that's the better approach!

[patricoferris]

@NathanReb -- as we discussed, since ppxlib only supports compilers 4.08+ we no longer need to handle multiple possible values for Location.none which greatly simplifies this PR. We're making a bit of a bet on it not changing in the future but I think that's ok.

[NathanReb]

Perfect, thanks!

I also think it's acceptable to bet on Location.none not changing again. If it does we can experiment with the other suggestions, e.g. migrating Location.none between versions or have Location.is_none accept a range of representations.