Add formatter for the parse AST

[LaurenzV]

No description provided.

[LaurenzV]

So, overall, I think I've managed to get rid of a bunch of overrides, thanks for the hints, they helped a lot! :D However, there still are quite a few remaining but I think it will be rather hard to remove those... :/ I'll try to add explanations for some of them below.

Some other notes:
Unfortunately, I couldn't reuse leftPad/rightPad in all instances, because there are a few examples where we need to deviate from the pretty printer (for example, the pretty printer by default wraps any binary operator with spaces, but for the reformatter we handle operators like + and ==> differently, so always padding with spaces messes things up). So I took the route of using the padding of the pretty printer by default, but override it for a few select cases.

Regarding some of the overridden reformats:
PGrouped, PDelimited: As you can see in the code there's quite a few different cases that need to be distinguished, so I don't think this can easily be removed.

PFunctionType: We can't reuse the pretty impl here because it would call pretty on all children as well.

PExp/PBinExp: For those we also have different code paths depending on what exactly we are processing, so it's again hard to find a generalization for these.

PCondExp,PUnfolding,PApplying,PAsserting, etc.: For those (i.e. the ones with PExp), the difficulty is that the nesting we apply does not really have a clear general pattern, so I'm not exactly sure how you would generically apply the grouping. For example, for unfolding you have a single expression which is nested, grouped and then prepended with a line (which will evaluate to either a space or a new line depending on the situation), while for PCondExp, we have two expressions, out of which the else part shouldn't get an additional layer of nesting. Also, always prepending a line to an expression is also not desirable, for example, if you have a field access somewhere in your code, this shouldn't automatically have a prepended whitespace.

PIf, PWhile, etc.: Those also have somewhat conditional formatting. For example, if a while loop has invariants, then the opening bracket should always be on a newline, while if there aren't any invariants it looks nicer if it's on the same line.

PDefine: I don't think I can remove the manual space here since adding spaces to either PDelimited or even PNode will break a lot of other things.

PDomainFunction, PExp: Same here, adding a space by default to either PIdnDef or PDelimited or PBracedExp would break lots of other things.

Similar problems apply to the remaining ones.

Regarding your comments:

I would change leftPad and rightPad to be Boolean (I think currently they only are "" or " ") and then add a space to the left/right based on that.

I changed that!

You could also add a addNewline field which is set for e.g. all stmt keywords and function/method/requires etc. which adds a newline before the keyword.

I tried this, unfortunately this doesn't seem to work very well in my case... The problem is that I had to implement a somewhat sophisticated logic to preserve newlines in the existing program while also respecting the intended formatting, and it doesn't work well if I just always prepend a newline. Just to give you an idea, if you have for example:

field x: Int
field y: Int

After reformatting, it should preferably stay the same, while if you have something like:

field y: Int



predicate StructA(this: Ref) {
  acc(this.x) && acc(this.y)
}

The reformatted result will now look like:

field y: Int

predicate StructA(this: Ref) {
  acc(this.x) && acc(this.y)
}

instead of:

field y: Int
predicate StructA(this: Ref) {
  acc(this.x) && acc(this.y)
}

Things get even more complicated if there are comments in between the members where I need to look at the whitespaces before and after the outer comments, so this is why the logic for reformatting is pretty delicate, and just prepending a newline before each members unfortunately trips it up.

Lastly, I would define a some things as "grouping" in the sense that they add two spaces after a newline for all contained RNodes, which would probably handle most cases of indentation (I think you already do this below, but I'm not 100% sure).

Yes I have the rne() function which will indent everything that appears in-between, but as mentioned above there are kind of a lot of edge cases with different constructs having different preferred nesting rules, so generalizing those doesn't seem easy to me... The reason the pretty printer has much less of those is that it simplifies a lot, and therefore the output isn't great in many cases and often even produces syntactically broken results (I tried it). But for reformatting, it's obviously important that the output looks nice and the program stays syntactically valid.

Let me know what you think, I hope it's at least an improvement to before.

[rayman2000]

@LaurenzV Jonas and I discussed this and we think we should not remove the pretty() calls (which would break ViperServer) but wherever possible, have pretty() just call reformat() internally so that we do not have to parallel implementations of almost the same thing. Could you take a look at that?

[LaurenzV]

That's already what happens, see here: https://github.com/LaurenzV/silver/blob/063279a982dfd0d1c2af49c0e009610f290e7500/src/main/scala/viper/silver/parser/ParseAst.scala#L41-L47.

All of the remaining overrides are (as mentioned above) more complex and thus I'm not really sure how I can remove them without sacrificing the output quality of the reformatter.

[LaurenzV]

Ah, sorry. I thought you meant reformat should call pretty, not the other way around. I can look into that!

[rayman2000]

Ah sorry, I misunderstood that. It looks like reformat calls pretty and not the other way around? But that is also fine. So then this is ready from your point of view?

[LaurenzV]

Yes, right now, reformat will call pretty as a fallback. I can of course try letting pretty call reformat instead, though it might be a bit tricky because the reformatter also expects a reformatter context with information about comments/whitespaces in the program, which doesn't really exist if we pretty print a node individually. So without trying it, I can't tell how feasible it is.

Otherwise, this would be ready from my side. :)

[rayman2000]

Yeah I think you can leave it as is, the main goal is to not have duplicate stuff, the direction does not matter too much to me.
@JonasAlaif can you take another look

[JonasAlaif]

@LaurenzV see my last commit. I've changed things to be even more in the direction that I suggested. The expected formatting changed a tiny bit (check that it looks reasonable, or revert it as you wish). There is still one failing test that I couldn't figure out - maybe you could have a look.

If there are parts you don't like about this version, I'm happy to discuss alternative solutions :)

[LaurenzV]

Thanks a lot! Seems like you had to change a lot, sorry about that, I didn't have any experience with Scala or Viper before, so I imagine a lot of the code was unidiomatic. 😅

I have fixed the issue with the failing silver test, though it seems like your changes to the parsing and macro expansion logic are causing CI failures, I thought this would fix it, but looks like it doesn't fully solve the issue, so I've reverted it for now. Any idea what the best solution to the problem is? I can try to look into it again tomorrow or so, but given that I'm not at all familiar with that part of the code base, I'm not sure I would be able to figure out the right solution fast.

Other then that, I don't have any objections to your changes, especially since the test cases still pass, which do have a pretty wide coverage.

[LaurenzV]

Since some of them are match errors, it might mean we need to make changes in the Silicon test suite? Not sure if that's acceptable to change, though.

[JonasAlaif]

I think that most of the errors are stemming from the change to PIdnUseExp, it used to be both an PExp and PIdnUseName. I changed it to be a PExp only which wraps a PIdnUseName, since then there aren't two conflicting implementations of reformat (i.e. the PLeaf one from PIdnUseName and the PExp one). I think the change makes sense, but there are various places which match a _: PIdnUseName expecting the PIdnUseExp to match, but it no longer does. I think that this is likely the cause for both the match errors and the macro expansion errors. I tried to change the occurrences of this match in the macro expansion code, but it seems that I must have missed something.

I couldn't find the parse error. Could you send a link to the CI of that one?

Regarding changing the SIlicon test suite, it's a bit annoying but definitely in scope for this PR.

[LaurenzV]

I couldn't find the parse error. Could you send a link to the CI of that one?

Ah no, I didn't mean that there is a specific error related to parsing, just that as part of those changes the CI failures appeared. It's possible that they are all just related to the PIdnUseExp thing.

Okay, in this case I will give it another shot a bit later and try to fix the failures. Thanks again!

[LaurenzV]

Okay, I've figured the Carbon stuff out, only the match problem remaining. I've opened a PR, with that those tests pass locally for me: viperproject/silicon#908

Once that's merged, we can rerun CI and hopefully merge this PR.

[rayman2000]

@JonasAlaif are you happy with this now? You still have "requested changes" but I think everything has been addressed.