fix: minimise printing parenthesis for type casts in expression

[algomaster99]

Reference: #4881 (comment)

The changes fix bracket printing for expressions with type casts. It actually does two things:

1. Remove outer brackets. Eg. outermost brackets in ((int) myInt) are not printed if minimization is requested.
2. Initially, brackets were enforced around the variable as well. For example, ((int) (myInt)). Maybe, I should do this in a separate PR because it may entail a discussion.

[algomaster99]

I also noticed in edea720 (#4995) that the suffix D is not printed.

[algomaster99]

@I-Al-Istannen @MartinWitt can you drop your reviews? I think we need better tests. Maybe you could help me with that?

[SirYwell]

Minimising parentheses for casts is conservative at the moment, but mainly because it's not trivial to do right.
As an example, consider following line as second statement in your test case:
int myInt2 = ((Double) myDouble).intValue();
With your current changes, this will be printed as int myInt2 = (java.lang.Double) myDouble.intValue() which does not compile anymore.

We can surely think about cases where the parentheses are not required (for example, a direct assignment like your test might be such case), but I'd prefer to still be conservative and only drop the brackets if we are really sure that it will still compile.

[algomaster99]

I am sorry I am seeing this so late. It got lost somewhere in my notifications.

Thanks for this test case. A patch like checking if the default expression is an instance of invocation and then checking if it has a target and executable ref can fix it.

We can surely think about cases where the parentheses are not required

We should. As of now, I do not have any more test cases in mind. I usually rely on clients reporting bugs in this area to improve the printer.

if we are really sure that it will still compile.

That is true. Maybe we can add an assertion in our test cases that compiles the pretty-printed string?

[SirYwell]

We should. As of now, I do not have any more test cases in mind. I usually rely on clients reporting bugs in this area to improve the printer.

The important point is that we go the safe route, so instead of "don't print parentheses unless ..." the code has to be "always print parentheses unless ...". Too many parentheses are okay, code that doesn't compile is not okay.

Maybe we can add an assertion in our test cases that compiles the pretty-printed string?

I think there are some larger tests doing that. Afaik there are some test cases that take the spoon source code, print it and compile it to make sure the printer works. Maybe we can reuse something like that with the minimize brackets setting.

[algomaster99]

The important point is that we go the safe route, so instead of "don't print parentheses unless ..." the code has to be "always print parentheses unless ...". Too many parentheses are okay, code that doesn't compile is not okay.

This is what is happening right now, don't you think? Initially, we used to print redundant brackets, but then clients requested minimisation so we started adding patches like #3823, #4809, and a few more to adhere to specific cases.

Maybe we can reuse something like that with the minimize brackets setting.

Yes, I plan to do that. Could you point to some test cases which do that? I could use them as an example?

[algomaster99]

@SirYwell I tried to compile the pretty-printed string in 24385dc (#4995). I have an intuition there may already be a utility for this so let me know if you are aware. Also, maybe we can add the afterEach method in DJPPTest and SJPPTest that checks for compilation afterward in a separate PR?

I feel the changes in 4c68134 (#4995) belong to RoundBracketAnalyzer#requiresRoundBrackets. However, in its current state, it only checks for unary and binary expression so it would take some refactoring to make it generic.

[algomaster99]

There seems to be some flakiness in the CI. Could anyone please run the workflow again?

[I-Al-Istannen]

You should be able to just create a new Launcher and build the printed model. Set Environment#shouldCompile to true. If buildModel exists cleanly, it compiled.

The failing workflow was https://github.com/INRIA/spoon/actions/runs/3438229451/jobs/5734018883, I will need to have a look at that later. A flaky CI is really annoying.

[algomaster99]

The failing workflow was INRIA/spoon/actions/runs/3438229451/jobs/5734018883, I will need to have a look at that later. A flaky CI is really annoying.

This is not flaky. I think I introduced some regression. Let me check again.

[SirYwell]

This is what is happening right now, don't you think? Initially, we used to print redundant brackets, but then clients requested minimisation so we started adding patches like #3823, #4809, and a few more to adhere to specific cases.

This is true for the linked PRs, but with the current state of this PR, we say should set bracket for a specific case (we don't know if we cover all other cases) instead of `should not set bracket´ for a specific case (we know that it is save for this case).

The isTargetOfInvocation check is also somewhat duplicated below (https://github.com/INRIA/spoon/pull/4995/files#diff-69f595720b242e1a1322a888005148a4400e96e04f3fe5a2e898e44e6471ebf4R423-R425) but with more special casing. That's also the source of the current test failures, you're setting brackets now always if minimizeRoundBrackets is enabled.

I have an intuition there may already be a utility for this so let me know if you are aware.

I don't have anything in my mind right now, I would need to skim through existing tests first.

Also, maybe we can add the afterEach method in DJPPTest and SJPPTest that checks for compilation afterward in a separate PR?

I'm not sure if that's doable, the tests are likely not consistent enough and if it does fail during the test, tooling might break.

I feel the changes in 4c68134 (#4995) belong to RoundBracketAnalyzer#requiresRoundBrackets. However, in its current state, it only checks for unary and binary expression so it would take some refactoring to make it generic.

Yes, that sounds like a good place for anything related to minimizeRoundBrackets. Generally, I think that should touch the DJPP as little as possible.

[algomaster99]

The isTargetOfInvocation check is also somewhat duplicated below (#4995 (files)) but with more special casing. That's also the source of the current test failures, you're setting brackets now always if minimizeRoundBrackets is enabled.

Indeed that was the source. Thank you! I fixed it in recent commits.

[algomaster99]

@SirYwell it is ready for review now.

[SirYwell]

This additional check can still cause issues, because now we can go through the whole method and return false as a default value, even if there are type casts. It might work by returning !e.getTypeCasts().isEmpty() at the bottom,

Consider following silly example:

class A {
    A a;
    int i;
}
class C extends A {
    Object a;
}

(the subclass shadows a field of its superclass)
and following statement:

C c = null;
int i = ((A) c).a.i;

This currently does not enter the if body but instead return false at the end of the method. Obviously, (A) c.a.i does not compile.

Even worse, we can construct code that still compiles but has different semantics by adjusting the classes as follows:

class A {
    A a;
    int i;
}
class C extends A {
    C a;
		A i;
}

and the statements:

C c = null;
var i = ((A) c).a.i;

[algomaster99]

It might work by returning !e.getTypeCasts().isEmpty()

I think we should return false by default as it aligns with your statement - "always print parentheses unless ...".

int i = ((A) c).a.i;

For this case, we need to enclose c which is VariableRead so if we just add a case for it, the test would pass.

However, I think it is becoming very confusing to read through shouldSetBracket. We should really consider refactoring it inside RoundBracketAnalyzer. Basically, refactor

spoon/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java

Lines 414 to 419 in 8e23bae

	if ((e.getParent() instanceof CtBinaryOperator) || (e.getParent() instanceof CtUnaryOperator)) {
	return (e instanceof CtAssignment) || (e instanceof CtConditional) || (e instanceof CtUnaryOperator) || e instanceof CtBinaryOperator;
	}
	if (e.getParent() instanceof CtTargetedExpression && ((CtTargetedExpression) e.getParent()).getTarget() == e) {
	return (e instanceof CtBinaryOperator) || (e instanceof CtAssignment) || (e instanceof CtConditional) || (e instanceof CtUnaryOperator);
	}

and possibly minimise the conditions. In theory, the exact if-conditions should be built upon these three conditions:

1. We need to put round brackets wherever we type cast before invoking some executable on it.
2. We should not put round brackets if we type cast, but we do not invoke anything on it.
3. By default, we always set round brackets.

[SirYwell]

I think we should return false by default as it aligns with your statement - "always print parentheses unless ...".

From my understanding, the method should return true whenever the printer should print parentheses.

However, I think it is becoming very confusing to read through shouldSetBracket. [...]

I think we're at a stage where we need to refactor something, but I'm not sure about the where and the how.
One problem is that this method currently does two things:

1. It avoids the obvious cases where no parentheses are needed, e.g., System.out.println("Hello World") instead of (System.out).println("Hello World")
2. It conditionally (depending on the minimizeRoundBrackets) avoids additional, not as obvious cases in which parentheses can be omitted. Before this PR, this only happened in the RoundBracketAnalyzer, and never if casts are present.

With the new additional check at the very top of the method, we do not longer return true unconditionally if casts are present, but in worst case we fall back to returning false. This heavily changes how the method behaves in a lot of cases which were only covered implicitly (i.e. "we always print parentheses if casts are present").

One idea would be to modify the first if as follows:

if (isMinimizeRoundBrackets()) {
	// Do RoundBracketAnalyzer stuff here, make RoundBracketAnalyzer work with type casts too
} else if (!e.getTypeCasts().isEmpty()) {
	return true; // keep behavior of non-minimizing printer
}
// drop RoundBracketAnalyzer stuff below, keep everything else as before this PR

This way, we can make sure the behavior of non-minimizing printing just remains the same.
WDYT?

[algomaster99]

From my understanding, the method should return true whenever the printer should print parentheses.

Oh right. I meant true. I am sorry for the confusion.

I was trying to refactor similarly before I typed #4995 (comment). I made a lot of tests fail hence I delegated it to later PRs. Maybe I can have a look again :)

[algomaster99]

@SirYwell do you think the refactoring makes it more readable?

[SirYwell]

Yes! This definitely looks better. I'm still somewhat concerned as the RoundBracketAnalyzer is now called in cases with casts present. This wasn't the case before, so any behavior of the RoundBracketAnalyzer probably isn't properly tested for such cases. The trivial fix for now would be to also have an addtional isEmpty check in the return statement here https://github.com/INRIA/spoon/pull/4995/files#diff-69f595720b242e1a1322a888005148a4400e96e04f3fe5a2e898e44e6471ebf4R409 too.

[algomaster99]

@SirYwell can you check now?

[SirYwell]

This is good from my side now, but someone else should probably take a look too (@I-Al-Istannen maybe?).

[I-Al-Istannen]

Hey,

I had a look at it with Hannes and we came up with a few more failing cases. In particular we also need brackets around the target of casts sometimes: (int) (1 + 2). We hope our list is conservative.

Additionally we fixed

return requiresBrackets == RoundBracketAnalyzer.EncloseInRoundBrackets.YES && e.getTypeCasts().isEmpty();

to ensure it behaves the same as before and doesn't omit casts for some of the new tests we added.

Good luck 🐞

A patch

From c2a903a046d4781015a5f523bc250fbabbd8e619 Mon Sep 17 00:00:00 2001
From: I-Al-Istannen <i-al-istannen@users.noreply.github.com>
Date: Wed, 16 Nov 2022 18:45:34 +0100
Subject: [PATCH] Some comments

Co-authored-by: Hannes Greule <SirYwell@users.noreply.github.com>
---
 .../visitor/DefaultJavaPrettyPrinter.java     | 32 +++++++++++++++----
 .../visitor/DefaultJavaPrettyPrinterTest.java |  4 ++-
 src/test/java/spoon/test/eval/EvalTest.java   |  2 +-
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java
index 9a8b4a935..ede37c500 100644
--- a/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java
+++ b/src/main/java/spoon/reflect/visitor/DefaultJavaPrettyPrinter.java
@@ -68,6 +68,7 @@ import spoon.reflect.code.CtTryWithResource;
 import spoon.reflect.code.CtTypeAccess;
 import spoon.reflect.code.CtTypePattern;
 import spoon.reflect.code.CtUnaryOperator;
+import spoon.reflect.code.CtVariableAccess;
 import spoon.reflect.code.CtVariableRead;
 import spoon.reflect.code.CtVariableWrite;
 import spoon.reflect.code.CtWhile;
@@ -296,7 +297,7 @@ public class DefaultJavaPrettyPrinter implements CtVisitor, PrettyPrinter {
 			elementPrinterHelper.writeComment(e, CommentOffset.BEFORE);
 		}
 		getPrinterHelper().mapLine(e, sourceCompilationUnit);
-		if (shouldSetBracket(e)) {
+		if (shouldSetBracketAroundExpressionAndCast(e)) {
 			context.parenthesedExpression.push(e);
 			printer.writeSeparator("(");
 		}
@@ -305,12 +306,29 @@ public class DefaultJavaPrettyPrinter implements CtVisitor, PrettyPrinter {
 				printer.writeSeparator("(");
 				scan(r);
 				printer.writeSeparator(")").writeSpace();
-				if (!isMinimizeRoundBrackets()) {
-					printer.writeSeparator("(");
-					context.parenthesedExpression.push(e);
-				}
 			}
+			if (shouldSetBracketAroundCastTarget(e)) {
+				printer.writeSeparator("(");
+				context.parenthesedExpression.push(e);
+			}
+		}
+	}
+
+	private boolean shouldSetBracketAroundCastTarget(CtExpression<?> expr) {
+		if (!isMinimizeRoundBrackets()) {
+			return true;
 		}
+
+		if (expr instanceof CtTargetedExpression) {
+			return false;
+		}
+		if (expr instanceof CtLiteral) {
+			return false;
+		}
+		if (expr instanceof CtVariableAccess) {
+			return false;
+		}
+		return true;
 	}
 
 	/**
@@ -401,12 +419,12 @@ public class DefaultJavaPrettyPrinter implements CtVisitor, PrettyPrinter {
 		return this;
 	}
 
-	private boolean shouldSetBracket(CtExpression<?> e) {
+	private boolean shouldSetBracketAroundExpressionAndCast(CtExpression<?> e) {
 		if (isMinimizeRoundBrackets()) {
 			RoundBracketAnalyzer.EncloseInRoundBrackets requiresBrackets =
 					RoundBracketAnalyzer.requiresRoundBrackets(e);
 			if (requiresBrackets != RoundBracketAnalyzer.EncloseInRoundBrackets.UNKNOWN) {
-				return requiresBrackets == RoundBracketAnalyzer.EncloseInRoundBrackets.YES && e.getTypeCasts().isEmpty();
+				return requiresBrackets == RoundBracketAnalyzer.EncloseInRoundBrackets.YES || !e.getTypeCasts().isEmpty();
 			}
 			if (e.isParentInitialized() && e.getParent() instanceof CtTargetedExpression && ((CtTargetedExpression) e.getParent()).getTarget() == e) {
 				return e instanceof CtVariableRead<?> && !e.getTypeCasts().isEmpty();
diff --git a/src/test/java/spoon/reflect/visitor/DefaultJavaPrettyPrinterTest.java b/src/test/java/spoon/reflect/visitor/DefaultJavaPrettyPrinterTest.java
index 6546fa2e7..84ff94fe4 100644
--- a/src/test/java/spoon/reflect/visitor/DefaultJavaPrettyPrinterTest.java
+++ b/src/test/java/spoon/reflect/visitor/DefaultJavaPrettyPrinterTest.java
@@ -67,7 +67,9 @@ public class DefaultJavaPrettyPrinterTest {
             "1 | 2 & 3",
             "(1 | 2) & 3",
             "1 | 2 ^ 3",
-            "(1 | 2) ^ 3"
+            "(1 | 2) ^ 3",
+            "((int) (1 + 2)) * 3",
+            "(int) (int) (1 + 1)",
     })
     public void testParenOptimizationCorrectlyPrintsParenthesesForExpressions(String rawExpression) {
         // contract: When input expressions are minimally parenthesized, pretty-printed output
diff --git a/src/test/java/spoon/test/eval/EvalTest.java b/src/test/java/spoon/test/eval/EvalTest.java
index 8a565bde2..c6b55b009 100644
--- a/src/test/java/spoon/test/eval/EvalTest.java
+++ b/src/test/java/spoon/test/eval/EvalTest.java
@@ -96,7 +96,7 @@ public class EvalTest {
 		CtBlock<?> b = type.getMethodsByName("testDoNotSimplifyCasts").get(0).getBody();
 		assertEquals(1, b.getStatements().size());
 		b = b.partiallyEvaluate();
-		assertEquals("return ((U) ((java.lang.Object) (spoon.test.eval.testclasses.ToEvaluate.castTarget(element).getClass())))", b.getStatements().get(0).toString());
+		assertEquals("return ((U) (java.lang.Object) (spoon.test.eval.testclasses.ToEvaluate.castTarget(element).getClass()))", b.getStatements().get(0).toString());
 	}
 
 	@Test
-- 
2.38.1

[algomaster99]

return requiresBrackets == RoundBracketAnalyzer.EncloseInRoundBrackets.YES || !e.getTypeCasts().isEmpty();

It should have been this way to preserve the old behaviour. Thank you for noticing. The commit looks good to me and I have pushed it.

[MartinWitt]

@algomaster99 we had some changes to Qodana on the master branch. Could you rebase/merge with the master to run the checks with the current Qodana version?

[algomaster99]

Done :)

…

On Mon, Nov 21, 2022 at 8:59 PM MartinWitt ***@***.***> wrote: @algomaster99 <https://github.com/algomaster99> we had some changes to Qodana on the master branch. Could you rebase/merge with the master to run the checks with the current Qodana version? — Reply to this email directly, view it on GitHub <#4995 (comment)>, or unsubscribe . You are receiving this because you were mentioned.Message ID: ***@***.***>

[MartinWitt]

Thanks @algomaster99