SONARJAVA-7162 Implement rule S9435 - Ternary operator conditions should not be negated

its/ruling/src/test/resources/expected/sonarqube-6.5/java-S9435.json

@@ -0,0 +1,9 @@
+{
+"sonarqube-6.5:src/main/java/org/sonar/server/debt/DebtRulesXMLImporter.java": [
+254
+],
+"sonarqube-6.5:src/test/java/org/sonar/server/issue/IssueDocTesting.java": [
+69,
+74
+]
+}

java-checks-test-sources/default/src/main/java/checks/NegatedTernaryConditionCheckSample.java

@@ -0,0 +1,121 @@
+package checks;
+
+import java.util.List;
+
+public class NegatedTernaryConditionCheckSample {
+
+  private boolean verbose;
+  private boolean flag;
+  private int count;
+  private String state;
+  private Object value;
+  private List<String> items;
+
+  String negatedBooleanVariable() {
+    return !verbose ? "short" : "long"; // Noncompliant {{Invert this negated condition and swap the two branches of the ternary.}}
+//         ^^^^^^^^
+  }
+
+  String notEqualComparison() {
+    return count != 0 ? "some" : "none"; // Noncompliant
+//         ^^^^^^^^^^
+  }
+
+  String negatedMethodCall() {
+    return !items.isEmpty() ? items.get(0) : "none"; // Noncompliant
+//         ^^^^^^^^^^^^^^^^
+  }
+
+  String negatedEqualsCall() {
+    return !"SHIPPED".equals(state) ? "Pending" : "Shipped"; // Noncompliant
+//         ^^^^^^^^^^^^^^^^^^^^^^^^
+  }
+
+  String parenthesizedCondition() {
+    return (!verbose) ? "short" : "long"; // Noncompliant
+//         ^^^^^^^^^^
+  }
+
+  String negatedConjunction() {
+    return !(verbose && flag) ? "short" : "long"; // Noncompliant
+//         ^^^^^^^^^^^^^^^^^^
+  }
+
+  String inLocalVariable() {
+    String label = !flag ? "off" : "on"; // Noncompliant
+//                 ^^^^^
+    return label;
+  }
+
+  String asArgument() {
+    return String.valueOf(!flag ? "off" : "on"); // Noncompliant
+//                        ^^^^^
+  }
+
+  String nestedTernaryInBranch() {
+    return verbose ? (!flag ? "off" : "on") : "none"; // Noncompliant
+//                    ^^^^^
+  }
+
+  String bothTernariesNegated() {
+    return !verbose // Noncompliant
+//         ^^^^^^^^
+      ? "short"
+      : (count != 0 ? "some" : "none"); // Noncompliant
+//       ^^^^^^^^^^
+  }
+
+  // Compliant cases
+
+  String positiveCondition() {
+    return verbose ? "long" : "short";
+  }
+
+  String equalComparison() {
+    return count == 0 ? "none" : "some";
+  }
+
+  String positiveMethodCall() {
+    return items.isEmpty() ? "none" : items.get(0);
+  }
+
+  String nullCheckIsExempt() {
+    return value != null ? value.toString() : "unknown";
+  }
+
+  String reversedNullCheckIsExempt() {
+    return null != value ? value.toString() : "unknown";
+  }
+
+  String negatedNullEqualityIsExempt() {
+    return !(value == null) ? value.toString() : "unknown";
+  }
+
+  String relationalComparison() {
+    return count > 0 ? "some" : "none";
+  }
+
+  String conjunctionIsNotNegatedAtTopLevel() {
+    return verbose && flag ? "both" : "other";
+  }
+
+  String nestedNotEqualIsNotNegatedAtTopLevel() {
+    return value != null && count != 0 ? "both" : "other";
+  }
+
+  String negatedConditionInIfStatementIsOutOfScope() {
+    if (!verbose) {
+      return "short";
+    } else {
+      return "long";
+    }
+  }
+
+  boolean negationOutsideTernary() {
+    return !verbose;
+  }
+
+  boolean notEqualOutsideTernary() {
+    return count != 0;
+  }
+}

java-checks/src/main/java/org/sonar/java/checks/NegatedTernaryConditionCheck.java

@@ -0,0 +1,71 @@
+/*
+ * 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.ConditionalExpressionTree;
+import org.sonar.plugins.java.api.tree.ExpressionTree;
+import org.sonar.plugins.java.api.tree.Tree;
+import org.sonar.plugins.java.api.tree.UnaryExpressionTree;
+
+@Rule(key = "S9435")
+public class NegatedTernaryConditionCheck extends IssuableSubscriptionVisitor {
+
+  private static final String MESSAGE = "Invert this negated condition and swap the two branches of the ternary.";
+
+  @Override
+  public List<Tree.Kind> nodesToVisit() {
+    return List.of(Tree.Kind.CONDITIONAL_EXPRESSION);
+  }
+
+  @Override
+  public void visitNode(Tree tree) {
+    ExpressionTree condition = ((ConditionalExpressionTree) tree).condition();
+    if (isNegated(ExpressionUtils.skipParentheses(condition))) {
+      reportIssue(condition, MESSAGE);
+    }
+  }
+
+  private static boolean isNegated(ExpressionTree condition) {
+    if (condition.is(Tree.Kind.LOGICAL_COMPLEMENT)) {
+      ExpressionTree negated = ExpressionUtils.skipParentheses(((UnaryExpressionTree) condition).expression());
+      // "!(value == null)" is a null check spelled the long way, and is exempt like "value != null".
+      return !isNullComparison(negated, Tree.Kind.EQUAL_TO);
+    }
+    return isNotEqualButNotNullCheck(condition);
+  }
+
+  private static boolean isNotEqualButNotNullCheck(ExpressionTree condition) {
+    return condition.is(Tree.Kind.NOT_EQUAL_TO) && !isNullComparison(condition, Tree.Kind.NOT_EQUAL_TO);
+  }
+
+  private static boolean isNullComparison(ExpressionTree expression, Tree.Kind kind) {
+    if (!expression.is(kind)) {
+      return false;
+    }
+    BinaryExpressionTree comparison = (BinaryExpressionTree) expression;
+    return isNullLiteral(comparison.leftOperand()) || isNullLiteral(comparison.rightOperand());
+  }
+
+  private static boolean isNullLiteral(ExpressionTree expression) {
+    return ExpressionUtils.skipParentheses(expression).is(Tree.Kind.NULL_LITERAL);
+  }
+}

java-checks/src/test/java/org/sonar/java/checks/NegatedTernaryConditionCheckTest.java

@@ -0,0 +1,45 @@
+/*
+ * 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;
+
+class NegatedTernaryConditionCheckTest {
+
+  private static final String SAMPLE = "checks/NegatedTernaryConditionCheckSample.java";
+  private static final NegatedTernaryConditionCheck CHECK = new NegatedTernaryConditionCheck();
+
+  @Test
+  void test() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath(SAMPLE))
+      .withCheck(CHECK)
+      .verifyIssues();
+  }
+
+  @Test
+  void test_without_semantic() {
+    CheckVerifier.newVerifier()
+      .onFile(mainCodeSourcesPath(SAMPLE))
+      .withCheck(CHECK)
+      .withoutSemantic()
+      .verifyIssues();
+  }
+}

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9435.html

@@ -0,0 +1,59 @@
+<h2>Why is this an issue?</h2>
+<p>A ternary expression packs both outcomes of a choice into a single line, so the reader takes in the condition and the two results at once. When the
+condition is negated, that reading no longer works in one pass: the reader has to invert the condition mentally before pairing each result with the
+case it belongs to. In <code>order.status() != Status.SHIPPED ? "Pending" : "Shipped"</code>, the value that goes with "shipped" is the second one,
+even though shipping is what the condition talks about.</p>
+<p>Flipping the condition and swapping the two results expresses exactly the same logic without that double negative. It also settles the ordering
+question in a consistent way: the positive case, which is usually the common one, comes first.</p>
+<p>This rule raises an issue on a ternary expression whose condition is negated. A condition counts as negated when it is a <code>!</code> expression
+such as <code>!verbose</code> or <code>!list.isEmpty()</code>, or a <code>!=</code> comparison.</p>
+<p>The rule is only about the readability of the two branches of a ternary. Negated conditions elsewhere, such as in <code>if</code> statements, are
+out of scope.</p>
+<h3>Exceptions</h3>
+<p>Null checks do not raise an issue. <code>value != null ? value.name() : "unknown"</code> reads naturally as a positive guard, "if there is a
+value", so inverting it would not help the reader.</p>
+<h2>How to fix it</h2>
+<p>Replace the condition with its positive form and swap the two result expressions, keeping each result with the case it handles. For
+<code>!x</code>, use <code>x</code>. For <code>x != y</code>, use <code>x == y</code>. For a negated call such as <code>!list.isEmpty()</code>, use
+<code>list.isEmpty()</code>.</p>
+<p>The rewritten expression must produce the same value as before, so make sure you move both results and not just the condition. Evaluation order
+matters when a result expression has a side effect or can throw, because only the selected branch is evaluated, and swapping the branches must not
+change which one that is for a given input.</p>
+<p>If the negated form is genuinely the clearer one, for example because the negative case is the one the surrounding code exists to handle, this is a
+trade-off you may decide to keep rather than a false positive.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<pre data-diff-id="1" data-diff-type="noncompliant">
+public class OrderFormatter {
+
+  public String statusLabel(Order order) {
+    return order.status() != Status.SHIPPED ? "Pending" : "Shipped"; // Noncompliant: negated condition
+  }
+
+  public String summarize(Order order, boolean verbose) {
+    return !verbose ? order.id() : order.id() + " for " + order.customer(); // Noncompliant: negated condition
+  }
+}
+</pre>
+<h4>Compliant solution</h4>
+<pre data-diff-id="1" data-diff-type="compliant">
+public class OrderFormatter {
+
+  public String statusLabel(Order order) {
+    return order.status() == Status.SHIPPED ? "Shipped" : "Pending";
+  }
+
+  public String summarize(Order order, boolean verbose) {
+    return verbose ? order.id() + " for " + order.customer() : order.id();
+  }
+}
+</pre>
+<h2>Resources</h2>
+<h3>Related rules</h3>
+<ul>
+  <li>{rule:java:S1940} - Boolean checks should not be inverted
+    <br>
+    <em>{rule:java:S1940} flags a negation that is redundant in itself, such as <code>!(a == b)</code>, wherever it appears in the code. This rule
+    keeps the negation count unchanged and only reorders the two branches of a ternary.</em></li>
+</ul>
+

sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S9435.json

@@ -0,0 +1,23 @@
+{
+  "title": "Ternary operator conditions should not be negated",
+  "type": "CODE_SMELL",
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "2min"
+  },
+  "tags": [
+    "clumsy"
+  ],
+  "defaultSeverity": "Minor",
+  "ruleSpecification": "RSPEC-9435",
+  "sqKey": "S9435",
+  "scope": "All",
+  "code": {
+    "impacts": {
+      "MAINTAINABILITY": "LOW"
+    },
+    "attribute": "CLEAR"
+  },
+  "quickfix": "targeted"
+}