Skip to content

Commit

Permalink
[null] Incomplete handling of @NonNullByDefault - fixes #682
Browse files Browse the repository at this point in the history 
+ applying @NNBD to a type parameter needs to know, whether on demand
  resolving has already happened (to detect explicit null annotations)
  - ASTNode.resolveAnnotations
  - STB.maybeMarkTypeParametersNonNull
+ better distinction if null tag bits result from @NNBD or explicit anno
  - ASTNode.mergeAnnotationsIntoType
    - may now report redundance
    - fix withdrawal of contradiction null annots after these changes
+ more detection of redundance below @NNBD
  - TypeParameter.resolveAnnotations
  - TypeReference.resolveAnnotations - one focus on annots on dimensions
  - MethodBinding.fillInDefaultNonNullness18 - less reporting here
  - ProblemReporter.nullAnnotationIsRedundant
    - 2 more overloads
    - fine tuning of error positions
+ be careful not to apply @NNBD to typevar (when using TYPE_USE)
  - new methods {Scope,STB}.hasDefaultNullnessForType()
+ expect redundance warning in may more tests
  - NullTypeAnnotationTest
  - NullAnnotationBatchCompilerTest
+ ignore newly reported warnings in other tests:
  - IncrementalTests18.testBug483744_remove
  - ExternalAnnotations18Test (several)
+ new tests from Bugs 522142 and 499596
  • Loading branch information
@stephan-herrmann
stephan-herrmann committed Feb 1, 2023
1 parent 04d7a00 commit 422a9b5
Show file tree
Hide file tree
Showing 17 changed files with 522 additions and 79 deletions.
57 changes: 39 additions & 18 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ASTNode.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@

import org.eclipse.jdt.core.compiler.CharOperation;
import org.eclipse.jdt.internal.compiler.ASTVisitor;
import org.eclipse.jdt.internal.compiler.ast.TypeReference.AnnotationPosition;
import org.eclipse.jdt.internal.compiler.classfmt.ClassFileConstants;
import org.eclipse.jdt.internal.compiler.env.AccessRestriction;
import org.eclipse.jdt.internal.compiler.lookup.AnnotationBinding;
Expand Down Expand Up @@ -851,8 +852,11 @@ public static void resolveAnnotations(BlockScope scope, Annotation[] sourceAnnot
}
break;
case Binding.TYPE_PARAMETER :
((TypeVariableBinding) recipient).tagBits |= TagBits.AnnotationResolved;
//$FALL-THROUGH$
case Binding.TYPE_USE :
// deliberately don't set the annotation resolved tagbits, it is not material and also we are working with a dummy static object.
// for TYPE_USE we deliberately don't set the annotation resolved tagbits,
// it is not material and also we are working with a dummy static object.
annotations = new AnnotationBinding[length];
break;
case Binding.MODULE:
Expand Down Expand Up @@ -1157,7 +1161,8 @@ public static void copySE8AnnotationsToType(BlockScope scope, Binding recipient,
if (Annotation.isTypeUseCompatible(typeRef, scope)) { // discard hybrid annotations on name qualified types.
local.declaration.bits |= HasTypeAnnotations;
typeRef.bits |= HasTypeAnnotations;
local.type = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation, typeRef, local.type);
int location = local.isParameter() ? Binding.DefaultLocationParameter : 0 /*no default for locals*/;
local.type = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation, typeRef, location, local.type);
if(scope.environment().usesNullTypeAnnotations()) {
local.tagBits &= ~(se8nullBits);
}
Expand All @@ -1170,7 +1175,8 @@ public static void copySE8AnnotationsToType(BlockScope scope, Binding recipient,
if (Annotation.isTypeUseCompatible(fieldDeclaration.type, scope)) { // discard hybrid annotations on name qualified types.
fieldDeclaration.bits |= HasTypeAnnotations;
fieldDeclaration.type.bits |= HasTypeAnnotations;
field.type = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation, fieldDeclaration.type, field.type);
field.type = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation,
fieldDeclaration.type, Binding.DefaultLocationField, field.type);
if(scope.environment().usesNullTypeAnnotations()) {
field.tagBits &= ~(se8nullBits);
}
Expand All @@ -1182,7 +1188,8 @@ public static void copySE8AnnotationsToType(BlockScope scope, Binding recipient,
if (Annotation.isTypeUseCompatible(recordComponent.type, scope)) { // discard hybrid annotations on name qualified types.
recordComponent.bits |= HasTypeAnnotations;
recordComponent.type.bits |= HasTypeAnnotations;
recordComponentBinding.type = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation, recordComponent.type, recordComponentBinding.type);
recordComponentBinding.type = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation, recordComponent.type,
Binding.DefaultLocationField, recordComponentBinding.type);
if(scope.environment().usesNullTypeAnnotations()) { //TODO Bug 562478
recordComponentBinding.tagBits &= ~(se8nullBits);
}
Expand All @@ -1196,7 +1203,8 @@ public static void copySE8AnnotationsToType(BlockScope scope, Binding recipient,
if (Annotation.isTypeUseCompatible(methodDecl.returnType, scope)) {
methodDecl.bits |= HasTypeAnnotations;
methodDecl.returnType.bits |= HasTypeAnnotations;
method.returnType = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation, methodDecl.returnType, method.returnType);
method.returnType = mergeAnnotationsIntoType(scope, se8Annotations, se8nullBits, se8NullAnnotation,
methodDecl.returnType, Binding.DefaultLocationReturnType, method.returnType);
if(scope.environment().usesNullTypeAnnotations()) {
method.tagBits &= ~(se8nullBits);
}
Expand Down Expand Up @@ -1278,7 +1286,7 @@ public static Annotation[] copyRecordComponentAnnotations(Scope scope, Binding r
}

private static TypeBinding mergeAnnotationsIntoType(BlockScope scope, AnnotationBinding[] se8Annotations, long se8nullBits, Annotation se8NullAnnotation,
TypeReference typeRef, TypeBinding existingType)
TypeReference typeRef, int location, TypeBinding existingType)
{
if (existingType == null || !existingType.isValidBinding()) return existingType;
TypeReference unionRef = typeRef.isUnionType() ? ((UnionTypeReference) typeRef).typeReferences[0] : null;
Expand All @@ -1298,27 +1306,40 @@ private static TypeBinding mergeAnnotationsIntoType(BlockScope scope, Annotation
return existingType;
}

if (typeRef.dimensions() > 0) {
location = Binding.DefaultLocationArrayContents; // moving from SE7 to SE8 position applies to the array contents!
}

long prevNullBits = oldLeafType.tagBits & TagBits.AnnotationNullMASK;
if ((prevNullBits | se8nullBits) == TagBits.AnnotationNullMASK) { // contradiction after merge?
if ((prevNullBits | se8nullBits) == TagBits.AnnotationNullMASK && typeRef.hasNullTypeAnnotation(AnnotationPosition.MAIN_TYPE)) {
// would merging introduce a contradiction?
if (!(oldLeafType instanceof TypeVariableBinding)) { // let type-use annotations override annotations on the type parameter declaration
if (prevNullBits != TagBits.AnnotationNullMASK && se8nullBits != TagBits.AnnotationNullMASK) { // conflict caused by the merge?
scope.problemReporter().contradictoryNullAnnotations(se8NullAnnotation);
}
se8Annotations = Binding.NO_ANNOTATIONS;
se8nullBits = 0;
se8nullBits = TagBits.AnnotationNullMASK;
}
oldLeafType = oldLeafType.withoutToplevelNullAnnotation();
} else if (se8nullBits == TagBits.AnnotationNonNull
&& location != Binding.DefaultLocationReturnType // normal return type cases are handled in MethodBinding.fillInDefaultNonNullness18()
&& scope.hasDefaultNullnessForType(typeRef.resolvedType, location, typeRef.sourceStart)) {
scope.problemReporter().nullAnnotationIsRedundant(typeRef, new Annotation[] { se8NullAnnotation });
}
if (se8nullBits == TagBits.AnnotationNullMASK) {
// withdraw contradicting null annotations
se8Annotations = scope.environment().filterNullTypeAnnotations(se8Annotations);
}
if (se8Annotations.length > 0) {
AnnotationBinding [][] goodies = new AnnotationBinding[typeRef.getAnnotatableLevels()][];
goodies[0] = se8Annotations; // @T X.Y.Z local; ==> @T should annotate X
TypeBinding newLeafType = scope.environment().createAnnotatedType(oldLeafType, goodies);

AnnotationBinding [][] goodies = new AnnotationBinding[typeRef.getAnnotatableLevels()][];
goodies[0] = se8Annotations; // @T X.Y.Z local; ==> @T should annotate X
TypeBinding newLeafType = scope.environment().createAnnotatedType(oldLeafType, goodies);

if (unionRef == null) {
typeRef.resolvedType = existingType.isArrayType() ? scope.environment().createArrayType(newLeafType, existingType.dimensions(), existingType.getTypeAnnotations()) : newLeafType;
} else {
unionRef.resolvedType = newLeafType;
unionRef.bits |= HasTypeAnnotations;
if (unionRef == null) {
typeRef.resolvedType = existingType.isArrayType() ? scope.environment().createArrayType(newLeafType, existingType.dimensions(), existingType.getTypeAnnotations()) : newLeafType;
} else {
unionRef.resolvedType = newLeafType;
unionRef.bits |= HasTypeAnnotations;
}
}
return typeRef.resolvedType;
}
Expand Down
7 changes: 7 additions & 0 deletions ...compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ArrayQualifiedTypeReference.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ public void setAnnotationsOnDimensions(Annotation [][] annotationsOnDimensions)
this.annotationsOnDimensions = annotationsOnDimensions;
}

@Override
public Annotation[] getTopAnnotations() {
if (this.annotationsOnDimensions != null)
return this.annotationsOnDimensions[0];
return new Annotation[0];
}

/**
* @return char[][]
*/
Expand Down
8 changes: 8 additions & 0 deletions ...jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/ArrayTypeReference.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,14 @@ public Annotation[][] getAnnotationsOnDimensions(boolean useSourceOrder) {
public void setAnnotationsOnDimensions(Annotation [][] annotationsOnDimensions) {
this.annotationsOnDimensions = annotationsOnDimensions;
}

@Override
public Annotation[] getTopAnnotations() {
if (this.annotationsOnDimensions != null)
return this.annotationsOnDimensions[0];
return new Annotation[0];
}

/**
* @return char[][]
*/
Expand Down
37 changes: 21 additions & 16 deletions ...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeParameter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import org.eclipse.jdt.internal.compiler.lookup.MethodBinding;
import org.eclipse.jdt.internal.compiler.lookup.MethodScope;
import org.eclipse.jdt.internal.compiler.lookup.Scope;
import org.eclipse.jdt.internal.compiler.lookup.TagBits;
import org.eclipse.jdt.internal.compiler.lookup.TypeConstants;
import org.eclipse.jdt.internal.compiler.lookup.TypeVariableBinding;

Expand Down Expand Up @@ -144,22 +145,26 @@ public void resolveAnnotations(Scope scope) {
}
if (isAnnotationBasedNullAnalysisEnabled) {
if (this.binding != null && this.binding.isValidBinding()) {
if (!this.binding.hasNullTypeAnnotations()
&& scope.hasDefaultNullnessFor(Binding.DefaultLocationTypeParameter, this.sourceStart())) {
AnnotationBinding[] annots = new AnnotationBinding[] { environment.getNonNullAnnotation() };
TypeVariableBinding previousBinding = this.binding;
this.binding = (TypeVariableBinding) environment.createAnnotatedType(this.binding, annots);

if (scope instanceof MethodScope) {
/*
* for method type parameters, references to the bindings have already been copied into
* MethodBinding.typeVariables - update them.
*/
MethodScope methodScope = (MethodScope) scope;
if (methodScope.referenceContext instanceof AbstractMethodDeclaration) {
MethodBinding methodBinding = ((AbstractMethodDeclaration) methodScope.referenceContext).binding;
if (methodBinding != null) {
methodBinding.updateTypeVariableBinding(previousBinding, this.binding);
if (scope.hasDefaultNullnessFor(Binding.DefaultLocationTypeParameter, this.sourceStart())) {
if (this.binding.hasNullTypeAnnotations()) {
if ((this.binding.tagBits & TagBits.AnnotationNonNull) != 0)
scope.problemReporter().nullAnnotationIsRedundant(this);
} else { // no explicit type annos, add the default:
AnnotationBinding[] annots = new AnnotationBinding[] { environment.getNonNullAnnotation() };
TypeVariableBinding previousBinding = this.binding;
this.binding = (TypeVariableBinding) environment.createAnnotatedType(this.binding, annots);

if (scope instanceof MethodScope) {
/*
* for method type parameters, references to the bindings have already been copied into
* MethodBinding.typeVariables - update them.
*/
MethodScope methodScope = (MethodScope) scope;
if (methodScope.referenceContext instanceof AbstractMethodDeclaration) {
MethodBinding methodBinding = ((AbstractMethodDeclaration) methodScope.referenceContext).binding;
if (methodBinding != null) {
methodBinding.updateTypeVariableBinding(previousBinding, this.binding);
}
}
}
}
Expand Down
34 changes: 25 additions & 9 deletions ...ipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/ast/TypeReference.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -681,9 +681,13 @@ protected void resolveAnnotations(Scope scope, int location) {
long[] nullTagBitsPerDimension = ((ArrayBinding)this.resolvedType).nullTagBitsPerDimension;
if (nullTagBitsPerDimension != null) {
for (int i = 0; i < dimensions; i++) { // skip last annotations at [dimensions] (concerns the leaf type)
if ((nullTagBitsPerDimension[i] & TagBits.AnnotationNullMASK) == TagBits.AnnotationNullMASK) {
long nullTagBits = nullTagBitsPerDimension[i] & TagBits.AnnotationNullMASK;
if (nullTagBits == TagBits.AnnotationNullMASK) {
scope.problemReporter().contradictoryNullAnnotations(annotationsOnDimensions[i]);
nullTagBitsPerDimension[i] = 0;
} else if (nullTagBits == TagBits.AnnotationNonNull) {
if (scope.hasDefaultNullnessFor(Binding.DefaultLocationArrayContents, this.sourceStart))
scope.problemReporter().nullAnnotationIsRedundant(this, annotationsOnDimensions[i]);
}
}
}
Expand All @@ -693,21 +697,33 @@ protected void resolveAnnotations(Scope scope, int location) {
}
if (scope.compilerOptions().isAnnotationBasedNullAnalysisEnabled
&& this.resolvedType != null
&& (this.resolvedType.tagBits & TagBits.AnnotationNullMASK) == 0
&& !this.resolvedType.isTypeVariable()
&& !this.resolvedType.isWildcard()
&& location != 0
&& scope.hasDefaultNullnessFor(location, this.sourceStart))
&& scope.hasDefaultNullnessForType(this.resolvedType, location, this.sourceStart))
{
if (location == Binding.DefaultLocationTypeBound && this.resolvedType.id == TypeIds.T_JavaLangObject) {
scope.problemReporter().implicitObjectBoundNoNullDefault(this);
} else {
LookupEnvironment environment = scope.environment();
AnnotationBinding[] annots = new AnnotationBinding[]{environment.getNonNullAnnotation()};
this.resolvedType = environment.createAnnotatedType(this.resolvedType, annots);
long nullTagBits = this.resolvedType.tagBits & TagBits.AnnotationNullMASK;
if (nullTagBits == 0) {
if (location == Binding.DefaultLocationTypeBound && this.resolvedType.id == TypeIds.T_JavaLangObject) {
scope.problemReporter().implicitObjectBoundNoNullDefault(this);
} else {
LookupEnvironment environment = scope.environment();
AnnotationBinding[] annots = new AnnotationBinding[]{environment.getNonNullAnnotation()};
this.resolvedType = environment.createAnnotatedType(this.resolvedType, annots);
}
} else if (nullTagBits == TagBits.AnnotationNonNull) {
if (location != Binding.DefaultLocationParameter) { // parameters are handled in MethodBinding.fillInDefaultNonNullness18()
scope.problemReporter().nullAnnotationIsRedundant(this, getTopAnnotations());
}
}
}
}
public Annotation[] getTopAnnotations() {
if (this.annotations != null)
return this.annotations[getAnnotatableLevels()-1];
return new Annotation[0];
}

public int getAnnotatableLevels() {
return 1;
}
Expand Down
4 changes: 2 additions & 2 deletions ...t.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/BinaryTypeBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1987,7 +1987,7 @@ private void scanFieldForNullAnnotation(IBinaryField field, VariableBinding fiel
&& fieldType.acceptsNonNullDefault()) {
int nullDefaultFromField = getNullDefaultFrom(field.getAnnotations());
if (nullDefaultFromField == Binding.NO_NULL_DEFAULT
? hasNonNullDefaultFor(DefaultLocationField, -1)
? hasNonNullDefaultForType(fieldType, DefaultLocationField, -1)
: (nullDefaultFromField & DefaultLocationField) != 0) {
fieldBinding.type = this.environment.createAnnotatedType(fieldType,
new AnnotationBinding[] { this.environment.getNonNullAnnotation() });
Expand Down Expand Up @@ -2027,7 +2027,7 @@ private void scanFieldForNullAnnotation(IBinaryField field, VariableBinding fiel
this.externalAnnotationStatus = ExternalAnnotationStatus.TYPE_IS_ANNOTATED;
if (!explicitNullness) {
int nullDefaultFromField = getNullDefaultFrom(field.getAnnotations());
if (nullDefaultFromField == Binding.NO_NULL_DEFAULT ? hasNonNullDefaultFor(DefaultLocationField, -1)
if (nullDefaultFromField == Binding.NO_NULL_DEFAULT ? hasNonNullDefaultForType(fieldBinding.type, DefaultLocationField, -1)
: (nullDefaultFromField & DefaultLocationField) != 0) {
fieldBinding.tagBits |= TagBits.AnnotationNonNull;
}
Expand Down
13 changes: 6 additions & 7 deletions ...e.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/MethodBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -563,9 +563,6 @@ protected void fillInDefaultNonNullness18(AbstractMethodDeclaration sourceMethod
if (sourceMethod != null)
sourceMethod.arguments[i].binding.type = this.parameters[i];
}
} else if (sourceMethod != null && (parameter.tagBits & TagBits.AnnotationNonNull) != 0
&& sourceMethod.arguments[i].hasNullTypeAnnotation(AnnotationPosition.MAIN_TYPE)) {
sourceMethod.scope.problemReporter().nullAnnotationIsRedundant(sourceMethod, i);
}
}
if (added)
Expand Down Expand Up @@ -1354,7 +1351,7 @@ public TypeVariableBinding[] typeVariables() {
}
//pre: null annotation analysis is enabled
public boolean hasNonNullDefaultForReturnType(AbstractMethodDeclaration srcMethod) {
return hasNonNullDefaultFor(Binding.DefaultLocationReturnType, srcMethod, srcMethod == null ? -1 : srcMethod.declarationSourceStart);
return hasNonNullDefaultForType(this.returnType, Binding.DefaultLocationReturnType, srcMethod, srcMethod == null ? -1 : srcMethod.declarationSourceStart);
}

static int getNonNullByDefaultValue(AnnotationBinding annotation) {
Expand Down Expand Up @@ -1412,7 +1409,7 @@ public ParameterNonNullDefaultProvider hasNonNullDefaultForParameter(AbstractMet
// parameter specific NNBD found
b = (nonNullByDefaultValue & Binding.DefaultLocationParameter) != 0;
} else {
b = hasNonNullDefaultFor(Binding.DefaultLocationParameter, srcMethod, start);
b = hasNonNullDefaultForType(this.parameters[i], Binding.DefaultLocationParameter, srcMethod, start);
}
if (b) {
trueFound = true;
Expand All @@ -1427,12 +1424,14 @@ public ParameterNonNullDefaultProvider hasNonNullDefaultForParameter(AbstractMet
return trueFound ? ParameterNonNullDefaultProvider.TRUE_PROVIDER : ParameterNonNullDefaultProvider.FALSE_PROVIDER;
}
//pre: null annotation analysis is enabled
private boolean hasNonNullDefaultFor(int location, AbstractMethodDeclaration srcMethod, int start) {
private boolean hasNonNullDefaultForType(TypeBinding type, int location, AbstractMethodDeclaration srcMethod, int start) {
if (type != null && !type.acceptsNonNullDefault() && srcMethod != null && srcMethod.scope.environment().usesNullTypeAnnotations())
return false;
if ((this.modifiers & ExtraCompilerModifiers.AccIsDefaultConstructor) != 0)
return false;
if (this.defaultNullness != 0)
return (this.defaultNullness & location) != 0;
return this.declaringClass.hasNonNullDefaultFor(location, start);
return this.declaringClass.hasNonNullDefaultForType(null /*type was already checked*/, location, start);
}

public boolean redeclaresPublicObjectMethod(Scope scope) {
Expand Down
4 changes: 3 additions & 1 deletion ...dt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/ReferenceBinding.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1257,8 +1257,10 @@ public boolean hasMemberTypes() {
* for 1.8 check if the default is applicable to the given kind of location.
*/
// pre: null annotation analysis is enabled
boolean hasNonNullDefaultFor(int location, int sourceStart) {
boolean hasNonNullDefaultForType(TypeBinding type, int location, int sourceStart) {
// Note, STB overrides for correctly handling local types
if (type != null && !type.acceptsNonNullDefault())
return false;
ReferenceBinding currentType = this;
while (currentType != null) {
int nullDefault = ((ReferenceBinding)currentType.original()).getNullDefault();
Expand Down
6 changes: 6 additions & 0 deletions org.eclipse.jdt.core.compiler.batch/src/org/eclipse/jdt/internal/compiler/lookup/Scope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5404,6 +5404,12 @@ public boolean hasDefaultNullnessFor(int location, int sourceStart) {
return this.parent.hasDefaultNullnessFor(location, sourceStart);
}

public boolean hasDefaultNullnessForType(TypeBinding type, int location, int sourceStart) {
if (environment().usesNullTypeAnnotations() && type != null && !type.acceptsNonNullDefault())
return false;
return hasDefaultNullnessFor(location, sourceStart);
}

/*
* helper for hasDefaultNullnessFor(..) which inspects only ranges recorded within this scope.
*/
Expand Down
Loading

0 comments on commit 422a9b5

Please sign in to comment.