diff --git a/its/ruling/src/test/resources/commons-beanutils/java-S9360.json b/its/ruling/src/test/resources/commons-beanutils/java-S9360.json new file mode 100644 index 00000000000..55309c93efd --- /dev/null +++ b/its/ruling/src/test/resources/commons-beanutils/java-S9360.json @@ -0,0 +1,8 @@ +{ +"commons-beanutils:commons-beanutils:src/main/java/org/apache/commons/beanutils2/ConstructorUtils.java": [ +106, +154, +218, +267 +] +} diff --git a/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9360.json b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9360.json new file mode 100644 index 00000000000..c2cf3131395 --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty-similar-to-main/java-S9360.json @@ -0,0 +1,59 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java": [ +343, +358, +373, +388, +403 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java": [ +1028 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java": [ +431, +436, +445, +453, +455, +459, +465, +471, +477, +483, +489, +495, +497, +501, +503, +514, +514, +515, +575, +615, +617, +622, +624, +628, +634, +640, +646, +652, +658, +664, +666, +670, +677, +684 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java": [ +93, +414 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java": [ +94 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java": [ +744, +835 +] +} diff --git a/its/ruling/src/test/resources/eclipse-jetty/java-S9360.json b/its/ruling/src/test/resources/eclipse-jetty/java-S9360.json new file mode 100644 index 00000000000..e4627a8f3bd --- /dev/null +++ b/its/ruling/src/test/resources/eclipse-jetty/java-S9360.json @@ -0,0 +1,75 @@ +{ +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpStatus.java": [ +343, +358, +373, +388, +403 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/HttpURI.java": [ +1028 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/MimeTypes.java": [ +431, +436, +445, +453, +455, +459, +465, +471, +477, +483, +489, +495, +497, +501, +503, +514, +514, +515, +575, +615, +617, +622, +624, +628, +634, +640, +646, +652, +658, +664, +666, +670, +677, +684 +], +"org.eclipse.jetty:jetty-project:jetty-http/src/main/java/org/eclipse/jetty/http/pathmap/ServletPathSpec.java": [ +93, +414 +], +"org.eclipse.jetty:jetty-project:jetty-io/src/test/java/org/eclipse/jetty/io/ArrayByteBufferPoolTest.java": [ +94 +], +"org.eclipse.jetty:jetty-project:jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java": [ +744, +835 +], +"org.eclipse.jetty:jetty-project:jetty-util-ajax/src/main/java/org/eclipse/jetty/util/ajax/AsyncJSON.java": [ +1275 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java": [ +901, +907, +925, +942, +957, +962, +978 +], +"org.eclipse.jetty:jetty-project:jetty-util/src/main/java/org/eclipse/jetty/util/security/Password.java": [ +117, +132 +] +} diff --git a/its/ruling/src/test/resources/guava/java-S9360.json b/its/ruling/src/test/resources/guava/java-S9360.json new file mode 100644 index 00000000000..a0b6b009e4a --- /dev/null +++ b/its/ruling/src/test/resources/guava/java-S9360.json @@ -0,0 +1,13 @@ +{ +"com.google.guava:guava:src/com/google/common/base/SmallCharMatcher.java": [ +61 +], +"com.google.guava:guava:src/com/google/common/io/LittleEndianDataInputStream.java": [ +82 +], +"com.google.guava:guava:src/com/google/common/net/MediaType.java": [ +628, +631, +632 +] +} diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/YodaConditionCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/YodaConditionCheckSample.java new file mode 100644 index 00000000000..6fc2172102d --- /dev/null +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/YodaConditionCheckSample.java @@ -0,0 +1,10 @@ +package checks; + +class YodaConditionCheckSample { + + void unknownLiteralType() { + Object x = new Object(); + if (UNKNOWN_LITERAL == x) { } // Compliant - UNKNOWN_LITERAL is not a valid literal + } + +} diff --git a/java-checks-test-sources/default/src/main/java/checks/YodaConditionCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/YodaConditionCheckSample.java new file mode 100644 index 00000000000..95344739077 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/YodaConditionCheckSample.java @@ -0,0 +1,131 @@ +package checks; + +class YodaConditionCheckSample { + + void testIntLiteral() { + int count = 0; + int x = 5; + if (0 == count) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^ + if (5 != x) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^ + if (count == 0) { } // Compliant + if (x != 5) { } // Compliant + } + + void testNullLiteral() { + Object obj = null; + Object myObject = null; + if (null == obj) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^^ + if (null != myObject) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^^ + if (obj == null) { } // Compliant + if (myObject != null) { } // Compliant + if (null == null) { } // Compliant + } + + void testBooleanLiteral() { + boolean flag = true; + boolean result = false; + if (true == flag) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^^ + if (false != result) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^^^ + if (flag == true) { } // Compliant + if (result != false) { } // Compliant + } + + void testStringLiteral() { + String str = "hello"; + String value = ""; + if ("hello" == str) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^^^^^ + if ("" != value) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^ + if (str == "hello") { } // Compliant + if (value != "") { } // Compliant + } + + void testCharLiteral() { + char ch = 'a'; + if ('a' == ch) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^ + if (ch == 'a') { } // Compliant + } + + void testFloatingPointLiteral() { + double doubleValue = 0.0; + if (0.0 == doubleValue) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^ + if (doubleValue == 0.0) { } // Compliant + } + + void testNestedParentheses() { + int count = 0; + Object obj = null; + if ((0) == count) { } // Noncompliant {{Put the variable on the left side of this comparison.}} + if (((null)) == obj) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^^^^ + } + + void testLessThanGreaterThan() { + int count = 0; + int x = 5; + if (0 < count) { } // Noncompliant {{Put the variable on the left side of this comparison and reverse the operator.}} +// ^ + if (5 > x) { } // Noncompliant {{Put the variable on the left side of this comparison and reverse the operator.}} +// ^ + if (count > 0) { } // Compliant + if (x < 5) { } // Compliant + } + + void testLessThanOrEqualGreaterThanOrEqual() { + int count = 0; + int x = 5; + if (0 <= count) { } // Noncompliant {{Put the variable on the left side of this comparison and reverse the operator.}} +// ^ + if (5 >= x) { } // Noncompliant {{Put the variable on the left side of this comparison and reverse the operator.}} +// ^ + if (count >= 0) { } // Compliant + if (x <= 5) { } // Compliant + } + + void testNonComparisonContexts() { + int count = 0; + int a = 1; + int b = 2; + count = 0; // Compliant - assignment + int sum = a + 5; // Compliant - arithmetic + int product = 5 * b; // Compliant - arithmetic + Object obj = Math.max(5, 10); // Compliant - method call + } + + void testTernaryOperator() { + boolean condition = true; + int result = condition ? 5 : 10; // Compliant + if (condition) { } // Compliant + } + + void testArrayAccess() { + int[] array = {1, 2, 3}; + if (0 == array[0]) { } // Noncompliant {{Put the variable on the left side of this comparison.}} +// ^ + if (array[0] == 0) { } // Compliant + } + + void testBothLiterals() { + if (0 == 0) { } // Compliant - both sides are literals + if (5 != 10) { } // Compliant - both sides are literals + if (true == false) { } // Compliant - both sides are literals + } + + void testBothVariables() { + int count = 0; + int otherCount = 0; + Object obj1 = null; + Object obj2 = null; + if (count == otherCount) { } // Compliant - both are variables + if (obj1 == obj2) { } // Compliant - both are variables + } +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java new file mode 100644 index 00000000000..c35de93d549 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/YodaConditionCheck.java @@ -0,0 +1,77 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * 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 Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import java.util.List; +import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.tree.BinaryExpressionTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S9360") +public class YodaConditionCheck extends IssuableSubscriptionVisitor { + + private static final String MESSAGE = "Put the variable on the left side of this comparison."; + private static final String MESSAGE_WITH_REVERSE = "Put the variable on the left side of this comparison and reverse the operator."; + + @Override + public List nodesToVisit() { + return List.of( + Tree.Kind.EQUAL_TO, + Tree.Kind.NOT_EQUAL_TO, + Tree.Kind.LESS_THAN, + Tree.Kind.GREATER_THAN, + Tree.Kind.LESS_THAN_OR_EQUAL_TO, + Tree.Kind.GREATER_THAN_OR_EQUAL_TO + ); + } + + @Override + public void visitNode(Tree tree) { + BinaryExpressionTree binaryExpression = (BinaryExpressionTree) tree; + ExpressionTree left = ExpressionUtils.skipParentheses(binaryExpression.leftOperand()); + ExpressionTree right = ExpressionUtils.skipParentheses(binaryExpression.rightOperand()); + + if (isLiteral(left) && !isLiteral(right)) { + reportIssue(left, isRelationalOperator(tree) ? MESSAGE_WITH_REVERSE : MESSAGE); + } + } + + private static boolean isRelationalOperator(Tree tree) { + return tree.is( + Tree.Kind.LESS_THAN, + Tree.Kind.GREATER_THAN, + Tree.Kind.LESS_THAN_OR_EQUAL_TO, + Tree.Kind.GREATER_THAN_OR_EQUAL_TO + ); + } + + private static boolean isLiteral(ExpressionTree tree) { + return tree.is( + Tree.Kind.INT_LITERAL, + Tree.Kind.LONG_LITERAL, + Tree.Kind.FLOAT_LITERAL, + Tree.Kind.DOUBLE_LITERAL, + Tree.Kind.BOOLEAN_LITERAL, + Tree.Kind.CHAR_LITERAL, + Tree.Kind.STRING_LITERAL, + Tree.Kind.NULL_LITERAL + ); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/YodaConditionCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/YodaConditionCheckTest.java new file mode 100644 index 00000000000..1ad35320272 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/YodaConditionCheckTest.java @@ -0,0 +1,51 @@ +/* + * SonarQube Java + * Copyright (C) SonarSource Sàrl + * mailto:info AT sonarsource DOT com + * + * You can redistribute and/or modify this program under the terms of + * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. + * + * 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 Sonar Source-Available License for more details. + * + * You should have received a copy of the Sonar Source-Available License + * along with this program; if not, see https://sonarsource.com/license/ssal/ + */ +package org.sonar.java.checks; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; + +import static org.sonar.java.checks.verifier.TestUtils.mainCodeSourcesPath; +import static org.sonar.java.checks.verifier.TestUtils.nonCompilingTestSourcesPath; + +class YodaConditionCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/YodaConditionCheckSample.java")) + .withCheck(new YodaConditionCheck()) + .verifyIssues(); + } + + @Test + void no_issue_without_semantic() { + CheckVerifier.newVerifier() + .onFile(mainCodeSourcesPath("checks/YodaConditionCheckSample.java")) + .withCheck(new YodaConditionCheck()) + .withoutSemantic() + .verifyIssues(); + } + + @Test + void test_non_compiling() { + CheckVerifier.newVerifier() + .onFile(nonCompilingTestSourcesPath("checks/YodaConditionCheckSample.java")) + .withCheck(new YodaConditionCheck()) + .verifyNoIssues(); + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html new file mode 100644 index 00000000000..63e30ee4b71 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.html @@ -0,0 +1,58 @@ +

This rule raises an issue when comparisons place constants on the left side (Yoda conditions).

+

Why is this an issue?

+

Yoda conditions place the constant on the left side of a comparison: 0 == count instead of count == 0. This pattern +originated in C programming to prevent accidental assignment (single equals operator) instead of comparison (double equals operator).

+

In modern type-safe languages, this defensive technique is unnecessary. Type systems that distinguish between assignment and comparison contexts +will reject code that attempts assignment where a boolean expression is expected. For example, attempting to assign a numeric value in a conditional +expression produces a compiler error in languages with strong type checking.

+

Placing constants on the left reduces readability. Natural language flows from subject to comparison: "Is the count zero?" translates more +naturally to count == 0 than to 0 == count.

+

By following conventional comparison order, code becomes more intuitive for developers to read and maintain.

+

What is the potential impact?

+

The impact on code quality is primarily related to maintainability:

+ +

How to fix it

+

Reverse the comparison to place the variable on the left and the constant on the right.

+

Code examples

+

Noncompliant code example

+
+public class Example {
+    public void check() {
+        int count = 0;
+        if (0 == count) { // Noncompliant
+            return;
+        }
+
+        Object obj = null;
+        boolean flag = true;
+        if (null == obj && true == flag) { // Noncompliant
+            System.out.println("Both conditions met");
+        }
+    }
+}
+
+

Compliant solution

+
+public class Example {
+    public void check() {
+        int count = 0;
+        if (count == 0) { // Natural comparison order
+            return;
+        }
+
+        Object obj = null;
+        boolean flag = true;
+        if (obj == null && flag) { // Natural comparison order
+            System.out.println("Both conditions met");
+        }
+    }
+}
+
+

Resources

+

Documentation

+ diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.json new file mode 100644 index 00000000000..a0b0b7c8b34 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9360.json @@ -0,0 +1,25 @@ +{ + "title": "Comparisons should not use Yoda conditions", + "type": "CODE_SMELL", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5 min" + }, + "tags": [ + "confusing", + "convention", + "clarity" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-9360", + "sqKey": "S9360", + "scope": "All", + "quickfix": "unknown", + "code": { + "impacts": { + "MAINTAINABILITY": "MEDIUM" + }, + "attribute": "CLEAR" + } +} diff --git a/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9360 b/sonar-java-plugin/src/main/resources/profiles/Sonar_way/S9360 new file mode 100644 index 00000000000..e69de29bb2d