fix a few failures when printing a modified AST

[v-gb]

No description provided.

[v-gb]

The failure might be because of carriage returns on windows. But before figuring out what to do, I'd like to know if the approach is reasonable.

Also it might be useful to see what comes out of ocaml/ocaml#13833, because the parser is producing loc_ghost in cases that don't make sense to me, so I'm not sure that the use of loc_ghost to distinguish modifications made to the AST always work (I got a strange test change when using loc_ghost in Fmt_ast.sequence_blank_line).

[Julow]

Perhaps Source.loc_of_first_token_at shouldn't attempt to read the source at all and always return None for ghost loc ? I don't see other impossible cases related to these functions returning None.

Also, this relocate_ext_cmts shouldn't raise errors at all as it's just using heuristics to move comments around. The impossible case should be removed.

[v-gb]

I removed the impossible case.

The problem with checking loc_ghost earlier is that the upstream parser sets loc_ghost incorrectly, and the bug is inherited in the ocamlformat parser, and the result is blank lines or comments moving around.
Specifically, the tests that break use begin%test e end in one case and new%foo e in the other, and in both case, we're exercising the same parser semantic action that creates Pexp_extension with a ghost location, and the inner expression with a real location, whereas the opposite is what would be correct.

[v-gb]

In general, I think the use of loc_ghost is not unreasonable, but the most principled solution would be to make the printer work using the ast only, without the side information from Source.t or Cmts.t.

This can be seen with this thought experimental: imagine that you're combining files with print (parse Structure file1 @ parse Structure file2). In principle, this should have the same effect as In_channel.read_all file1 ^ "\n" ^ In_channel.read file2. But in practice, either you set loc_ghost on one ast and lose part of the formatting, or you don't and instead risk the printer raising exceptions.

Whereas if the ast was self contained, the concatenated asts would get printed the same as the concatenation of the source files, modulo the whitespace at the boundary. I don't know how complex that would be, but in this function, it would probably mean that the Pexp_extension should have a loc_of_first_token field.

[Julow]

Right!

but the most principled solution would be to make the printer work using the ast only, without the side information from Source.t or Cmts.t.

That was once a goal but we're not there yet as it's a lot of work. The modified AST is closer to the concrete syntax than the upstream parser but not enough to stop reading the source yet.
Any work in that direction is very welcome :)

Comments is a much more complex story. Representing comments at every nodes in the AST was deemed too hard since the beginning.

This can be seen with this thought experimental: imagine that you're combining files with print (parse Structure file1 @ parse Structure file2).

In theory, you can concatenate the Cmts.t as well and not loose the comments.
For the source part, we could make use the filename field of a location but that's just adhoc. We definitely want to get rid of it.

But keep in mind that OCamlformat is a string -> string function. In a much simpler use-case than yours I made it a string -> (ast -> ast) -> string function and haven't had to deal with source and comments. Could that help ?

[v-gb]

but the most principled solution would be to make the printer work using the ast only, without the side information from Source.t or Cmts.t.

That was once a goal but we're not there yet as it's a lot of work. The modified AST is closer to the concrete syntax than the upstream parser but not enough to stop reading the source yet. Any work in that direction is very welcome :)

Yes, clearly doing this everywhere would be a lot of work. But it seems like the kind of work where you chip away at it, and the most common problems get solved, and then you solve the long tail of remaining problems over time.

I don't think it requires bringing the AST close to the concrete syntax though, it only requires capturing the relevant information from the concrete syntax into the tree.
ocamlformat's AST is actually sufficient difficult to manipulate due to some of these choices that I ended up rewriting the AST into a more usable form, processing the ast the way I want, then rewriting the AST back into a "normal" ocamlformat ast. For instance, the fact that functions are represented by 3 nodes (Pexp_function, Pexp_let, Pexp_letop), that opens are represented by 2 nodes, that applications are represented by 4 nodes (Pexp_apply, Pexp_infix, Pexp_prefix, Pexp_indexedop and potentially Pexp_letop) complicate things significantly.

Comments is a much more complex story. Representing comments at every nodes in the AST was deemed too hard since the beginning.

This can be seen with this thought experimental: imagine that you're combining files with print (parse Structure file1 @ parse Structure file2).

In theory, you can concatenate the Cmts.t as well and not loose the comments. For the source part, we could make use the filename field of a location but that's just adhoc. We definitely want to get rid of it.

Ok, good to know!

I agree that comments are likely much harder. ocamlformat handles comments in this way:

1. parse string into ast
2. assign comments to ast locations
3. print ast, considering the comments assigned to the locations

Without being very familiar with these steps, I was thinking that perhaps the second step could turn comments into attributes like [@ocamlformat.comment_before] in various places. But it's very possible that it's not close to good enough.

Anyway, I don't anything urgent to solve, it's just that I've been thinking that using loc_ghost is limited in general. Given:

let x = 1 in
let y = 1 in
(x, y)

If you inlined y, you'd end up with an undesired empty line:

let x = 1 in

(x, 1)

because the position between in and the tuple are 2 lines apart, and yet every node has loc_ghost=false.
Whereas if you Pexp_let stored both the loc_of_in and also loc_of_body (which is redundant with the body, in the ast produced by the parser, but not when you start to modify the ast), then this spurious empty line wouldn't show up.

But keep in mind that OCamlformat is a string -> string function. In a much simpler use-case than yours I made it a string -> (ast -> ast) -> string function and haven't had to deal with source and comments. Could that help ?

Oh yeah, that's what I'm doing as well, but it sometimes causes problems, like the exceptions fixed in this pull request. But I was thinking more generally about "what would it take for ocamlformat to fully support printing modified asts". If the modifications are simple, ocamlformat is pretty reasonable already.

[Julow]

it only requires capturing the relevant information from the concrete syntax into the tree.

That's exactly what we called "closer to the concrete syntax" so far. What we called "CST" in older issues is not a tree full of lexer tokens and snippets of source code.

We found that having different nodes that represent things with similar semantic but different syntax to be extremely useful (Pexp_function, Pexp_let, Pexp_letop or Pexp_apply, Pexp_infix, Pexp_prefix, Pexp_indexedop).
We could change ocamlformat's AST again if that could help your usecase, though we don't want to make ocamlformat harder to maintain.

because the position between in and the tuple are 2 lines apart, and yet every node has loc_ghost=false.

I don't think loc_ghost is relevant here. This is a case where we tried to not use Source and used line numbers instead. To fix this issue, we could perhaps record whether there's an empty line by adding a boolean into the Pexp_let.

[Julow]

I'd prefer cram tests. ppx_expect requires defining a library in Dune, which becomes installable and is annoying to deal with during releases. You can build an executable with Dune that uses ocamlformat-lib within a cram test.

[v-gb]

I'm confused. Libraries are only installable if they have a public name, isn't it?

[Julow]

Libraries become installable (even private libraries) when they have a (package) field. This field might not be needed for ocamlformat right now but I had enough problems with ppx_expect and so much success with cram tests that I really don't want to go back.

[Julow]

Thanks!