Target parenthesized primary expressions in RedundantParentheses

Author: cirras

CHANGELOG.md

@@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Improve type inference around unsigned integer literals.
 - Improve type inference around array constructors containing integer literals.
 - Improve type inference around real expressions.
+- Parentheses enclosing a primary expression are considered redundant in `RedundantParentheses`.
 - `out` parameters are treated as uninitialized at the start of a routine in
   `VariableInitialization`.
 

delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantParenthesesCheck.java

@@ -20,6 +20,7 @@
 
 import org.sonar.check.Rule;
 import org.sonar.plugins.communitydelphi.api.ast.ParenthesizedExpressionNode;
+import org.sonar.plugins.communitydelphi.api.ast.PrimaryExpressionNode;
 import org.sonar.plugins.communitydelphi.api.check.DelphiCheck;
 import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext;
 import org.sonar.plugins.communitydelphi.api.reporting.QuickFix;
@@ -34,7 +35,8 @@ public class RedundantParenthesesCheck extends DelphiCheck {
   @Override
   public DelphiCheckContext visit(
       ParenthesizedExpressionNode expression, DelphiCheckContext context) {
-    if (expression.getParent() instanceof ParenthesizedExpressionNode) {
+    if (expression.getExpression() instanceof ParenthesizedExpressionNode
+        || expression.getExpression() instanceof PrimaryExpressionNode) {
       context
           .newIssue()
           .onNode(expression.getChild(0))

delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/RedundantParentheses.html

@@ -1,8 +1,14 @@
 <h2>Why is this an issue?</h2>
 <p>
   Parentheses should be used to clarify the intent behind a piece of code or enforce a desired
-  order of operations. Redundant pairs of parentheses do neither of these things, making code more
-  confusing and less readable.
+  order of operations. Redundant parentheses do neither of these things, making code more confusing
+  and less readable.
+</p>
+<p>
+  This rule will suggest removing parentheses that wrap another parenthesized expression, or a
+  primary expression (e.g. <code>'foo'</code> or <code>Bar.Baz(123)</code>). In both cases,
+  parenthesizing the expression is syntactically meaningless without improving readability for a
+  human.
 </p>
 <h2>How to fix it</h2>
 <p>Remove the redundant parentheses:</p>

delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantParenthesesCheckTest.java

@@ -24,7 +24,7 @@
 
 class RedundantParenthesesCheckTest {
   @Test
-  void testNoParenthesesShouldNotAddIssue() {
+  void testPrimaryExpressionShouldNotAddIssue() {
     CheckVerifier.newVerifier()
         .withCheck(new RedundantParenthesesCheck())
         .onFile(
@@ -37,29 +37,87 @@ void testNoParenthesesShouldNotAddIssue() {
   }
 
   @Test
-  void testParenthesesShouldNotAddIssue() {
+  void testBinaryExpressionShouldNotAddIssue() {
     CheckVerifier.newVerifier()
         .withCheck(new RedundantParenthesesCheck())
         .onFile(
             new DelphiTestUnitBuilder()
                 .appendImpl("function GetInteger: Integer;")
                 .appendImpl("begin")
-                .appendImpl("  Result := (123);")
+                .appendImpl("  Result := 1 + 2;")
                 .appendImpl("end;"))
         .verifyNoIssues();
   }
 
   @Test
-  void testRedundantParenthesesShouldAddIssue() {
+  void testUnaryExpressionShouldNotAddIssue() {
     CheckVerifier.newVerifier()
         .withCheck(new RedundantParenthesesCheck())
         .onFile(
             new DelphiTestUnitBuilder()
                 .appendImpl("function GetInteger: Integer;")
                 .appendImpl("begin")
-                .appendImpl("  // Fix@[+2:13 to +2:14] <<>>")
-                .appendImpl("  // Fix@[+1:17 to +1:18] <<>>")
-                .appendImpl("  Result := ((123)); // Noncompliant")
+                .appendImpl("  Result := -123;")
+                .appendImpl("end;"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testParenthesesOnBinaryExpressionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantParenthesesCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("function GetInteger: Integer;")
+                .appendImpl("begin")
+                .appendImpl("  Result := (1 + 2);")
+                .appendImpl("end;"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testParenthesesOnUnaryExpressionShouldNotAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantParenthesesCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("function GetInteger: Integer;")
+                .appendImpl("begin")
+                .appendImpl("  Result := (-123);")
+                .appendImpl("end;"))
+        .verifyNoIssues();
+  }
+
+  @Test
+  void testParenthesesOnParenthesizedExpressionShouldAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantParenthesesCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("function GetInteger: Integer;")
+                .appendImpl("begin")
+                .appendImpl("  // Fix@[+2:12 to +2:13] <<>>")
+                .appendImpl("  // Fix@[+1:20 to +1:21] <<>>")
+                .appendImpl("  Result := ((1 + 2)); // Noncompliant")
+                .appendImpl("end;"))
+        .verifyIssues();
+  }
+
+  @Test
+  void testParenthesesOnPrimaryExpressionShouldAddIssue() {
+    CheckVerifier.newVerifier()
+        .withCheck(new RedundantParenthesesCheck())
+        .onFile(
+            new DelphiTestUnitBuilder()
+                .appendImpl("function GetInteger: Integer;")
+                .appendImpl("begin")
+                .appendImpl("  // Fix qf1@[+6:12 to +6:13] <<>>")
+                .appendImpl("  // Fix qf2@[+5:13 to +5:14] <<>>")
+                .appendImpl("  // Fix qf2@[+4:17 to +4:18] <<>>")
+                .appendImpl("  // Fix qf1@[+3:18 to +3:19] <<>>")
+                .appendImpl("  // Noncompliant@+2")
+                .appendImpl("  // Noncompliant@+1")
+                .appendImpl("  Result := ((123));")
                 .appendImpl("end;"))
         .verifyIssues();
   }