SONARJAVA-1573 Fix FP on RSPEC-2638 to respect LSP

Author: benzonico

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

@@ -28,6 +28,7 @@
 import org.sonar.java.tag.Tag;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.semantic.Symbol;
+import org.sonar.plugins.java.api.tree.AnnotationTree;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.squidbridge.annotations.ActivatedByDefault;
 import org.sonar.squidbridge.annotations.SqaleConstantRemediation;
@@ -71,13 +72,22 @@ private void checkContractChange(MethodTreeImpl methodTree, JavaSymbol.MethodJav
     for (int i = 0; i < methodTree.parameters().size(); i++) {
       Symbol paramSymbol = methodTree.parameters().get(i).symbol();
       Symbol overrideeParamSymbol = overridee.getParameters().scopeSymbols().get(i);
-      if (nonNullVsNull(paramSymbol, overrideeParamSymbol) || nonNullVsNull(overrideeParamSymbol, paramSymbol)) {
-        reportIssue(methodTree.parameters().get(i), "The \"" + paramSymbol.name()
-            + "\" parameter nullability is different in the superclass method, and that should not be changed.");
+      if (nonNullVsNull(paramSymbol, overrideeParamSymbol)) {
+        Tree reportTree = methodTree.parameters().get(i);
+        for (AnnotationTree annotationTree : methodTree.parameters().get(i).modifiers().annotations()) {
+          if(annotationTree.symbolType().is("javax.annotation.Nonnull")) {
+            reportTree = annotationTree;
+          }
+        }
+        reportIssue(reportTree, "Remove this \"Nonnull\" annotation to honor the overridden method's contract.");
       }
     }
-    if (nonNullVsNull(methodTree.symbol(), overridee) || nonNullVsNull(overridee, methodTree.symbol())) {
-      reportIssue(methodTree.returnType(), "The return value nullability of this method is different in the superclass, and that should not be changed.");
+    if (nonNullVsNull(overridee, methodTree.symbol())) {
+      for (AnnotationTree annotationTree : methodTree.modifiers().annotations()) {
+        if(annotationTree.symbolType().is("javax.annotation.Nullable") || annotationTree.symbolType().is("javax.annotation.CheckForNull")) {
+          reportIssue(annotationTree, "Remove this \""+ annotationTree.symbolType().name() +"\" annotation to honor the overridden method's contract.");
+        }
+      }
     }
   }
 

java-checks/src/main/resources/org/sonar/l10n/java/rules/squid/S2638.html

@@ -1,4 +1,4 @@
-<p>Because a subclass instance may be cast to and treated as an instance of the superclass, overriding methods should uphold the same contracts as the ones in the superclass. Specifically, if the parameters or return type of the superclass method are marked with any of the following, that should not be changed in a subclass: <code>@Nullable</code>, <code>@CheckForNull</code>, <code>@NotNull</code>, <code>@NonNull</code>, and <code>@Nonnull</code>.</p>
+<p>Because a subclass instance may be cast to and treated as an instance of the superclass, overriding methods should uphold the aspects of the superclass contract that relate to the Liskov Substitution Principle. Specifically, if the parameters or return type of the superclass method are marked with any of the following: <code>@Nullable</code>, <code>@CheckForNull</code>, <code>@NotNull</code>, <code>@NonNull</code>, and <code>@Nonnull</code>, then subclass parameters are not allowed to tightened the contract, and return values are not allowed to loosen it.</p>
 
 <h2>Noncompliant Code Example</h2>
 <pre>
@@ -7,7 +7,7 @@ <h2>Noncompliant Code Example</h2>
   private Season ripe;
   private String color;
 
-  public void setRipe(@NotNull Season ripe) {
+  public void setRipe(@Nullable Season ripe) {
     this.ripe = ripe;
   }
 
@@ -18,7 +18,7 @@ <h2>Noncompliant Code Example</h2>
 
 public class Raspberry extends Fruit {
 
-  public void setRipe(@Nullable Season ripe) {  // Noncompliant
+  public void setRipe(@NotNull Season ripe) {  // Noncompliant
     this.ripe = ripe;
   }
 
@@ -28,3 +28,6 @@ <h2>Noncompliant Code Example</h2>
 }
 </pre>
 
+<h2>See also</h2>
+<p>https://en.wikipedia.org/wiki/Liskov_substitution_principle</p>
+

java-checks/src/test/files/checks/ChangeMethodContractCheck.java

@@ -27,30 +27,32 @@ void foo1(@javax.annotation.CheckForNull Object a) { }
     void foo2(@javax.annotation.Nullable Object a) { }
 
     @Override
-    void foo3(@javax.annotation.CheckForNull Object a) { } // Noncompliant [[sc=15;ec=54]] {{The "a" parameter nullability is different in the superclass method, and that should not be changed.}}
+    void foo3(@javax.annotation.CheckForNull Object a) { }
     @javax.annotation.Nullable
     void foo4(Object a) { }
     @javax.annotation.CheckForNull
     void foo5(Object a) { }
-    @javax.annotation.CheckForNull
-    void foo6(Object a) { } // Noncompliant
+    @javax.annotation.CheckForNull // Noncompliant
+    void foo6(Object a) { }
     public boolean equals(Object o) { } // compliant
 }
 
 class C extends A {
     @Override
-    void foo1(@javax.annotation.Nonnull Object a) { } // Noncompliant
+    void foo1(@javax.annotation.Nonnull Object a) { } // Noncompliant {{Remove this "Nonnull" annotation to honor the overridden method's contract.}}
     @Override
     void foo2(@javax.annotation.Nonnull Object a) { } // Noncompliant
     @Override
-    void foo3(@javax.annotation.Nullable Object a) { } // Noncompliant
+    void foo3(@javax.annotation.Nullable Object a) { }
 
     @javax.annotation.Nonnull
-    void foo4(Object a) { } // Noncompliant
+    @Deprecated
+    void foo4(Object a) { }
     @javax.annotation.Nonnull
-    void foo5(Object a) { } // Noncompliant
-    @javax.annotation.Nullable
-    void foo6(Object a) { } // Noncompliant [[sc=5;ec=9]] {{The return value nullability of this method is different in the superclass, and that should not be changed.}}
+    void foo5(Object a) { }
+    @Deprecated
+    @javax.annotation.Nullable // Noncompliant [[sc=5;ec=31]] {{Remove this "Nullable" annotation to honor the overridden method's contract.}}
+    void foo6(Object a) { }
 
     public boolean equals(@javax.annotation.Nonnull Object o) { } // Noncompliant {{Equals method should accept null parameters and return false.}}
 }