Fix intersection of wildcards with extends bounds

checker/tests/nullness/generics/WildcardBounds.java

@@ -0,0 +1,134 @@
+import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+class WildcardBounds {
+
+    abstract class OuterNbl<T extends @Nullable Object> {
+        abstract T get();
+
+        abstract class Inner<U extends T> {
+            abstract U get();
+
+            abstract class Chain<V extends U, W extends V> {
+                abstract W get();
+            }
+
+            Object m0(Chain<? extends Object, ? extends Object> p) {
+                return p.get();
+            }
+
+            Object m1(Chain<? extends @NonNull T, ? extends @NonNull T> p) {
+                return p.get();
+            }
+
+            Object m2(Chain<?, ?> p) {
+                // :: error: (return.type.incompatible)
+                return p.get();
+            }
+
+            Object m3(Chain<? extends @Nullable Object, ? extends @Nullable Object> p) {
+                // :: error: (return.type.incompatible)
+                return p.get();
+            }
+
+            Object m4(Chain<@NonNull ?, @NonNull ?> p) {
+                return p.get();
+            }
+
+            void callsNonNull(
+                    OuterNbl<Object>.Inner<Number> i,
+                    OuterNbl<Object>.Inner<Number>.Chain<Integer, Integer> n) {
+                i.m0(n);
+                i.m1(n);
+                i.m2(n);
+                i.m3(n);
+                i.m4(n);
+            }
+
+            void callsNullable(
+                    OuterNbl<@Nullable Object>.Inner<@Nullable Number> i,
+                    OuterNbl<@Nullable Object>.Inner<@Nullable Number>.Chain<
+                                    @Nullable Integer, @Nullable Integer>
+                            n) {
+                // :: error: (argument.type.incompatible)
+                i.m0(n);
+                // :: error: (argument.type.incompatible)
+                i.m1(n);
+                // OK
+                i.m2(n);
+                // OK
+                i.m3(n);
+                // :: error: (argument.type.incompatible)
+                i.m4(n);
+            }
+        }
+
+        Object m0(Inner<? extends Object> p) {
+            return p.get();
+        }
+
+        Object m1(Inner<? extends @NonNull T> p) {
+            return p.get();
+        }
+
+        Object m2(Inner<?> p) {
+            // :: error: (return.type.incompatible)
+            return p.get();
+        }
+
+        Object m3(Inner<? extends @Nullable Object> p) {
+            // :: error: (return.type.incompatible)
+            return p.get();
+        }
+
+        Object m4(Inner<@NonNull ?> p) {
+            return p.get();
+        }
+
+        // We could add calls for these methods.
+    }
+
+    Object m0(OuterNbl<? extends Object> p) {
+        return p.get();
+    }
+
+    Object m1(OuterNbl<? extends @NonNull Object> p) {
+        return p.get();
+    }
+
+    Object m2(OuterNbl<?> p) {
+        // :: error: (return.type.incompatible)
+        return p.get();
+    }
+
+    Object m3(OuterNbl<? extends @Nullable Object> p) {
+        // :: error: (return.type.incompatible)
+        return p.get();
+    }
+
+    Object m4(OuterNbl<@NonNull ?> p) {
+        return p.get();
+    }
+
+    void callsOuter(OuterNbl<String> s, OuterNbl<@Nullable String> ns) {
+        m0(s);
+        m1(s);
+        m2(s);
+        m3(s);
+        m4(s);
+
+        // :: error: (argument.type.incompatible)
+        m0(ns);
+        // :: error: (argument.type.incompatible)
+        m1(ns);
+        // OK
+        m2(ns);
+        // OK
+        m3(ns);
+        // :: error: (argument.type.incompatible)
+        m4(ns);
+    }
+
+    // We could add an OuterNonNull to also test with a non-null upper bound.
+    // But we probably already test that enough.
+}

checker/tests/nullness/generics/WildcardOverrideMore.java

@@ -0,0 +1,48 @@
+import org.checkerframework.checker.nullness.qual.NonNull;
+import org.checkerframework.checker.nullness.qual.Nullable;
+
+class WildcardOverrideMore {
+    interface Box<X extends @Nullable Object> {}
+
+    interface Super<T extends @Nullable Object> {
+        <U extends T> void foo(Box<? extends U> lib);
+
+        <U extends T> Box<? extends U> retfoo();
+
+        <U extends T> void bar(Box<? super U> lib);
+
+        <U extends T> Box<? super U> retbar();
+    }
+
+    interface Sub<V extends @Nullable Object> extends Super<V> {
+        @Override
+        <W extends V> void foo(Box<? extends W> lib);
+
+        @Override
+        <W extends V> Box<? extends W> retfoo();
+
+        @Override
+        <W extends V> void bar(Box<? super W> lib);
+
+        @Override
+        <W extends V> Box<? super W> retbar();
+    }
+
+    interface SubErrors<V extends @Nullable Object> extends Super<V> {
+        @Override
+        // :: error: (override.param.invalid)
+        <W extends V> void foo(Box<? extends @NonNull W> lib);
+
+        @Override
+        // :: error: (override.return.invalid)
+        <W extends V> Box<? extends @Nullable W> retfoo();
+
+        @Override
+        // :: error: (override.param.invalid)
+        <W extends V> void bar(Box<@NonNull ? super @NonNull W> lib);
+
+        @Override
+        // :: error: (override.return.invalid)
+        <W extends V> Box<@Nullable ? super @NonNull W> retbar();
+    }
+}

framework/src/main/java/org/checkerframework/framework/type/DefaultTypeHierarchy.java

@@ -82,7 +82,7 @@ public class DefaultTypeHierarchy extends AbstractAtmComboVisitor<Boolean, Void>
     protected final StructuralEqualityVisitHistory areEqualVisitHistory;
 
     /** The Covariant.value field/element. */
-    final ExecutableElement covariantValueElement;
+    protected final ExecutableElement covariantValueElement;
 
     /**
      * Creates a DefaultTypeHierarchy.
@@ -392,8 +392,8 @@ protected boolean isContainedBy(
             areEqualVisitHistory.put(inside, outside, currentTop, result);
             return result;
         }
-        if ((TypesUtils.isCapturedTypeVariable(outside.getUnderlyingType())
-                && !TypesUtils.isCapturedTypeVariable(inside.getUnderlyingType()))) {
+        if (TypesUtils.isCapturedTypeVariable(outside.getUnderlyingType())
+                && !TypesUtils.isCapturedTypeVariable(inside.getUnderlyingType())) {
             // TODO: This branch should be removed after #979 is fixed.
             // This workaround is only needed when outside is a captured type variable,
             // but inside is not.
@@ -977,33 +977,58 @@ public Boolean visitTypevar_Typevar(
         TypeMirror superTM = supertype.getUnderlyingType();
         if (AnnotatedTypes.haveSameDeclaration(checker.getTypeUtils(), subtype, supertype)) {
             // The underlying types of subtype and supertype are uses of the same type parameter,
-            // but they
-            // may have different primary annotations.
-            boolean subtypeHasAnno = subtype.getAnnotationInHierarchy(currentTop) != null;
-            boolean supertypeHasAnno = supertype.getAnnotationInHierarchy(currentTop) != null;
+            // but they may have different primary annotations.
+            AnnotationMirror subtypeAnno = subtype.getAnnotationInHierarchy(currentTop);
+            boolean subtypeHasAnno = subtypeAnno != null;
+            AnnotationMirror supertypeAnno = supertype.getAnnotationInHierarchy(currentTop);
+            boolean supertypeHasAnno = supertypeAnno != null;
 
             if (subtypeHasAnno && supertypeHasAnno) {
                 // If both have primary annotations then just check the primary annotations
                 // as the bounds are the same.
                 return isPrimarySubtype(subtype, supertype);
             } else if (!subtypeHasAnno && !supertypeHasAnno) {
-                // two unannotated uses of the same type parameter are of the same type
-                return areEqualInHierarchy(subtype, supertype);
+                // Two unannotated uses of the same type parameter need to compare
+                // both upper and lower bounds.
+
+                // Upper bound of the subtype needs to be below the upper bound of the supertype.
+                if (!qualHierarchy.isSubtypeShallow(
+                        subtype.getEffectiveAnnotationInHierarchy(currentTop),
+                        subTM,
+                        supertype.getEffectiveAnnotationInHierarchy(currentTop),
+                        superTM)) {
+                    return false;
+                }
+
+                // Lower bound of the subtype needs to be below the lower bound of the supertype.
+                // TODO: Think through this and add better test coverage.
+                AnnotationMirrorSet subLBs =
+                        AnnotatedTypes.findEffectiveLowerBoundAnnotations(qualHierarchy, subtype);
+                AnnotationMirror subLB =
+                        qualHierarchy.findAnnotationInHierarchy(subLBs, currentTop);
+                AnnotationMirrorSet superLBs =
+                        AnnotatedTypes.findEffectiveLowerBoundAnnotations(qualHierarchy, supertype);
+                AnnotationMirror superLB =
+                        qualHierarchy.findAnnotationInHierarchy(superLBs, currentTop);
+                return qualHierarchy.isSubtypeShallow(subLB, subTM, superLB, superTM);
             } else if (subtypeHasAnno && !supertypeHasAnno) {
                 // This is the case "@A T <: T" where T is a type variable.
+                // TODO: should this also test the upper bounds?
                 AnnotationMirrorSet superLBs =
                         AnnotatedTypes.findEffectiveLowerBoundAnnotations(qualHierarchy, supertype);
                 AnnotationMirror superLB =
                         qualHierarchy.findAnnotationInHierarchy(superLBs, currentTop);
-                return qualHierarchy.isSubtypeShallow(
-                        subtype.getAnnotationInHierarchy(currentTop), subTM, superLB, superTM);
+                return qualHierarchy.isSubtypeShallow(subtypeAnno, subTM, superLB, superTM);
             } else if (!subtypeHasAnno && supertypeHasAnno) {
                 // This is the case "T <: @A T" where T is a type variable.
+                // TODO: should this also test the lower bounds?
                 return qualHierarchy.isSubtypeShallow(
                         subtype.getEffectiveAnnotationInHierarchy(currentTop),
                         subTM,
-                        supertype.getAnnotationInHierarchy(currentTop),
+                        supertypeAnno,
                         superTM);
+            } else {
+                throw new BugInCF("Unreachable");
             }
         }
 

framework/src/main/java/org/checkerframework/framework/util/AnnotatedTypes.java

@@ -955,13 +955,17 @@ private static AnnotatedTypeMirror glbSubtype(
                 if (subtype.getKind() != TypeKind.TYPEVAR) {
                     throw new BugInCF("Missing primary annotations: subtype: %s", subtype);
                 }
-                AnnotationMirrorSet lb = findEffectiveLowerBoundAnnotations(qualHierarchy, subtype);
-                AnnotationMirror lbAnno = qualHierarchy.findAnnotationInHierarchy(lb, top);
-                if (lbAnno != null
-                        && !qualHierarchy.isSubtypeShallow(lbAnno, subTM, superAnno, superTM)) {
-                    // The superAnno is lower than the lower bound annotation, so add it.
-                    glb.addAnnotation(superAnno);
-                } // else don't add any annotation.
+                AnnotationMirror ubAnno = subtype.getEffectiveAnnotationInHierarchy(top);
+                if (!qualHierarchy.isSubtypeQualifiersOnly(ubAnno, superAnno)) {
+                    // Instead of superAnno <: ubAnno check for ubAnno <!: superAnno to exclude the
+                    // case where ubAnno == superAnno.
+                    // We know that `glb` is a type variable, because `subtype` is.
+                    // Do not add the annotation to the type variable itself, because that would
+                    // change the upper and the lower bound.
+                    // Adding the more restrictive `superAnno` only to the upper bound ensures that
+                    // the type variable is below `superAnno`.
+                    ((AnnotatedTypeVariable) glb).getUpperBound().replaceAnnotation(superAnno);
+                }
             } else {
                 throw new BugInCF("GLB: subtype: %s, supertype: %s", subtype, supertype);
             }

framework/tests/all-systems/WildcardInReturn.java

@@ -0,0 +1,25 @@
+// Minimized test case from
+//
+// https://github.com/eisop/guava/blob/290cfe5c926de28cfdda491535901b09bab90ef9/guava/src/com/google/common/reflect/TypeToken.java#L1228
+// which failed in https://github.com/eisop/checker-framework/pull/1066 with:
+//
+// guava/guava/src/com/google/common/reflect/TypeToken.java:[1228,29] error: [[value,
+// allcheckers]:return.type.incompatible] incompatible types in return.
+//   type of expression: @UnknownVal TypeToken<capture#07[ extends capture#08[ extends T[ extends
+// @UnknownVal Object super @UnknownVal Void] super @BottomVal Void] super @BottomVal Void]>
+//   method return type: @UnknownVal TypeToken<?[ extends T[ extends @UnknownVal Object super
+// @BottomVal Void] super @BottomVal Void]>
+
+abstract class WildcardInReturn<T> {
+
+    abstract WildcardInReturn<?> of(String key);
+
+    abstract WildcardInReturn<? extends T> getSubtype(Class<?> subclass);
+
+    WildcardInReturn<? extends T> getSubtypeFromLowerBounds(Class<?> subclass, String key) {
+        @SuppressWarnings("unchecked") // T's lower bound is <? extends T>
+        WildcardInReturn<? extends T> bound = (WildcardInReturn<? extends T>) of(key);
+        // Java supports only one lowerbound anyway.
+        return bound.getSubtype(subclass);
+    }
+}