Fix ghost locations for modular extensions

[antalsz]

Modular extensions weren't marking sufficiently many of their new AST nodes as ghost, and ppxlib was breaking. This PR makes all the new nodes ghost except for the single outermost one.

[goldfirere]

Why were no changes required in the immutable arrays and strengthen stuff? Well, in Strengthen, the location flows only to make_extension, so that's OK. But I don't think immutable arrays are necessarily OK.

Also, what's the testing plan here?

Glad this is moving forward!

[antalsz]

The testing plan isn't obvious, since the problems are only with ppxlib and that's not in the compiler. So far I've tested by manually running stuff through ppxlib, and I don't know how to systematize that.

[antalsz]

As for your other questions:

• Immutable arrays never calls wrap_desc, and the new contract is that you have to ghostify yourself if and only if you're constructing an ast (e.g. expression), not an ast_desc (e.g., expression_desc); make_extension is guaranteed to ghostify its argument, and this is documented.
• Module strengthening was just missed in the rebase, but also satisfies this property... mostly. It calls Ast_helper functions with no location, which defaults to Location.none. Does this work? Someone should probably check, but that's exactly your suggestion for not propagating ~loc in comprehensions, so I'll be testing the theory myself shortly.

[goldfirere]

The testing plan isn't obvious, since the problems are only with ppxlib and that's not in the compiler. So far I've tested by manually running stuff through ppxlib, and I don't know how to systematize that.

I was worried about that. Yes, that's true, but is it possible to e.g. copy the sanity check in ppxlib into the testsuite? This bug was a surprise when it occurred and was hard to track down. Adding a good test would avoid at least the first in the future.

[antalsz]

Oh, and a further clarification for immutable arrays: The location is propagated to the underlying mutable array, but this ought to have the location of the whole thing, much as subterms do generally. But I could be argued away from this point of view.

[antalsz]

I talked myself out of the inner mutable arrays being nonghost

[goldfirere]

I talked myself out of the inner mutable arrays being nonghost

Why? It's not that I have a sense of what the right answer is, but rather that it would be good to distill out what your thinking process is so that we can get this right in the future. What are the guidelines for what should be ghost and what shouldn't be? How should new extension authors think about this? We've learned this is all important for correctness.

Example: what happens if a user selects the immutable array literal and queries its type? Presumably, locations are very important for this. I think this will probably work, as long as the outermost application of [%extension.immutable_array] has the location of the original source. (I think?) Actually, this might be an argument for what you decided: make the inner mutable array a ghost. My understanding of Merlin's algorithm for getting from source location (i.e. the range of a user selection) to AST element is to look for the innermost AST element that has a location containing the selection. If the mutable array is non-ghost, then Merlin might decide that the user selected the mutable array, and get a type e.g. int array instead of int iarray. (Is it possible to add this as a test to merlin? That might be hard -- I don't know.)

Do double-check my reasoning above, though -- this is just a rough guess from one merlin debugging session.

[goldfirere]

The more I think about this, the more I think that the outermost location shouldn't be a ghost -- that's got to be the way that merlin identifies when a user selects and entire e.g. comprehension. But I think this needs to be tested before further changes are made here.

And I'm still worried about the testing story; is that something you're looking at? Normally I wouldn't be so worried about tests around locations, but we've had several major bugs recently around locations, so I'm paying them more attention. :)

[antalsz]

I talked myself out of the inner mutable arrays being nonghost

Why? It's not that I have a sense of what the right answer is, but rather that it would be good to distill out what your thinking process is so that we can get this right in the future. What are the guidelines for what should be ghost and what shouldn't be? How should new extension authors think about this? We've learned this is all important for correctness.

Great question. You did articulate why: the outermost application must be nonghost (which is currently handled by the parser), and that will correspond to "the whole thing", every time. If there's a whole thing, we don't need another one, and we should be applying the golden rule: every synthesized node must have a ghost location. I don't know if Merlin could actually get things wrong, but why provide it the possibility for an ambiguity?

[antalsz]

And I'm still worried about the testing story; is that something you're looking at? Normally I wouldn't be so worried about tests around locations, but we've had several major bugs recently around locations, so I'm paying them more attention. :)

As we discussed, I am looking at it, because I'm also worried. I think we clear the first hurdle: ppxlib is MIT-licensed and ocaml is LGPL licensed, so we should be legally able to copy ppxlib code here for a test. But I don't know how practically viable that is; can we realistically minimize the code? And if we do, then we have a slowly-bitrotting copy of ppxlib sitting around, and while I figure these checks should be stable, that's not great. Copying may still be the best option, though; I can't think of another approach.

Thinking about it further, though, I presume ppxlib is tested on the regular OCaml AST. Thus, we'd only need to test it on our weird extensions stuff, and maybe that's fine. Or even, kicking this further down, we should be adding tests to a ppxlib-jst? Keeping those synced would be hard, but maybe a little easier? I'm not sure.

[goldfirere]

There's got to be a discrete little chunk of code in ppxlib that checks for locations, right? Just grab that bit. We don't need anything else.

[antalsz]

There's got to be a discrete little chunk of code in ppxlib that checks for locations, right?

Hardly: there's a 736-line file that implements the check, which has a bunch of dependencies within ppxlib. I've been trying to copy in what I can, but it also depends on Ast_traverse, which depends on Ast_traverse0, which depends on Ppxlib_traverse_builtins and Ast... I'm not sure continuing in this vein is worthwhile

[goldfirere]

After some discussion, @antalsz and I agreed that I would Approve this PR and that Antal would follow up by working out a testing story that lives outside the ocaml repo. I agree that it's just too hard to squeeze the check into the repo, and it's really about compatibility with ppxlib, not a property that ocaml needs for its own uses.