fix: minimise bracket printing in case of string operands of binary operator

[algomaster99]

Fixes #4807

I have tried to reproduce #4807 with a related test case.

Expected: java.lang.System.out.println("1" + "2" + "3" + "4")
Actual: java.lang.System.out.println("1" + ("2" + "3" + "4"))

I have noticed that the test pass when I change each operand in the above expression to an integer, thanks to #3823.

[algomaster99]

I think I have figured out why it is happening. To understand it, let's consider a working case first - "1 + 2 + 3 + 4".

1 + 2 + 3 + 4

The above expression is modelled as:

                                +
                    +                    4
           +               3
     1           2

So everytime nestedOperatorRequiresRoundBrackets is called for each nested operator, the associativity and positionInParent are equal and thus no brackets are printed (the operator precedence is obviously equal).

"1" + "2" + "3" + "4"

This is the failing test case. We expect "1" + "2" + "3" + "4" to be printed, however, "1" + ("2" + "3" + "4") is printed. I feel this has to do with how spoon models this expression.

                                 +
                    1                     +
                                  +               4
                           2           3

For the first nested operator, besides "4" in this case, there is a difference in associativity and positionInParent and so the brackets are put to enclose "2" + "3" + "4".

The resolution would be to see why there is a difference in AST modelling of the two expressions and probably fix it if it turns out to be a bug. @slarse do you also agree that the difference in behaviour arises from how Spoon is modelling nested binary operators?

[algomaster99]

The minimizeRoundBrackets was false for this test case. Hence, I rearranged the brackets.

[algomaster99]

I have done exactly what I have explained here. I ensured that the left operand takes the nested operator whenever possible.

[algomaster99]

I have not put the exact test case as listed in the issue, but I feel the one I added in the PR captures the essence and should fix it. @slarse @MartinWitt @I-Al-Istannen @SirYwell please review it when you get time.

P.S. Is there a team for maintainers so that I can collectively tag them?

[I-Al-Istannen]

I don't see any justification for the reordering in the introducing commit, which apparently was meant for switch case statements: 25156e1.

Based on your example the current behaviour seems questionable. It should be semantically correct, as + on string literals is well-defined and associative, but the AST differs from the underlying code. I am not sure if this can be classified as a bug necessarily, but I'd welcome a change.

[slarse]

@algomaster99

P.S. Is there a team for maintainers so that I can collectively tag them?

No, there isn't. There isn't really a strong use case for it. We go over PRs for review if they are prefixed with review:, and usually maintainers will ping each other if they know one or the other is more well equipped for any particular review.

@I-Al-Istannen

I am not sure if this can be classified as a bug necessarily, but I'd welcome a change.

Well, if it builds a non-equivalent AST (even if semantically equivalent), I'd say it's a bug. It makes string concatenation look right associative, which it technically isn't. Although it also technically doesn't matter because there's only one operator involved in concatenation of literal strings.

[slarse]

@algomaster99 Thanks for the PR. Could you add a minimal test case demonstrating the problem on the AST level? That is to say, given a literal string concatenation "a" + "b" + "c", we expect the AST binop(binop("a", "b", +), "c", +).

[slarse]

Thanks @algomaster99 , lgtm!

I'm adjusting the commit message from the PR title as this isn't really about pretty-printing (the printer was doing the right thing by the AST all along).