SONARJAVA-6084 Validation logic should be placed in constructor prologue when possible (#5446)

Author: asya-vorobeva

its/autoscan/src/test/resources/autoscan/diffs/diff_S8433.json

@@ -0,0 +1,6 @@
+{
+  "ruleKey": "S8433",
+  "hasTruePositives": true,
+  "falseNegatives": 0,
+  "falsePositives": 0
+}

java-checks-test-sources/default/src/main/files/non-compiling/checks/FlexibleConstructorBodyValidationCheckSample.java

@@ -0,0 +1,169 @@
+package checks;
+
+import java.util.Objects;
+
+class FlexibleConstructorBodyValidationCheckSample {
+
+  static class Coffee {
+    Coffee() {}
+    Coffee(int water, int milk) {}
+  }
+
+  static class SmallCoffee extends Coffee {
+    private String topping;
+    private int maxVolume = 100;
+
+    public SmallCoffee(int water, int milk, String topping) {
+      super(water, milk);
+      int maxCupCoffee = 90;
+      int totalVolume = water + milk;
+      if (totalVolume > maxCupCoffee) { // Noncompliant {{Move this validation logic before the super() or this() call.}}
+    //^[el=+3;ec=7]
+        throw new IllegalArgumentException();
+      }
+      this.topping = topping;
+    }
+
+    public SmallCoffee(int water, int milk) {
+      int totalVolume = water + milk;
+      int maxCupCoffee = 90;
+      if (totalVolume > maxCupCoffee) { // Compliant: validation before super()
+        throw new IllegalArgumentException();
+      }
+      maxCupCoffee++;
+      super(water, milk);
+    }
+
+    public SmallCoffee(int water) {
+      super(water, 0);
+      if (water > maxVolume) { // Compliant: Uses instance field
+        throw new IllegalArgumentException();
+      }
+    }
+
+    public SmallCoffee() {
+      super(0, 0); // Compliant: no validation
+    }
+  }
+
+  static class MediumCoffee extends Coffee {
+    private String flavor;
+
+    public MediumCoffee(int water, int milk) {
+      this.isValidFlavor("");
+      super(water, milk);
+      if (water + milk > MAX_SIZE) { // Noncompliant
+        throw new IllegalArgumentException();
+      }
+      this.isValidFlavor("");
+      this.flavor = "Vanilla";
+    }
+
+    public MediumCoffee(int water) {
+      if (water < 0) { // Compliant: throw before this()
+        throw new IllegalArgumentException("Water cannot be negative");
+      }
+      this(water, 0);
+    }
+
+    public MediumCoffee(String flavor) {
+      this(100, 50);
+      if (!isValidFlavor(flavor)) { // Compliant: validation uses instance method, cannot be moved
+        throw new IllegalArgumentException();
+      }
+      this.flavor = flavor;
+    }
+
+    private boolean isValidFlavor(String flavor) {
+      return flavor != null && !flavor.isEmpty();
+    }
+  }
+
+  // Test using static members
+  static class LargeCoffee extends Coffee {
+    private String name;
+    private static final int MAX_SIZE = 500;
+
+    public LargeCoffee(int water, int milk) {
+      super(water, milk);
+      if (water + milk > MAX_SIZE) { // Noncompliant
+        throw new IllegalArgumentException();
+      }
+    }
+
+    public LargeCoffee(String name) {
+      super(100, 100);
+      this.name = name;
+      if (!isValidName(name)) { // NonCompliant
+        throw new IllegalArgumentException();
+      }
+    }
+
+    private static boolean isValidName(String name) {
+      return name != null && !name.isEmpty();
+    }
+  }
+
+  // Test with different validation libraries
+  static class GuavaCoffee extends Coffee {
+    public GuavaCoffee(String name) {
+      super(100, 50);
+      com.google.common.base.Preconditions.checkNotNull(name); // Noncompliant {{Move this validation logic before the super() or this() call.}}
+    }
+
+    public GuavaCoffee(String name) {
+      com.google.common.base.Preconditions.checkNotNull(name); // Compliant
+      super(100, 50);
+    }
+  }
+
+  static class SpringCoffee extends Coffee {
+    public SpringCoffee(String name) {
+      super(100, 50);
+      org.springframework.util.Assert.notNull(name, "Name must not be null"); // Noncompliant {{Move this validation logic before the super() or this() call.}}
+    }
+
+    public SpringCoffee(String name) {
+      org.springframework.util.Assert.notNull(name, "Name must not be null"); // Compliant
+      super(100, 50);
+    }
+  }
+
+  // Test with implicit super()
+  static class ImplicitSuperCoffee extends Coffee {
+    public ImplicitSuperCoffee(int water) {
+      // Implicit super() call here
+      if (water < 0) { // Noncompliant
+        throw new IllegalArgumentException();
+      }
+    }
+  }
+
+  // Test multiple validations
+  static class MultiValidationCoffee extends Coffee {
+    public MultiValidationCoffee(int water, int milk, String name) {
+      super(water, milk);
+      if (water < 0) { // Noncompliant
+        throw new IllegalArgumentException("Invalid water");
+      }
+      if (milk < 0) { // Noncompliant
+        throw new IllegalArgumentException("Invalid milk");
+      }
+      Objects.requireNonNull(name); // Noncompliant
+    }
+  }
+
+  // Test nested if statements
+  static class NestedIfCoffee extends Coffee {
+    public NestedIfCoffee(int water, int milk) {
+      super(water, milk);
+      if (water > 0) { // Noncompliant
+        if (milk > 0) {
+          if (water + milk > 200) {
+            throw new IllegalArgumentException();
+          }
+        }
+      }
+    }
+  }
+}

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

@@ -17,6 +17,7 @@
 package org.sonar.java.checks;
 
 import org.sonar.check.Rule;
+import org.sonar.java.model.ExpressionUtils;
 import org.sonar.java.model.JUtils;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.semantic.Symbol;
@@ -93,18 +94,13 @@ private static boolean superTypesHaveOnlyStaticMethods(Symbol.TypeSymbol newClas
   private static Collection<Symbol> filterMethodsAndFields(Collection<Symbol> symbols) {
     List<Symbol> filtered = new ArrayList<>();
     for (Symbol symbol : symbols) {
-      if ((symbol.isVariableSymbol() && !isThisOrSuper(symbol)) || (symbol.isMethodSymbol() && !isConstructor(symbol))) {
+      if ((symbol.isVariableSymbol() && !ExpressionUtils.isThisOrSuper(symbol.name())) || (symbol.isMethodSymbol() && !isConstructor(symbol))) {
         filtered.add(symbol);
       }
     }
     return filtered;
   }
 
-  private static boolean isThisOrSuper(Symbol symbol) {
-    String name = symbol.name();
-    return "this".equals(name) || "super".equals(name);
-  }
-
   private static boolean isConstructor(Symbol symbol) {
     return "<init>".equals(symbol.name());
   }