diff --git a/CHANGELOG.md b/CHANGELOG.md index 3594934f9..c76214a07 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Support for MSBuild item and item metadata expressions in project files. - `ExhaustiveEnumCase` analysis rule, which flags `case` statements that do not handle all values in an enumeration. - `IterationPastHighBound` analysis rule, which flags `for` loops that iterate past the end of the collection. +- `ExplicitBitwiseNot` analysis rule, which flags potentially incorrect bitwise `not` operations. - **API:** `EnumeratorOccurrence` type. - **API:** `ForInStatementNode::getEnumeratorOccurrence` method. - **API:** `TypeOfTypeNode::getTypeReferenceNode` method. diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java index 9e004aa34..07f4e5f41 100644 --- a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/CheckList.java @@ -67,6 +67,7 @@ public final class CheckList { EmptyVisibilitySectionCheck.class, EnumNameCheck.class, ExhaustiveEnumCaseCheck.class, + ExplicitBitwiseNotCheck.class, ExplicitDefaultPropertyReferenceCheck.class, ExplicitTObjectInheritanceCheck.class, FieldNameCheck.class, diff --git a/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExplicitBitwiseNotCheck.java b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExplicitBitwiseNotCheck.java new file mode 100644 index 000000000..d190ef97e --- /dev/null +++ b/delphi-checks/src/main/java/au/com/integradev/delphi/checks/ExplicitBitwiseNotCheck.java @@ -0,0 +1,67 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2025 Integrated Application Development + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package au.com.integradev.delphi.checks; + +import org.sonar.check.Rule; +import org.sonar.plugins.communitydelphi.api.ast.BinaryExpressionNode; +import org.sonar.plugins.communitydelphi.api.ast.ExpressionNode; +import org.sonar.plugins.communitydelphi.api.ast.UnaryExpressionNode; +import org.sonar.plugins.communitydelphi.api.check.DelphiCheck; +import org.sonar.plugins.communitydelphi.api.check.DelphiCheckContext; +import org.sonar.plugins.communitydelphi.api.check.FilePosition; +import org.sonar.plugins.communitydelphi.api.operator.BinaryOperator; +import org.sonar.plugins.communitydelphi.api.operator.UnaryOperator; +import org.sonar.plugins.communitydelphi.api.reporting.QuickFix; +import org.sonar.plugins.communitydelphi.api.reporting.QuickFixEdit; + +@Rule(key = "ExplicitBitwiseNot") +public class ExplicitBitwiseNotCheck extends DelphiCheck { + @Override + public DelphiCheckContext visit(BinaryExpressionNode node, DelphiCheckContext context) { + if (node.getOperator() == BinaryOperator.IN) { + checkBitwiseNot(node.getLeft(), context); + checkBitwiseNot(node.getRight(), context); + } + + return super.visit(node, context); + } + + private void checkBitwiseNot(ExpressionNode node, DelphiCheckContext context) { + if (!(node instanceof UnaryExpressionNode)) { + return; + } + UnaryExpressionNode unaryNode = (UnaryExpressionNode) node; + + if (isBitwiseNot(unaryNode)) { + context + .newIssue() + .onFilePosition(FilePosition.from(unaryNode.getToken())) + .withMessage("Parenthesize this bitwise 'not' operation.") + .withQuickFixes( + QuickFix.newFix("Parenthesize bitwise 'not'") + .withEdits( + QuickFixEdit.insertBefore("(", node), QuickFixEdit.insertAfter(")", node))) + .report(); + } + } + + private boolean isBitwiseNot(UnaryExpressionNode node) { + return node.getOperator() == UnaryOperator.NOT && node.getType().isInteger(); + } +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExplicitBitwiseNot.html b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExplicitBitwiseNot.html new file mode 100644 index 000000000..850f3482d --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExplicitBitwiseNot.html @@ -0,0 +1,47 @@ +

Why is this an issue?

+

+ The Delphi bitwise not operator binds stronger than the in binary + operator, which can lead to subtly incorrect code. For example, in the code below the bitwise + not has been confused for a logical not, which has introduced a bug: +

+
+var MyByte: Byte := 3;
+
+if not MyByte in [252, 253, 254, 255] then
+  raise Exception.Create('MyByte must not be above 251!'); // This is raised!
+
+

+ To avoid this pitfall, complex expressions involving not and in should + be parenthesized appropriately, so the precedence is obvious at a glance. +

+

How to fix it

+

If the bitwise not is intentional, parenthesize it:

+
+var MyByte: Byte := 3;
+if not MyByte in [252] then
+  WriteLn('error: MyByte must not be 252!');
+
+
+var MyByte: Byte := 3;
+if (not MyByte) in [252] then
+  WriteLn('error: MyByte must not be 3!');
+
+

Otherwise, parenthesize the binary expression that should be negated:

+
+var MyByte: Byte := 3;
+if not MyByte in [252] then
+  WriteLn('error: MyByte must not be 252!');
+
+
+var MyByte: Byte := 3;
+if not (MyByte in [252]) then
+  WriteLn('error: MyByte must be 252!');
+
+

Resources

+ \ No newline at end of file diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExplicitBitwiseNot.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExplicitBitwiseNot.json new file mode 100644 index 000000000..e68be0a5f --- /dev/null +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/ExplicitBitwiseNot.json @@ -0,0 +1,19 @@ +{ + "title": "Ambiguous bitwise 'not' operations should be parenthesized", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant/Issue", + "constantCost": "2min" + }, + "code": { + "attribute": "LOGICAL", + "impacts": { + "RELIABILITY": "MEDIUM" + } + }, + "tags": ["suspicious"], + "defaultSeverity": "Minor", + "scope": "ALL", + "quickfix": "unknown" +} diff --git a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json index d3e6aacf7..7450460ef 100644 --- a/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json +++ b/delphi-checks/src/main/resources/org/sonar/l10n/delphi/rules/community-delphi/Sonar_way_profile.json @@ -33,6 +33,7 @@ "EmptyRoutine", "EmptyVisibilitySection", "EnumName", + "ExplicitBitwiseNot", "ExplicitDefaultPropertyReference", "FieldName", "FormatArgumentCount", diff --git a/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExplicitBitwiseNotCheckTest.java b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExplicitBitwiseNotCheckTest.java new file mode 100644 index 000000000..14c1c9c57 --- /dev/null +++ b/delphi-checks/src/test/java/au/com/integradev/delphi/checks/ExplicitBitwiseNotCheckTest.java @@ -0,0 +1,171 @@ +/* + * Sonar Delphi Plugin + * Copyright (C) 2019 Integrated Application Development + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02 + */ +package au.com.integradev.delphi.checks; + +import au.com.integradev.delphi.builders.DelphiTestUnitBuilder; +import au.com.integradev.delphi.checks.verifier.CheckVerifier; +import org.junit.jupiter.api.Test; + +class ExplicitBitwiseNotCheckTest { + @Test + void testSimpleBitwiseNotShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not 1;") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testOtherIntegerUnaryOperatorsShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := -1 in [2];") + .appendImpl(" Result := +1 in [2];") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testBitwiseNotInSetContainmentCheckShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("procedure Test;") + .appendImpl("begin") + .appendImpl(" // Fix@[+2:5 to +2:5] <<(>>") + .appendImpl(" // Fix@[+1:10 to +1:10] <<)>>") + .appendImpl(" if not 3 in [3, 252] then // Noncompliant") + .appendImpl(" Writeln('Foo');") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testBitwiseNotInBinaryExpressionShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not 1 in [2]; // Noncompliant") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testParenthesizedBitwiseNotInBinaryExpressionShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := (not 1) in [2];") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testBooleanNotShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not (1 in [2]);") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testUnresolvedNotShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not UnresolvedIdentifier in [2];") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testIntegerProceduralTypeShouldAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function GetMyInt: Integer;") + .appendImpl("begin") + .appendImpl(" Result := 1;") + .appendImpl("end;") + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not GetMyInt in [2]; // Noncompliant") + .appendImpl("end;")) + .verifyIssues(); + } + + @Test + void testNonIntegerProceduralTypeShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function GetMyStr: string;") + .appendImpl("begin") + .appendImpl(" Result := 'Hello world';") + .appendImpl("end;") + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not GetMyStr in [2];") + .appendImpl("end;")) + .verifyNoIssues(); + } + + @Test + void testParenthesizedIntegerProceduralTypeShouldNotAddIssue() { + CheckVerifier.newVerifier() + .withCheck(new ExplicitBitwiseNotCheck()) + .onFile( + new DelphiTestUnitBuilder() + .appendImpl("function GetMyInt: Integer;") + .appendImpl("begin") + .appendImpl(" Result := 1;") + .appendImpl("end;") + .appendImpl("function Test: Integer;") + .appendImpl("begin") + .appendImpl(" Result := not (GetMyInt in [2]);") + .appendImpl("end;")) + .verifyNoIssues(); + } +}