feat: print minimal amount of round brackets in sniper mode (INRIA#3823)

spoon-pom/pom.xml

@@ -54,7 +54,13 @@
  		<version>5.7.1</version>
  		<scope>test</scope>
 	</dependency>
-
+    <dependency>
+        <groupId>org.junit.jupiter</groupId>
+        <artifactId>junit-jupiter-params</artifactId>
+        <version>5.7.1</version>
+        <scope>test</scope>
+    </dependency>
+
   <dependency>
     <!-- must come after junit5 deps, otherwise it overrides the junit version -->
     <!-- must be here in the parent POM, because the parent always comes last, see https://stackoverflow.com/questions/28999057/order-of-inherited-dependencies-in-maven-3 -->

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

@@ -217,6 +217,12 @@ public class DefaultJavaPrettyPrinter implements CtVisitor, PrettyPrinter {
 	 */
 	protected boolean ignoreImplicit = true;
 
+	/**
+	 * EXPERIMENTAL: If true, the printer will attempt to print a minimal set of round brackets in
+	 * expressions while preserving the syntactical structure of the AST.
+	 */
+	private boolean minimizeRoundBrackets = false;
+
 	public boolean inlineElseIf = true;
 
 	/**
@@ -391,6 +397,13 @@ private boolean shouldSetBracket(CtExpression<?> e) {
 			return true;
 		}
 		try {
+			if (isMinimizeRoundBrackets()) {
+				RoundBracketAnalyzer.EncloseInRoundBrackets requiresBrackets =
+						RoundBracketAnalyzer.requiresRoundBrackets(e);
+				if (requiresBrackets != RoundBracketAnalyzer.EncloseInRoundBrackets.UNKNOWN) {
+					return requiresBrackets == RoundBracketAnalyzer.EncloseInRoundBrackets.YES;
+				}
+			}
 			if ((e.getParent() instanceof CtBinaryOperator) || (e.getParent() instanceof CtUnaryOperator)) {
 				return (e instanceof CtAssignment) || (e instanceof CtConditional) || (e instanceof CtUnaryOperator) || e instanceof CtBinaryOperator;
 			}
@@ -2130,4 +2143,29 @@ public void visitCtYieldStatement(CtYieldStatement statement) {
 		scan(statement.getExpression());
 		exitCtStatement(statement);
 	}
+
+	/**
+	 * @return true if the printer is minimizing the amount of round brackets in expressions
+	 */
+	protected boolean isMinimizeRoundBrackets() {
+		return minimizeRoundBrackets;
+	}
+
+	/**
+	 * When set to true, this activates round bracket minimization for expressions. This means that
+	 * the printer will attempt to only write round brackets strictly necessary for preserving
+	 * syntactical structure (and by extension, semantics).
+     *
+	 * As an example, the expression <code>1 + 2 + 3 + 4</code> is written as
+	 * <code>((1 + 2) + 3) + 4</code> without round bracket minimization, but entirely without
+	 * parentheses when minimization is enabled. However, an expression <code>1 + 2 + (3 + 4)</code>
+	 * is still written as <code>1 + 2 + (3 + 4)</code> to preserve syntactical structure, even though
+	 * the brackets are semantically redundant.
+	 *
+	 * @param minimizeRoundBrackets set whether or not to minimize round brackets in expressions
+	 */
+	protected void setMinimizeRoundBrackets(boolean minimizeRoundBrackets) {
+		this.minimizeRoundBrackets = minimizeRoundBrackets;
+	}
+
 }

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

@@ -16,6 +16,10 @@
  */
 class OperatorHelper {
 
+	public enum OperatorAssociativity {
+		LEFT, RIGHT, NONE
+	}
+
 	private OperatorHelper() {
 	}
 
@@ -101,4 +105,118 @@ public static String getOperatorText(BinaryOperatorKind o) {
 				throw new SpoonException("Unsupported operator " + o.name());
 		}
 	}
+
+	/**
+	 * Get the precedence of a binary operator as defined by
+	 * https://introcs.cs.princeton.edu/java/11precedence/
+	 *
+	 * @param o A binary operator kind.
+	 * @return The precedence of the given operator.
+	 */
+	public static int getOperatorPrecedence(BinaryOperatorKind o) {
+		switch (o) {
+			case OR: // ||
+				return 3;
+			case AND: // &&
+				return 4;
+			case BITOR: // |
+				return 5;
+			case BITXOR: // ^
+				return 6;
+			case BITAND: // &
+				return 7;
+			case EQ: // ==
+			case NE: // !=
+				return 8;
+			case LT: // <
+			case GT: // >
+			case LE: // <=
+			case GE: // >=
+			case INSTANCEOF:
+				return 9;
+			case SL: // <<
+			case SR: // >>
+			case USR: // >>>
+				return 10;
+			case PLUS: // +
+			case MINUS: // -
+				return 11;
+			case MUL: // *
+			case DIV: // /
+			case MOD: // %
+				return 12;
+			default:
+				throw new SpoonException("Unsupported operator " + o.name());
+		}
+	}
+
+	/**
+	 * Get the precedence of a unary operator as defined by
+	 * https://introcs.cs.princeton.edu/java/11precedence/
+	 *
+	 * @param o A unary operator kind.
+	 * @return The precedence of the given operator.
+	 */
+	public static int getOperatorPrecedence(UnaryOperatorKind o) {
+		switch (o) {
+			case POS:
+			case NEG:
+			case NOT:
+			case COMPL:
+			case PREINC:
+			case PREDEC:
+				return 14;
+			case POSTINC:
+			case POSTDEC:
+				return 15;
+			default:
+				throw new SpoonException("Unsupported operator " + o.name());
+		}
+	}
+
+	/**
+	 * Get the associativity of a binary operator as defined by
+	 * https://introcs.cs.princeton.edu/java/11precedence/
+	 *
+	 * All binary operators are left-associative in Java, except for the relational operators that
+	 * have no associativity (i.e. you can't chain them).
+	 *
+	 * There's an exception: the ternary operator ?: is right-associative, but that's not an
+	 * operator kind in Spoon so we don't deal with it.
+	 *
+	 * @param o A binary operator kind.
+	 * @return The associativity of the operator.
+	 */
+	public static OperatorAssociativity getOperatorAssociativity(BinaryOperatorKind o) {
+		switch (o) {
+			case LT: // <
+			case GT: // >
+			case LE: // <=
+			case GE: // >=
+			case INSTANCEOF:
+				return OperatorAssociativity.NONE;
+			default:
+				return OperatorAssociativity.LEFT;
+		}
+	}
+
+	/**
+	 * Get the associativity of a unary operator, as defined by
+	 * https://introcs.cs.princeton.edu/java/11precedence/
+	 *
+	 * All unary operators are right-associative, except for post increment and decrement, which
+	 * are not associative.
+	 *
+	 * @param o A unary operator kind.
+	 * @return The associativity of the operator.
+	 */
+	public static OperatorAssociativity getOperatorAssociativity(UnaryOperatorKind o) {
+		switch (o) {
+			case POSTINC:
+			case POSTDEC:
+				return OperatorAssociativity.NONE;
+			default:
+				return OperatorAssociativity.RIGHT;
+		}
+	}
 }

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

@@ -0,0 +1,114 @@
+/**
+ * SPDX-License-Identifier: (MIT OR CECILL-C)
+ *
+ * Copyright (C) 2006-2019 INRIA and contributors
+ *
+ * Spoon is available either under the terms of the MIT License (see LICENSE-MIT.txt) of the Cecill-C License (see LICENSE-CECILL-C.txt). You as the user are entitled to choose the terms under which to adopt Spoon.
+ */
+package spoon.reflect.visitor;
+
+import spoon.reflect.code.CtBinaryOperator;
+import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtUnaryOperator;
+import spoon.reflect.declaration.CtElement;
+
+/**
+ * Class for determining whether or not an expression requires round brackets in order to preserve
+ * AST structure (and consequently semantics).
+ */
+class RoundBracketAnalyzer {
+
+	enum EncloseInRoundBrackets {
+		YES, NO, UNKNOWN;
+	}
+
+	private RoundBracketAnalyzer() {
+	}
+
+	/**
+	 * @param expr A unary or binary expr.
+	 * @return true if the expr should be enclosed in round brackets.
+	 */
+	static EncloseInRoundBrackets requiresRoundBrackets(CtExpression<?> expr) {
+		return isNestedOperator(expr)
+				? nestedOperatorRequiresRoundBrackets(expr)
+				: EncloseInRoundBrackets.UNKNOWN;
+	}
+
+	/**
+	 * Assuming that operator is a nested operator (i.e. both operator and its parent are
+	 * {@link CtUnaryOperator} or {@link CtBinaryOperator}), determine whether or not it must be
+	 * enclosed in round brackets.
+	 *
+	 * Given an element <code>e</code> with a parent <code>p</code>, we must parenthesize
+	 * <code>e</code> if any of the following are true.
+	 *
+	 * <ul>
+	 *     <li>The parent p is a unary operator</li>
+	 *     <li>The parent p is a binary operator, and <code>precedence(p) > precedence(e></code></li>
+	 *     <li>The parent p is a binary operator, <code>precedence(p) == precedence(e)</code>,
+	 *     e appears as the X-hand-side operand of p, and e's operator is Y-associative, where
+	 *     <code>X != Y</code></li>
+	 * </ul>
+	 *
+	 * Note that the final rule is necessary to preserve syntactical structure, but it is not
+	 * required for preserving semantics.
+	 *
+	 * @param nestedOperator A nested operator.
+	 * @return Whether or not to enclose the nested operator in round brackets.
+	 */
+	private static EncloseInRoundBrackets nestedOperatorRequiresRoundBrackets(CtExpression<?> nestedOperator) {
+		if (nestedOperator.getParent() instanceof CtUnaryOperator) {
+			return EncloseInRoundBrackets.YES;
+		}
+
+		OperatorHelper.OperatorAssociativity associativity = getOperatorAssociativity(nestedOperator);
+		OperatorHelper.OperatorAssociativity positionInParent = getPositionInParent(nestedOperator);
+
+		int parentPrecedence = getOperatorPrecedence(nestedOperator.getParent());
+		int precedence = getOperatorPrecedence(nestedOperator);
+		return precedence < parentPrecedence
+				|| (precedence == parentPrecedence && associativity != positionInParent)
+				? EncloseInRoundBrackets.YES
+				: EncloseInRoundBrackets.NO;
+	}
+
+	private static boolean isNestedOperator(CtElement e) {
+		return e.isParentInitialized() && isOperator(e) && isOperator(e.getParent());
+	}
+
+	private static boolean isOperator(CtElement e) {
+		return e instanceof CtBinaryOperator || e instanceof CtUnaryOperator;
+	}
+
+	private static int getOperatorPrecedence(CtElement e) {
+		if (e instanceof CtBinaryOperator) {
+			return OperatorHelper.getOperatorPrecedence(((CtBinaryOperator<?>) e).getKind());
+		} else if (e instanceof CtUnaryOperator) {
+			return OperatorHelper.getOperatorPrecedence(((CtUnaryOperator<?>) e).getKind());
+		} else {
+			return 0;
+		}
+	}
+
+	private static OperatorHelper.OperatorAssociativity getOperatorAssociativity(CtElement e) {
+		if (e instanceof CtBinaryOperator) {
+			return OperatorHelper.getOperatorAssociativity(((CtBinaryOperator<?>) e).getKind());
+		} else if (e instanceof CtUnaryOperator) {
+			return OperatorHelper.getOperatorAssociativity(((CtUnaryOperator<?>) e).getKind());
+		} else {
+			return OperatorHelper.OperatorAssociativity.NONE;
+		}
+	}
+
+	private static OperatorHelper.OperatorAssociativity getPositionInParent(CtElement e) {
+		CtElement parent = e.getParent();
+		if (parent instanceof CtBinaryOperator) {
+			return ((CtBinaryOperator<?>) parent).getLeftHandOperand() == e
+					? OperatorHelper.OperatorAssociativity.LEFT
+					: OperatorHelper.OperatorAssociativity.RIGHT;
+		} else {
+			return OperatorHelper.OperatorAssociativity.NONE;
+		}
+	}
+}

src/main/java/spoon/support/sniper/SniperJavaPrettyPrinter.java

@@ -82,6 +82,9 @@ public SniperJavaPrettyPrinter(Environment env) {
 
 		// newly added elements are not fully qualified
 		this.setIgnoreImplicit(false);
+
+		// don't print redundant parentheses
+		this.setMinimizeRoundBrackets(true);
 	}
 
 	/**

src/test/java/spoon/reflect/visitor/DefaultJavaPrettyPrinterTest.java

@@ -0,0 +1,63 @@
+package spoon.reflect.visitor;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import spoon.Launcher;
+import spoon.reflect.code.CtExpression;
+import spoon.reflect.code.CtStatement;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+public class DefaultJavaPrettyPrinterTest {
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "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"
+    })
+    public void testParenOptimizationCorrectlyPrintsParenthesesForExpressions(String rawExpression) {
+        // contract: When input expressions are minimally parenthesized, pretty-printed output
+        // should match the input
+        CtExpression<?> expr = createLauncherWithOptimizeParenthesesPrinter()
+                .getFactory().createCodeSnippetExpression(rawExpression).compile();
+        assertThat(expr.toString(), equalTo(rawExpression));
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {
+            "int sum = 1 + 2 + 3",
+            "java.lang.String s = \"Sum: \" + (1 + 2)",
+            "java.lang.String s = \"Sum: \" + 1 + 2"
+    })
+    public void testParenOptimizationCorrectlyPrintsParenthesesForStatements(String rawStatement) {
+        // contract: When input expressions as part of statements are minimally parenthesized,
+        // pretty-printed output should match the input
+        CtStatement statement = createLauncherWithOptimizeParenthesesPrinter()
+                .getFactory().createCodeSnippetStatement(rawStatement).compile();
+        assertThat(statement.toString(), equalTo(rawStatement));
+    }
+
+    private static Launcher createLauncherWithOptimizeParenthesesPrinter() {
+        Launcher launcher = new Launcher();
+        launcher.getEnvironment().setPrettyPrinterCreator(() -> {
+            DefaultJavaPrettyPrinter printer = new DefaultJavaPrettyPrinter(launcher.getEnvironment());
+            printer.setMinimizeRoundBrackets(true);
+            return printer;
+        });
+        return launcher;
+    }
+}