Keep track of locations in the AST

[Armael]

This is WIP for keeping track of locations in the AST.
The motivation for this is ocsigen/tyxml#128 .

The core feature itself is implemented, namely producing an AST whose nodes are annotated by locations. What remains is polishing the API & tooling, and for that I thought I'd ask before implementing something potentially unsatisfactory.

Remarks/questions:

• This changes the lexer behavior a bit; RAW tokens now split at newlines. Considering that RAW tokens can already be split by '}'/'{', only because of an implementation detail (as far as I can tell), I thought it wouldn't be a problem;
• I export the Mustache.t type; I guess make Mustache.t non abstract #18 should be merged first, then this PR rebased on top of it;
• At the moment all the helper functions in the Mustache module ignore locations, and put a dummy value when one is needed. I'm not sure what would be a nicer API for these (which would probably be incompatible with the current one);
• I did not fix the tests yet, they currently fail as the helper functions from Mustache insert dummy locations, which do not match the actual locations provided by the parser. One solution I see would be to directly hand-write the AST (tedious), or improve these helper functions (equally or less tedious depending on how they are improved, I guess).

[rgrinberg]

Thanks for reminding me about #18. I've merged it so you can now rebase.

At first glance, your patch looks alright but the way you've added locs looks a bit clumsy to me. Why not have a type like type 'a loc = 'a * Loc.t and we can keep our old clean mustache type for uses where the loc info isn't needed.

@Drup you're a loc expert, could you please comment about the above?

[Armael]

Rebased.

About locations, I'm not sure to understand what you mean. Do you want to have locations separate from the mustache AST? I'm not sure how to do that, given each node of the AST is supposed to be annotated with a location.

[rgrinberg]

Ah yes, you're right. Shouldn't have commented in haste.

It's a been annoying to have to impose locations on people who won't necessarily
care about them. But working directly with the AST is a rare enough use case.

Will have a closer look at this in the next couple of days.

In-Reply-To: rgrinberg/ocaml-mustache/pull/21/c240966472@github.com

On 08/19, Armael wrote:

Rebased.

About locations, I'm not sure to understand what you mean. Do you want to have locations separate from the mustache AST? I'm not sure how to do that, given each node of the AST is supposed to be annotated with a location.

You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#21 (comment)

[Drup]

I would have done something like:

type desc = ... <original version of t> ...
and t = { loc : loc ; desc : desc }

It simplifies things a bit, and avoid repetitions.

[Drup]

Also, it may be a good idea to have two datatypes, one AST with loc (and that could be much more precise), and one that is just the (simplified) datatype, appropriate for manipulation in OCaml.

[Armael]

Would that mean duplicating the parser? An other option is to have a function that takes an annotated AST and produces a simplified AST, by removing locations (and the parser would produce annotated ASTs).

[Drup]

The second solution, of course.

[Armael]

I'm sorry it took me so long to get back to this.
I pushed changes that I hope implement Drup's advice. The API should be backwards compatible: the mustache type without locations and associated functions are still there, and the new things are in a new Mustache.With_locations module. Functions to convert between the two datatypes are provided.

Existing tests pass; I did not add tests checking locations at the moment.

[Armael]

I added a test doing a roundtrip add_dummy_locs -> erase_locs.

[rgrinberg]

I thought a tuple for the loc would be more natural. But it doesn't matter so much.

@Drup Would you mind taking a look? It looks good to me but it would be good to have your opinion here.

[Drup]

I adapted my mustache-ppx to the new module With_locations and it works very well. Tyxml type errors are reported at the correct places in the html, through mustache. I gave a very quick look at the code, and it looked fine.

One nice (future ?) addition would to raise/return a better error when parsing fails, including the location of the failure.

[rgrinberg]

One nice (future ?) addition would to raise/return a better error when parsing fails, including the location of the failure.

Probably future addition If I was implementing it. I have no idea how to accomplish this without rewriting the parser by hand.

[rgrinberg]

Thanks @Armael