From de3da8717196444b9d528cf2c39958603de96737 Mon Sep 17 00:00:00 2001 From: Jonah Jeleniewski Date: Fri, 30 Aug 2024 12:17:30 +1000 Subject: [PATCH] Target parenthesized primary expressions in `RedundantParentheses` --- CHANGELOG.md | 1 + .../checks/RedundantParenthesesCheck.java | 4 +- .../RedundantParentheses.html | 10 ++- .../checks/RedundantParenthesesCheckTest.java | 72 +++++++++++++++++-- 4 files changed, 77 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8e85a28b..a410bbb7a 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantParenthesesCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantParenthesesCheck.java index 061d57191..4b7c0af54 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/RedundantParenthesesCheck.java +++ b/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)) diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/RedundantParentheses.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/RedundantParentheses.html index ccfb24a33..4645647c0 100644 --- a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/RedundantParentheses.html +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/RedundantParentheses.html @@ -1,8 +1,14 @@

Why is this an issue?

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. +

+

+ This rule will suggest removing parentheses that wrap another parenthesized expression, or a + primary expression (e.g. 'foo' or Bar.Baz(123)). In both cases, + parenthesizing the expression is syntactically meaningless without improving readability for a + human.

How to fix it

Remove the redundant parentheses:

diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantParenthesesCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantParenthesesCheckTest.java index c74e0ce42..6f263f9a3 100644 --- a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/RedundantParenthesesCheckTest.java +++ b/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(); }