diff --git a/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java b/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java index fa25f35..45e6cdc 100644 --- a/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java +++ b/src/main/java/com/google/jspecify/nullness/NullSpecAnnotatedTypeFactory.java @@ -25,8 +25,10 @@ import static java.util.Collections.singletonList; import static java.util.Collections.unmodifiableList; import static javax.lang.model.element.ElementKind.ENUM_CONSTANT; +import static javax.lang.model.element.ElementKind.TYPE_PARAMETER; import static javax.lang.model.type.TypeKind.ARRAY; import static javax.lang.model.type.TypeKind.DECLARED; +import static javax.lang.model.type.TypeKind.INTERSECTION; import static javax.lang.model.type.TypeKind.NULL; import static javax.lang.model.type.TypeKind.TYPEVAR; import static javax.lang.model.type.TypeKind.WILDCARD; @@ -68,7 +70,11 @@ import javax.lang.model.element.ExecutableElement; import javax.lang.model.element.Name; import javax.lang.model.element.TypeElement; +import javax.lang.model.element.TypeParameterElement; +import javax.lang.model.type.DeclaredType; +import javax.lang.model.type.IntersectionType; import javax.lang.model.type.TypeMirror; +import javax.lang.model.type.TypeVariable; import javax.lang.model.util.Elements; import javax.lang.model.util.Types; import org.checkerframework.common.basetype.BaseTypeChecker; @@ -677,14 +683,49 @@ && isNullnessSubtype(subtype, ((AnnotatedTypeVariable) supertype).getLowerBound( && isSubtype(superTV.getLowerBound(), subTV.getLowerBound()); } } - return isNullInclusiveUnderEveryParameterization(supertype) - || isNullExclusiveUnderEveryParameterization(subtype) - || (nullnessEstablishingPathExists(subtype, supertype) - && !supertype.hasAnnotation(minusNull)); + boolean nullInclSupertype = isNullInclusiveUnderEveryParameterization(supertype); + boolean nullExclSubtype = isNullExclusiveUnderEveryParameterization(subtype); + boolean pathExists = nullnessEstablishingPathExists(subtype, supertype); + boolean supertypeNotMinusNull = !supertype.hasAnnotation(minusNull); + return nullInclSupertype || nullExclSubtype || (pathExists && supertypeNotMinusNull); } } boolean isNullInclusiveUnderEveryParameterization(AnnotatedTypeMirror type) { + /* + * For declared (non-captured) type variables with multiple bounds, use the + * source-declared bounds from TypeParameterElement.getBounds(). This avoids + * two pitfalls with the generic TYPEVAR lower-bound check below: + * + * 1. javac implicitly inserts java.lang.Object as the first bound for + * interface-only intersections (e.g., {@code T extends @Nullable A & @Nullable B}). + * The source-level element bounds don't include this implicit Object. + * + * 2. CF's copyIntersectionBoundAnnotations propagates @Nullable from any nullable + * bound to ALL bounds, so CF-annotated bounds are unreliable for mixed-nullness + * intersections (e.g., {@code T extends @Nullable Object & Lib}). + * + * Using TypeParameterElement.getBounds() gives us only the programmer-written bounds + * with their original source annotations, free of both issues. + */ + if (type.getKind() == TYPEVAR + && !type.hasAnnotation(minusNull) + && !isCapturedTypeVariable(type.getUnderlyingType())) { + Element element = ((TypeVariable) type.getUnderlyingType()).asElement(); + if (element.getKind() == TYPE_PARAMETER) { + List sourceBounds = ((TypeParameterElement) element).getBounds(); + if (sourceBounds.size() > 1) { + // Multi-bound: all source-declared bounds must be explicitly @Nullable for T to be + // null-inclusive under every parameterization. + for (TypeMirror sourceBound : sourceBounds) { + if (!hasExplicitNullableAnnotation(sourceBound)) { + return false; + } + } + return true; + } + } + } // We put the third case from the spec first because it's a mouthful. // (As discussed in the spec, we probably don't strictly need this case at all....) if (type.getKind() == TYPEVAR @@ -700,15 +741,36 @@ && isNullInclusiveUnderEveryParameterization( * type have an AnnotationMirror of its own. * * ...well, sort of. As I understand it, what CF does is more that it tries to keep the - * AnnotationMirror of the intersecton type in sync with the AnnotationMirror of each of its + * AnnotationMirror of the intersection type in sync with the AnnotationMirror of each of its * components (which should themselves all match). So the intersection type "has" an * AnnotationMirror, but it provides no *additional* information beyond what is already carried * by its components' AnnotationMirrors. * - * Nevertheless, the result is that we don't need a special case here: The check below is - * redundant with the subsequent check on the intersection's components, but redundancy is - * harmless. + * When components all agree, checking the intersection type's annotation is redundant with + * checking the components, and the redundancy is harmless. However, when bounds have *mixed* + * nullness (e.g., {@code @Nullable A & B}), CF synthesizes an annotation for the intersection + * by copying the annotation of the "widest" (most-nullable) bound to ALL bounds. For example, + * for {@code @Nullable A & B}, CF annotates both the intersection and B as @Nullable, even + * though B itself is non-null. We must not rely on these synthesized CF annotations; instead, + * we check the original source annotations on each bound via the underlying IntersectionType. + * This gives the JSpecify-correct semantics: a value of {@code @Nullable A & B} cannot be null + * if B is non-null (regardless of what CF puts on B after propagation). + * + *

Note: javac implicitly adds {@code java.lang.Object} as the first bound of any + * interface-only intersection type. This implicit Object bound carries no source-level nullness + * information and must be skipped; see {@link #isUnannotatedObjectBound(TypeMirror)}. */ + if (type.getKind() == INTERSECTION) { + for (TypeMirror rawBound : ((IntersectionType) type.getUnderlyingType()).getBounds()) { + if (isUnannotatedObjectBound(rawBound)) { + continue; + } + if (!hasExplicitNullableAnnotation(rawBound)) { + return false; + } + } + return true; + } return type.hasAnnotation(unionNull) || (!isLeastConvenientWorld && type.hasAnnotation(nullnessOperatorUnspecified)); } @@ -767,10 +829,70 @@ private boolean nullnessEstablishingPathExists( return true; } + /* + * For intersection types (e.g., {@code @Nullable A & B}), CF synthesizes an annotation for + * the intersection type by copying the annotation of the "widest" (most-nullable) bound to ALL + * bounds. For {@code @Nullable A & B} in NullMarked code, CF annotates both the intersection + * and B as @Nullable, even though B is non-null. We must not rely on these synthesized CF + * annotations. Instead, we check the original source annotations on each bound via the + * underlying IntersectionType. If any bound is not explicitly @Nullable in the source (and + * therefore non-null by default in NullMarked code), that bound establishes non-nullness. + * + *

Note: javac implicitly adds {@code java.lang.Object} as the first bound of any + * interface-only intersection type. This implicit Object bound carries no source-level nullness + * information and must be skipped; see {@link #isUnannotatedObjectBound(TypeMirror)}. + */ + if (subtype.getKind() == INTERSECTION) { + for (TypeMirror rawBound : ((IntersectionType) subtype.getUnderlyingType()).getBounds()) { + if (isUnannotatedObjectBound(rawBound)) { + continue; + } + if (!hasExplicitNullableAnnotation(rawBound) && supertypeMatcher.test(rawBound)) { + return true; + } + } + return false; + } + if (isUnionNullOrEquivalent(subtype)) { return false; } + /* + * For declared (non-captured) type variables with multiple bounds, use the + * source-declared bounds from TypeParameterElement.getBounds(). This avoids two pitfalls + * with going through getUpperBounds() + the INTERSECTION check above: + * + * 1. javac implicitly inserts java.lang.Object as the first bound for interface-only + * intersections (e.g., {@code T extends @Nullable A & @Nullable B}). The isUnannotatedObjectBound + * check in the INTERSECTION block above would skip this implicit Object, but it also + * skips an *explicit* unannotated Object (e.g., {@code T extends Object & @Nullable Lib}), + * which should be treated as a non-null bound in NullMarked code. + * + * 2. CF's copyIntersectionBoundAnnotations propagates @Nullable from any bound to ALL + * bounds, so the CF-annotated bounds passed to getUpperBounds() are unreliable for + * mixed-nullness intersections. + * + * TypeParameterElement.getBounds() returns only the programmer-written bounds with their + * original source annotations, free of both issues. If any source-declared bound is not + * explicitly @Nullable and matches the supertype, a non-null path is established. + */ + if (subtype.getKind() == TYPEVAR + && !isCapturedTypeVariable(subtype.getUnderlyingType())) { + Element element = ((TypeVariable) subtype.getUnderlyingType()).asElement(); + if (element.getKind() == TYPE_PARAMETER) { + List sourceBounds = ((TypeParameterElement) element).getBounds(); + if (sourceBounds.size() > 1) { + for (TypeMirror sourceBound : sourceBounds) { + if (!hasExplicitNullableAnnotation(sourceBound) && supertypeMatcher.test(sourceBound)) { + return true; + } + } + return false; + } + } + } + if (supertypeMatcher.test(subtype.getUnderlyingType())) { return true; } @@ -839,6 +961,55 @@ private boolean isUnionNullOrEquivalent(AnnotatedTypeMirror type) { || (isLeastConvenientWorld && type.hasAnnotation(nullnessOperatorUnspecified)); } + /** + * Returns true if the given raw {@link TypeMirror} (from an intersection type's underlying + * bounds) bears an explicit {@code @Nullable} annotation in the source code. + * + *

This checks the source-level type annotations on the {@link TypeMirror} directly, unlike + * CF's annotated types, which may have had nullness annotations propagated from other bounds in + * the same intersection type (see {@code AnnotatedIntersectionType.copyIntersectionBoundAnnotations}). + * + *

The raw {@link TypeMirror} carries the original source annotation (e.g., + * {@code @org.jspecify.annotations.Nullable}), not CF's internal alias. So we check both + * CF's internal {@code unionNull} annotation and all recognized nullable annotation names. + */ + private boolean hasExplicitNullableAnnotation(TypeMirror rawType) { + for (AnnotationMirror am : rawType.getAnnotationMirrors()) { + if (areSame(am, unionNull)) { + return true; + } + String qualifiedName = + ((TypeElement) am.getAnnotationType().asElement()).getQualifiedName().toString(); + if (NULLABLE_ANNOTATIONS.contains(qualifiedName)) { + return true; + } + } + return false; + } + + /** + * Returns true if the given raw {@link TypeMirror} is an unannotated {@code java.lang.Object} + * bound that should be skipped when evaluating the nullness of an intersection type. + * + *

When a type variable has only interface bounds (e.g., {@code T extends A & B}), javac + * implicitly inserts {@code java.lang.Object} as the first element of the + * {@link IntersectionType}'s bound list. This implicit bound carries no source-level nullness + * intent, so it must be excluded from the checks in {@link #isNullInclusiveUnderEveryParameterization} + * and {@link #nullnessEstablishingPathExists}. Without this exclusion, an unannotated Object + * bound would be mistakenly treated as a non-null bound, causing {@code T extends @Nullable A & + * @Nullable B} to be considered non-null (incorrect). + */ + private boolean isUnannotatedObjectBound(TypeMirror rawType) { + if (rawType.getKind() != DECLARED) { + return false; + } + if (!rawType.getAnnotationMirrors().isEmpty()) { + return false; + } + TypeElement typeElement = (TypeElement) ((DeclaredType) rawType).asElement(); + return typeElement.getQualifiedName().contentEquals("java.lang.Object"); + } + private final class NullSpecEqualityComparer extends StructuralEqualityComparer { NullSpecEqualityComparer(StructuralEqualityVisitHistory typeargVisitHistory) { super(typeargVisitHistory); diff --git a/tests/regression/CaptureTest.java b/tests/regression/CaptureTest.java new file mode 100644 index 0000000..3f182e6 --- /dev/null +++ b/tests/regression/CaptureTest.java @@ -0,0 +1,53 @@ +// Copyright 2025 The JSpecify Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Tests for captured wildcard nullness handling. +// In NullMarked code, a captured wildcard is nullable iff ALL its effective upper bounds are +// nullable. The checker should detect when a nullable captured value is returned as non-null. +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class CaptureTest { + // T extends @Nullable Foo, wildcard is unbounded (?). + // In NullMarked, ? defaults to @Nullable, so F_cap has nullable upper bound -> error. + Foo nullableUnbounded(NullableFooSupplier supplier) { + // :: error: return.type.incompatible + return supplier.get(); + } + + // T extends @Nullable Object, wildcard is ? extends @Nullable Lib. + // ALL upper bounds are nullable (@Nullable Lib, @Nullable Object) -> T_cap is nullable -> error. + Object nullableExplicitBound(NullableBounded x) { + // :: error: return.type.incompatible + return x.get(); + } + + // T extends @Nullable Object, wildcard is ? extends Lib (non-null). + // Lib is non-null -> T_cap has non-null bound -> no error. + Object nonNullExplicitBound(NullableBounded x) { + return x.get(); // should NOT be an error + } + + interface NullableFooSupplier { + F get(); + } + + interface NullableBounded { + T get(); + } + + interface Foo {} + interface Lib {} +} diff --git a/tests/regression/MultiBoundSampleLike.java b/tests/regression/MultiBoundSampleLike.java new file mode 100644 index 0000000..a8db7cd --- /dev/null +++ b/tests/regression/MultiBoundSampleLike.java @@ -0,0 +1,59 @@ +// Copyright 2025 The JSpecify Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// Tests mimicking jspecify's MultiBoundTypeVariableToObject.java and +// MultiBoundTypeVariableToOther.java samples. These cover cases with explicit +// java.lang.Object as the first intersection bound alongside @Nullable bounds. +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +@NullMarked +class MultiBoundSampleLike { + // x0: T extends Object & Lib -> both non-null -> no error + Object x0(T x) { + return x; + } + + // x2: T extends Object & @Nullable Lib -> Object is non-null -> no error + Object x2(T x) { + return x; + } + + // x6: T extends @Nullable Object & Lib -> Lib is non-null -> no error + Object x6(T x) { + return x; + } + + // x8: T extends @Nullable Object & @Nullable Lib -> both nullable -> error + Object x8(T x) { + // :: error: jspecify_nullness_mismatch + return x; + } + + // Same tests but returning as Lib (non-Object supertype) + Lib x2ToOther(T x) { + return x; + } + + Lib x6ToOther(T x) { + return x; + } + + Lib x8ToOther(T x) { + // :: error: jspecify_nullness_mismatch + return x; + } + + interface Lib {} +} diff --git a/tests/regression/MultiBoundTest.java b/tests/regression/MultiBoundTest.java new file mode 100644 index 0000000..7c6cded --- /dev/null +++ b/tests/regression/MultiBoundTest.java @@ -0,0 +1,49 @@ +// Copyright 2025 The JSpecify Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import java.io.Serializable; +import java.io.Closeable; +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +// Tests for multi-bound type variable handling. +// In NullMarked context, Serializable and Closeable are non-null by default. +// T extends @Nullable Serializable & Closeable: Closeable is non-null so T must be non-null. +@NullMarked +class MultiBoundTest { + // T extends @Nullable Serializable & Closeable: Closeable (non-null) establishes T as non-null. + // Returning T as Object! (non-null) should be FINE - no error. + Object nullableFirstNonNullSecond(T t) { + return t; // should NOT be an error + } + + // T extends Closeable & @Nullable Serializable: Closeable (non-null) establishes T as non-null. + // Returning T as Object! should be FINE - no error. + Object nonNullFirstNullableSecond(T t) { + return t; // should NOT be an error + } + + // T extends @Nullable Serializable & @Nullable Closeable: both nullable - T can be null. + // Returning T as Object! should be an error. + Object bothNullable(T t) { + // :: error: jspecify_nullness_mismatch + return t; + } + + // T extends Serializable & Closeable: both non-null (default in NullMarked). + // Returning T as Object! should be FINE - no error. + Object bothNonNull(T t) { + return t; // should NOT be an error + } +}