Bug 434600 - Incorrect null analysis error reporting on type parameters

- the payload

3 files changed, 165 insertions, 9 deletions

diff --git a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java
index e09eb9e4ee..6444d6e509 100644
--- a/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java
+++ b/org.eclipse.jdt.core.tests.compiler/src/org/eclipse/jdt/core/tests/compiler/regression/NullTypeAnnotationTest.java
@@ -5292,4 +5292,133 @@ public void testTypeVariable6a() {
 		getCompilerOptions(),
 		"");
 }
+public void testBug434600() {
+	runConformTestWithLibs(
+		new String[] {
+			"bug/Main.java",
+			"package bug;\n" +
+			"public class Main {\n" + 
+			"	public static void main(final String[] args) {\n" + 
+			"		System.out.println(\"Hello World\");\n" + 
+			"	}\n" + 
+			"}\n",
+			"bug/package-info.java",
+			"@org.eclipse.jdt.annotation.NonNullByDefault\n" + 
+			"package bug;\n",
+			"bug/ExpressionNode.java",
+			"package bug;\n" + 
+			"\n" + 
+			"public interface ExpressionNode extends CopyableNode<ExpressionNode> {\n" + 
+			"	\n" + 
+			"}\n",
+			"bug/ExtendedNode.java",
+			"package bug;\n" + 
+			"\n" + 
+			"public interface ExtendedNode {\n" + 
+			"	\n" + 
+			"}\n",
+			"bug/CopyableNode.java",
+			"package bug;\n" + 
+			"\n" + 
+			"public interface CopyableNode<T extends ExtendedNode> extends ExtendedNode {\n" + 
+			"	\n" + 
+			"}\n"
+		},
+		getCompilerOptions(),
+		"",
+		"Hello World");
+}
+public void testBug434600a() {
+	runConformTestWithLibs(
+		new String[] {
+			"I.java",
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"interface I<S, T extends @Nullable List<@NonNull List<S>>> {\n" +
+			"}\n",
+			"C.java",
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"public class C implements I<@Nullable String, @Nullable ArrayList<@NonNull List<@Nullable String>>> {}\n"
+		},
+		getCompilerOptions(),
+		"");
+}
+public void testBug434600a_qualified() {
+	runConformTestWithLibs(
+		new String[] {
+			"p/I.java",
+			"package p;\n" +
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"public interface I<S, T extends @Nullable List<@NonNull List<S>>> {\n" +
+			"}\n",
+			"C.java",
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"public class C implements p.I<java.lang.@Nullable String, java.util.@Nullable ArrayList<java.util.@NonNull List<java.lang.@Nullable String>>> {}\n"
+		},
+		getCompilerOptions(),
+		"");
+}
+public void testBug434600b() {
+	runNegativeTestWithLibs(
+		new String[] {
+			"I.java",
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"interface I<S, T extends @Nullable List<@NonNull List<S>>> {\n" +
+			"}\n",
+			"C.java",
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"public class C implements I<@Nullable String, ArrayList<@NonNull List<@Nullable String>>> {}\n" +
+			"class C1 {\n" +
+			"	I<String, @Nullable ArrayList<@Nullable List<String>>> field;\n" +
+			"}\n" +
+			"class C2 implements I<@NonNull String, @Nullable ArrayList<@NonNull List<@Nullable String>>> {}\n" // FIXME: cross checking for contradictory substitution for 'S' NYI
+		},
+		getCompilerOptions(),
+		"----------\n" + 
+		"1. ERROR in C.java (at line 3)\n" + 
+		"	public class C implements I<@Nullable String, ArrayList<@NonNull List<@Nullable String>>> {}\n" + 
+		"	                                              ^^^^^^^^^\n" + 
+		"Null constraint mismatch: The type \'ArrayList<@NonNull List<@Nullable String>>\' is not a valid substitute for the type parameter \'T extends @Nullable List<@NonNull List<S>>\'\n" + 
+		"----------\n" + 
+		"2. ERROR in C.java (at line 5)\n" + 
+		"	I<String, @Nullable ArrayList<@Nullable List<String>>> field;\n" + 
+		"	          ^^^^^^^^^^^^^^^^^^^\n" + 
+		"Null constraint mismatch: The type \'@Nullable ArrayList<@Nullable List<String>>\' is not a valid substitute for the type parameter \'T extends @Nullable List<@NonNull List<S>>\'\n" + 
+		"----------\n");
+}
+public void testBug434600b_qualified() {
+	runNegativeTestWithLibs(
+		new String[] {
+			"p/I.java",
+			"package p;\n" +
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"public interface I<S, T extends @Nullable List<@NonNull List<S>>> {\n" +
+			"}\n",
+			"C.java",
+			"import java.util.*;\n" +
+			"import org.eclipse.jdt.annotation.*;\n" +
+			"public class C implements p.I<@Nullable String, ArrayList<@NonNull List<@Nullable String>>> {}\n" +
+			"class C1 {\n" +
+			"	p.I<String, @Nullable ArrayList<@Nullable List<String>>> field;\n" +
+			"}\n" +
+			"class C2 implements p.I<@NonNull String, @Nullable ArrayList<@NonNull List<@Nullable String>>> {}\n" // FIXME: cross checking for contradictory substitution for 'S' NYI
+		},
+		getCompilerOptions(),
+		"----------\n" + 
+		"1. ERROR in C.java (at line 3)\n" + 
+		"	public class C implements p.I<@Nullable String, ArrayList<@NonNull List<@Nullable String>>> {}\n" + 
+		"	                                                ^^^^^^^^^\n" + 
+		"Null constraint mismatch: The type \'ArrayList<@NonNull List<@Nullable String>>\' is not a valid substitute for the type parameter \'T extends @Nullable List<@NonNull List<S>>\'\n" + 
+		"----------\n" + 
+		"2. ERROR in C.java (at line 5)\n" + 
+		"	p.I<String, @Nullable ArrayList<@Nullable List<String>>> field;\n" + 
+		"	            ^^^^^^^^^^^^^^^^^^^\n" + 
+		"Null constraint mismatch: The type \'@Nullable ArrayList<@Nullable List<String>>\' is not a valid substitute for the type parameter \'T extends @Nullable List<@NonNull List<S>>\'\n" + 
+		"----------\n");
+}
 }
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/NullAnnotationMatching.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/NullAnnotationMatching.java
index 5bc805e6c7..cc77802997 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/NullAnnotationMatching.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/NullAnnotationMatching.java
@@ -50,7 +50,9 @@ public class NullAnnotationMatching {
 		/** in this mode we check normal assignment compatibility. */
 		COMPATIBLE,
 		/** in this mode we do not tolerate incompatibly missing annotations on type parameters (for overriding analysis) */
-		OVERRIDE
+		OVERRIDE,
+		/** in this mode we check compatibility of a type argument against the corresponding type parameter. */
+		BOUND_CHECK
 	}
 
 	/** 0 = OK, 1 = unchecked, 2 = definite mismatch */
@@ -143,6 +145,27 @@ public class NullAnnotationMatching {
 				return NullAnnotationMatching.NULL_ANNOTATIONS_OK_NONNULL;
 			return okStatus;
 		}
+		if (mode == CheckMode.BOUND_CHECK && requiredType instanceof TypeVariableBinding) {
+			// during bound check against a type variable check the provided type against all upper bounds:
+			TypeBinding superClass = requiredType.superclass();
+			if (superClass != null && superClass.hasNullTypeAnnotations()) {
+				NullAnnotationMatching status = analyse(superClass, providedType, null, nullStatus, mode);
+				severity = Math.max(severity, status.severity);
+				if (severity == 2)
+					return new NullAnnotationMatching(severity, nullStatus, superTypeHint);
+			}
+			TypeBinding[] superInterfaces = requiredType.superInterfaces();
+			if (superInterfaces != null) {
+				for (int i = 0; i < superInterfaces.length; i++) {
+					if (superInterfaces[i].hasNullTypeAnnotations()) {
+						NullAnnotationMatching status = analyse(superInterfaces[i], providedType, null, nullStatus, mode);
+						severity = Math.max(severity, status.severity);
+						if (severity == 2)
+							return new NullAnnotationMatching(severity, nullStatus, superTypeHint);						
+					}
+				}
+			}
+		}
 		if (requiredType instanceof ArrayBinding) {
 			long[] requiredDimsTagBits = ((ArrayBinding)requiredType).nullTagBitsPerDimension;
 			if (requiredDimsTagBits != null) {
@@ -168,12 +191,13 @@ public class NullAnnotationMatching {
 				}
 			}
 		} else if (requiredType.hasNullTypeAnnotations() || providedType.hasNullTypeAnnotations() || requiredType.isTypeVariable()) {
-			long requiredBits = requiredNullTagBits(requiredType);
+			long requiredBits = requiredNullTagBits(requiredType, mode);
 			if (requiredBits != TagBits.AnnotationNullable // nullable lhs accepts everything, ...
 					|| nullStatus == -1) // only at detail/recursion even nullable must be matched exactly
 			{
 				long providedBits = providedNullTagBits(providedType);
-				severity = computeNullProblemSeverity(requiredBits, providedBits, nullStatus, mode == CheckMode.OVERRIDE && nullStatus == -1);
+				int s = computeNullProblemSeverity(requiredBits, providedBits, nullStatus, mode == CheckMode.OVERRIDE && nullStatus == -1);
+				severity = Math.max(severity, s);
 				if (severity == 0 && (providedBits & TagBits.AnnotationNonNull) != 0)
 					okStatus = NullAnnotationMatching.NULL_ANNOTATIONS_OK_NONNULL;
 			}
@@ -236,7 +260,7 @@ public class NullAnnotationMatching {
 
 	// interpreting 'type' as a required type, compute the required null bits
 	// we inspect the main type plus bounds of type variables and wildcards
-	static long requiredNullTagBits(TypeBinding type) {
+	static long requiredNullTagBits(TypeBinding type, CheckMode mode) {
 
 		long tagBits = type.tagBits & TagBits.AnnotationNullMASK;
 		if (tagBits != 0)
@@ -273,7 +297,8 @@ public class NullAnnotationMatching {
 						return TagBits.AnnotationNullable; // type cannot require @NonNull
 				}
 			}
-			return TagBits.AnnotationNonNull; // instantiation could require @NonNull
+			if (mode != CheckMode.BOUND_CHECK) // no pessimistic checks during boundcheck (we *have* the instantiation)
+				return TagBits.AnnotationNonNull; // instantiation could require @NonNull
 		}
 
 		return 0;
diff --git a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TypeReference.java b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TypeReference.java
index 940c752650..d2acbf5cef 100644
--- a/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TypeReference.java
+++ b/org.eclipse.jdt.core/compiler/org/eclipse/jdt/internal/compiler/ast/TypeReference.java
@@ -17,6 +17,7 @@
  *								Bug 427163 - [1.8][null] bogus error "Contradictory null specification" on varags
  *								Bug 429958 - [1.8][null] evaluate new DefaultLocation attribute of @NonNullByDefault
  *								Bug 434570 - Generic type mismatch for parametrized class annotation attribute with inner class
+ *								Bug 434600 - Incorrect null analysis error reporting on type parameters
  *        Andy Clement (GoPivotal, Inc) aclement@gopivotal.com - Contributions for
  *                          Bug 383624 - [1.8][compiler] Revive code generation support for type annotations (from Olivier's work)
  *                          Bug 409236 - [1.8][compiler] Type annotations on intersection cast types dropped by code generator
@@ -28,6 +29,7 @@ import java.util.ArrayList;
 import java.util.List;
 
 import org.eclipse.jdt.internal.compiler.ASTVisitor;
+import org.eclipse.jdt.internal.compiler.ast.NullAnnotationMatching.CheckMode;
 import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants;
 import org.eclipse.jdt.internal.compiler.codegen.AnnotationContext;
 import org.eclipse.jdt.internal.compiler.codegen.AnnotationTargetTypeConstants;
@@ -658,10 +660,10 @@ protected void checkNullConstraints(Scope scope, TypeReference[] typeArguments)
 /** Check whether this type reference conforms to all null constraints defined for any of the given type variables. */
 protected void checkNullConstraints(Scope scope, TypeBinding[] variables, int rank) {
 	if (variables != null && variables.length > rank) {
-		if (variables[rank].hasNullTypeAnnotations()) {
-			if (NullAnnotationMatching.validNullTagBits(this.resolvedType.tagBits) != NullAnnotationMatching.validNullTagBits(variables[rank].tagBits)) {
-				scope.problemReporter().nullityMismatchTypeArgument(variables[rank], this.resolvedType, this);
-			}
+		TypeBinding variable = variables[rank];
+		if (variable.hasNullTypeAnnotations()) {
+			if (NullAnnotationMatching.analyse(variable, this.resolvedType, null, -1, CheckMode.BOUND_CHECK).isAnyMismatch())
+				scope.problemReporter().nullityMismatchTypeArgument(variable, this.resolvedType, this);
     	}
 	}
 }