feat: print minimal amount of round brackets in sniper mode

[slarse]

Fix #3809

Related to #3811

This PR provides functionality in the default pretty-printer for printing only parentheses that are strictly necessary to preserve the syntactical structure of the AST in nested operators. Do note that this applies only to nesting of operators that are actually modeled as operators in Spoon (i.e. CtUnaryOperator and CtBinaryOperator). Redundant parentheses will therefore still be printed around e.g. casts, fixing that is a TODO!

As a brief example, all of the following expressions are printed exactly as given when parsed and re-printed:

1 + 2 + 3
1 + (2 + 3)
1 + 2 + -3
1 + 2 + -(2 + 3)
"Sum: " + (1 + 2)
"Sum: " + 1 + 2
-(1 + 2 + 3)
true || true && false
(true || false) && false
1 | 2 | 3
1 | (2 | 3)
1 | 2 & 3
(1 | 2) & 3
1 | 2 ^ 3
(1 | 2) ^ 3

Note that e.g. 1 + (2 + 3) is re-printed as 1 + (2 + 3). Even though 1 + 2 + 3 is semantically equivalent and requires fewer parentheses, it provides a different AST (see more elaborate description in #3809).

This functionality is currently disabled by default, and can be activated only with the package-private method DefaultJavaPrettyPrinter::setOptimizeParentheses(bool). In other words, this is strictly internal at the moment and can't be activated by clients.

I tried running with the entire test suite with parenthesis optimization enabled, and only 7 tests fail, all due to having redundant parentheses in the assertions. In terms of only the Spoon test suite, it therefore seems feasible to enable this feature right away, and the Jenkins tests should be pretty swift in telling us that something has gone wrong.

@jrfaller Any thoughts on the approach taken here? This is pretty much a straight implementation of the idea I laid out in #3809

@monperrus Would you like me to go for enabling this feature by default, or should we keep it internal/semi-internal for the time being? To be clear, my intention here is that minimizing the amount of printed parentheses should be the way DJPP works, it shouldn't be configurable. Given the binary choice of on or off, I can't imagine anyone would choose to turn parenthesis minimization off, and so it seems reasonable to me to omit such configuration entirely.

[slarse]

This dependency is necessary for using the @ParameterizedTest annotation.

[jrfaller]

Hi @slarse! It seems neat! I don't see in the diff when this method is triggered but I believe this will only be applied to transformed nodes, to keep the unmodified code with its original parentheses?

[slarse]

Hi @slarse! It seems neat! I don't see in the diff when this method is triggered but I believe this will only be applied to transformed nodes, to keep the unmodified code with its original parentheses?

In terms of sniper printing, yes. This patch applies to the default printer (DJPP = DefaultJavaPrettyPrinter), which has its own printing style, and is what we must use when we can't sniper print for some reason (modified node, new node, etc), or when we just don't want to sniper print.

In terms of the current state of affairs, it will also improve the sniper printing itself in some cases, as there will be far fewer parentheses to try to match to actual parentheses that exist in the source code. Parentheses should now never appear out of thin air, as has previously been a problem both in default printing and sniper printing. It will be less beneficial for sniper printing if we manage to implement our ideas in #3794, but it'll still be useful when we need to print new or changed expressions.

[jrfaller]

Thanks for the explanation! Therefore LGTM!

[slarse]

@monperrus This is ready for your consideration. Here are a a couple of thoughts I have:

• The word "brackets" is used internally in the printers. I think "parentheses" is more precise, as brackets more often refer to square brackets [], while I've never heard parentheses refer to anything but (). Thoughts?
• Currently, the parenthesis optimization is enabled by default for the sniper printer, but not for the default printer. There is no way to use the public API to enable it, as I didn't want to introduce more clutter to the DJPP API. Keep this way, or enable for DJPP, or make it possible to enable through DJPP API?
• I'm not sure "optimization" is the best word to use here, it's a bit vague. Minimization? Redundancy elimination?

[monperrus]

Thanks a lot @slarse

The word "brackets" is used internally in the printers. I think "parentheses" is more precise, as brackets more often refer to square brackets [], while I've never heard parentheses refer to anything but (). Thoughts?

Both are valid AFAIK, but it's good to be consistent. All comment and name changes outside the public API are welcome!

Currently, the parenthesis optimization is enabled by default for the sniper printer, but not for the default printer.

This is good this way.

I'm not sure "optimization" is the best word to use here, it's a bit vague. Minimization? Redundancy elimination?

I like "Minimization" (optimization is either minimization or maximization)

Overall, excellent PR, and we love parametrized tests! Will merge.

[slarse]

Thanks a lot @slarse

Both are valid AFAIK, but it's good to be consistent. All comment and name changes outside the public API are welcome!

Yes, both are valid: "bracket" is British and "parenthesis" is American English. The problem I find in Spoon is that the word "bracket" is used for everything (square bracket, round bracket, curly bracket are all just denoted bracket). This is a bit confusing to me when reading the code. This is all internal, though, I couldn't find a single mention in the public API.

However, I think that this PR is not the one to change widespread terminology use in Spoon. I'll use the term "round bracket".

Currently, the parenthesis optimization is enabled by default for the sniper printer, but not for the default printer.

This is good this way.

Right, so just to be clear here, it also can't be enabled for the default printer via the public API as the setter is protected. That's intentional, I think it's good we keep this internal until it's been field tested a bit in the Sniper.

I like "Minimization" (optimization is either minimization or maximization)

That is true. I'll migrate over to use "minimization".

[slarse]

On the note of reproducing formatting under AST transformations, a6a43e9 and 73338f7 show that IntelliJ isn't perfect either ...

[monperrus]

Thanks a lot @slarse . This is a very important contribution for the sniper mode.