-
Notifications
You must be signed in to change notification settings - Fork 437
A wildcard with an implicit upper bound applies its own defaulting rules to its corresponding type parameter's bound #3845
Description
$ cat DefaultClash.java
import org.checkerframework.checker.nullness.qual.Nullable;
import org.checkerframework.framework.qual.DefaultQualifier;
import org.checkerframework.framework.qual.TypeUseLocation;
class Holder<T> {
final T value;
Holder(T value) {
this.value = value;
}
}
interface HolderSupplier<H extends Holder<Object>> {
H get();
}
@DefaultQualifier(value = Nullable.class, locations = TypeUseLocation.ALL)
class DefaultClash {
Object go(HolderSupplier<?> s) {
if (s != null) {
return s.get();
}
return "";
}
}checker-framework-3.7.0 $ checker/bin/javac -processor org.checkerframework.checker.nullness.NullnessChecker DefaultClash.java
DefaultClash.java:19: error: [type.argument.type.incompatible] incompatible type argument for type parameter H of HolderSupplier.
Object go(HolderSupplier<?> s) {
^
found : ? extends @Initialized @NonNull Holder<@Initialized @Nullable Object>
required: @Initialized @NonNull Holder<@Initialized @NonNull Object>
1 error
At a high level, I would expect for HolderSupplier<?> to always be valid, with CF filling in an appropriate value for the implicit upper bound.
Drilling in a little more: If I'm reading the code right, the @Nullable Object argument to Holder comes from the default that's in effect at the use site -- that is, where HolderSupplier<?> is written. I would instead expect for it to use the default of @NonNull Object, since that's the default in effect where Holder<Object> appears in the source code.
Among the code involved is here:
checker-framework/framework/src/main/java/org/checkerframework/framework/type/BoundsInitializer.java
Lines 584 to 591 in 88f1dd2
| } else if (wildcard.getTypeVariable() != null) { | |
| // Otherwise use the upper bound of the type variable associated with this wildcard. | |
| javaExtendsBound = wildcard.getTypeVariable().getUpperBound(); | |
| } else { | |
| // Otherwise use the upper bound of the java wildcard. | |
| javaExtendsBound = | |
| TypesUtils.wildUpperBound( | |
| javaWildcardType, wildcard.atypeFactory.processingEnv); |
That gives us a TypeMirror, which is then converted to an unannotated AnnotatedTypeMirror to be filled in later by the defaults in effect at the wildcard site:
checker-framework/framework/src/main/java/org/checkerframework/framework/type/BoundsInitializer.java
Line 599 in 88f1dd2
| AnnotatedTypeMirror.createType(javaExtendsBound, typeFactory, false); |
It seems like what we'd want for the type-parameter bound is to create the AnnotatedTypeMirror based on the defaults in effect at the type-parameter declaration.
We do have access at this point to the AnnotatedTypeFactory, so perhaps we could call getAnnotatedType on wildcard.getTypeVariable().asElement() (and then extract getUpperBound()). (When getTypeVariable() is not available, we could probably get what we need from wildcardToTypeParam((com.sun.tools.javac.code.Type.WildcardType) wildcard.getUnderlyingType()).) That would (I think) apply appropriate defaults. But I worry that that would be approaching the problem at the wrong "level." In particular, getAnnotatedType could call back into BoundsInitializer (I think?), and I wonder if that could produce infinite loops with recursive generics like Comparable<E extends Comparable<? super E>> (or otherwise violate assumptions).
The good news is that the example above is just an unnecessary warning, rather than an unsoundness. But it seems likely that it could be turned into unsoundness. And I did actually hit this one in some actual code, unlike half of my reports so far :)
(One other bad idea I considered: Setting the bound not to the type parameter's bounds but rather to a usage of the corresponding type variable. For example, for class Foo<N extends Number> {} Foo<?> foo;, I could treat Foo<?> as Foo<? extends N>. But this is wrong because later logic would sometimes substitute in specific values for N, which would be incorrect.)