Skip to content

Commit

Permalink
Using method references to constructors can cause false positive: Unsafe
Browse files Browse the repository at this point in the history
null type conversion (type annotations)

Fixes eclipse-jdt#1009

+ real change is in ConstraintExpressionFormula
+ other changes are just extraction of a common convenience method
  • Loading branch information
stephan-herrmann committed Feb 1, 2024
1 parent 650c48b commit 2e4d0ca
Show file tree
Hide file tree
Showing 17 changed files with 57 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -529,7 +529,7 @@ public TypeBinding resolveType(BlockScope scope) {
for (int i = 0; i < this.typeArguments.length; i++)
this.typeArguments[i].checkNullConstraints(scope, (ParameterizedGenericMethodBinding) this.binding, typeVariables, i);
}
this.resolvedType = scope.environment().createAnnotatedType(this.resolvedType, new AnnotationBinding[] {scope.environment().getNonNullAnnotation()});
this.resolvedType = scope.environment().createNonNullAnnotatedType(this.resolvedType);
}
}
if (compilerOptions.sourceLevel >= ClassFileConstants.JDK1_8 &&
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,15 +218,13 @@ static TypeBinding maybeMarkArrayContentsNonNull(Scope scope, TypeBinding typeBi
LookupEnvironment environment = scope.environment();
if (environment.usesNullTypeAnnotations()
&& scope.hasDefaultNullnessFor(Binding.DefaultLocationArrayContents, sourceStart)) {
AnnotationBinding nonNullAnnotation = environment.getNonNullAnnotation();
typeBinding = addNonNullToDimensions(scope, typeBinding, nonNullAnnotation, dimensions);
typeBinding = addNonNullToDimensions(scope, typeBinding, environment.getNonNullAnnotation(), dimensions);

TypeBinding leafComponentType = typeBinding.leafComponentType();
if ((leafComponentType.tagBits & TagBits.AnnotationNullMASK) == 0 && leafComponentType.acceptsNonNullDefault()) {
if (leafConsumer != null)
leafConsumer.accept(leafComponentType);
TypeBinding nonNullLeafComponentType = scope.environment().createAnnotatedType(leafComponentType,
new AnnotationBinding[] { nonNullAnnotation });
TypeBinding nonNullLeafComponentType = scope.environment().createNonNullAnnotatedType(leafComponentType);
typeBinding = scope.createArrayType(nonNullLeafComponentType, typeBinding.dimensions(),
typeBinding.getTypeAnnotations());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class literals exist only for the raw underlying type.
boxedType = scope.boxing(this.targetType);
}
if (environment.usesNullTypeAnnotations())
boxedType = environment.createAnnotatedType(boxedType, new AnnotationBinding[] { environment.getNonNullAnnotation() });
boxedType = environment.createNonNullAnnotatedType(boxedType);
this.resolvedType = environment.createParameterizedType(classType, new TypeBinding[]{ boxedType }, null/*not a member*/);
} else {
this.resolvedType = classType;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@
import org.eclipse.jdt.internal.compiler.flow.FlowInfo;
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.eclipse.jdt.internal.compiler.impl.Constant;
import org.eclipse.jdt.internal.compiler.lookup.AnnotationBinding;
import org.eclipse.jdt.internal.compiler.lookup.Binding;
import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
import org.eclipse.jdt.internal.compiler.lookup.ExtraCompilerModifiers;
Expand Down Expand Up @@ -312,7 +311,7 @@ public TypeBinding resolveType(BlockScope scope) {
this.typeArguments[i].checkNullConstraints(scope, (ParameterizedGenericMethodBinding) this.binding, typeVariables, i);
}
if (this.resolvedType.isValidBinding()) {
this.resolvedType = scope.environment().createAnnotatedType(this.resolvedType, new AnnotationBinding[] {scope.environment().getNonNullAnnotation()});
this.resolvedType = scope.environment().createNonNullAnnotatedType(this.resolvedType);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -908,8 +908,7 @@ protected void checkNullAnnotations(BlockScope scope) {

TypeBinding descriptorParameter = this.descriptor.parameters[0];
if((descriptorParameter.tagBits & TagBits.AnnotationNullable) != 0) { // Note: normal dereferencing of 'unchecked' values is not reported, either
final TypeBinding receiver = scope.environment().createAnnotatedType(this.binding.declaringClass,
new AnnotationBinding[] { scope.environment().getNonNullAnnotation() });
final TypeBinding receiver = scope.environment().createNonNullAnnotatedType(this.binding.declaringClass);
scope.problemReporter().referenceExpressionArgumentNullityMismatch(this, receiver, descriptorParameter, this.descriptor, -1, NullAnnotationMatching.NULL_ANNOTATIONS_MISMATCH);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ void internalAnalyseOneArgument18(BlockScope currentScope, FlowContext flowConte
if (!expectedType.hasNullTypeAnnotations() && expectedNonNullness == Boolean.TRUE) {
// improve problem rendering when using a declaration annotation in a 1.8 setting
LookupEnvironment env = currentScope.environment();
expectedType = env.createAnnotatedType(expectedType, new AnnotationBinding[] {env.getNonNullAnnotation()});
expectedType = env.createNonNullAnnotatedType(expectedType);
}
flowContext.recordNullityMismatch(currentScope, argument, argument.resolvedType, expectedType, flowInfo, nullStatus, annotationStatus);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,8 @@ public void resolveAnnotations(Scope scope) {
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);
this.binding = (TypeVariableBinding) environment.createNonNullAnnotatedType(this.binding);

if (scope instanceof MethodScope) {
/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,11 @@
import org.eclipse.jdt.internal.compiler.flow.FlowInfo;
import org.eclipse.jdt.internal.compiler.impl.CompilerOptions;
import org.eclipse.jdt.internal.compiler.impl.Constant;
import org.eclipse.jdt.internal.compiler.lookup.AnnotationBinding;
import org.eclipse.jdt.internal.compiler.lookup.ArrayBinding;
import org.eclipse.jdt.internal.compiler.lookup.Binding;
import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
import org.eclipse.jdt.internal.compiler.lookup.ClassScope;
import org.eclipse.jdt.internal.compiler.lookup.LocalVariableBinding;
import org.eclipse.jdt.internal.compiler.lookup.LookupEnvironment;
import org.eclipse.jdt.internal.compiler.lookup.ParameterizedTypeBinding;
import org.eclipse.jdt.internal.compiler.lookup.ProblemReasons;
import org.eclipse.jdt.internal.compiler.lookup.ProblemReferenceBinding;
Expand Down Expand Up @@ -749,9 +747,7 @@ protected void resolveAnnotations(Scope scope, int location) {
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);
this.resolvedType = scope.environment().createNonNullAnnotatedType(this.resolvedType);
}
} else if (nullTagBits == TagBits.AnnotationNonNull) {
if (location != Binding.DefaultLocationParameter) { // parameters are handled in MethodBinding.fillInDefaultNonNullness18()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@ public char[] signature(ClassFile classFile) {
method.returnType = this.environment.globalOptions.sourceLevel >= ClassFileConstants.JDK1_5 ? this : originalMethod.returnType;
if (this.environment.globalOptions.isAnnotationBasedNullAnalysisEnabled) {
if (this.environment.usesNullTypeAnnotations())
method.returnType = this.environment.createAnnotatedType(method.returnType, new AnnotationBinding[] { this.environment.getNonNullAnnotation() });
method.returnType = this.environment.createNonNullAnnotatedType(method.returnType);
else
method.tagBits |= TagBits.AnnotationNonNull;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2038,8 +2038,7 @@ private void scanFieldForNullAnnotation(IBinaryField field, VariableBinding fiel
if (nullDefaultFromField == Binding.NO_NULL_DEFAULT
? hasNonNullDefaultForType(fieldType, DefaultLocationField, -1)
: (nullDefaultFromField & DefaultLocationField) != 0) {
fieldBinding.type = this.environment.createAnnotatedType(fieldType,
new AnnotationBinding[] { this.environment.getNonNullAnnotation() });
fieldBinding.type = this.environment.createNonNullAnnotatedType(fieldType);
}
}
return; // not using fieldBinding.tagBits when we have type annotations.
Expand Down Expand Up @@ -2124,8 +2123,7 @@ private void scanMethodForNullAnnotation(IBinaryMethod method, MethodBinding met
methodBinding.tagBits |= TagBits.AnnotationNonNull;
if (this.environment.usesNullTypeAnnotations()) {
if (methodBinding.returnType != null && !methodBinding.returnType.hasNullTypeAnnotations()) {
methodBinding.returnType = this.environment.createAnnotatedType(methodBinding.returnType,
new AnnotationBinding[] { this.environment.getNonNullAnnotation() });
methodBinding.returnType = this.environment.createNonNullAnnotatedType(methodBinding.returnType);
}
}
} else if (typeBit == TypeIds.BitNullableAnnotation) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@ private Object reduceReferenceExpressionCompatibility(ReferenceExpression refere
TypeBinding rPrime = compileTimeDecl.isConstructor() ? compileTimeDecl.declaringClass : compileTimeDecl.returnType.capture(inferenceContext.scope, reference.sourceStart(), reference.sourceEnd());
if (rPrime.id == TypeIds.T_void)
return FALSE;
if (compileTimeDecl.isConstructor() && inferenceContext.environment.usesNullTypeAnnotations())
rPrime = inferenceContext.environment.createNonNullAnnotatedType(rPrime);
return ConstraintTypeFormula.create(rPrime, r, COMPATIBLE, this.isSoft);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ public void fillInDefaultNonNullness(FieldDeclaration sourceField, Scope scope)
if (!this.type.acceptsNonNullDefault())
return;
if ( (this.type.tagBits & TagBits.AnnotationNullMASK) == 0) {
this.type = environment.createAnnotatedType(this.type, new AnnotationBinding[]{environment.getNonNullAnnotation()});
this.type = environment.createNonNullAnnotatedType(this.type);
} else if ((this.type.tagBits & TagBits.AnnotationNonNull) != 0) {
scope.problemReporter().nullAnnotationIsRedundant(sourceField);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1431,6 +1431,11 @@ public TypeBinding createAnnotatedType(TypeBinding type, AnnotationBinding[] new
return this.typeSystem.getAnnotatedType(type, new AnnotationBinding [][] { newbies });
}

// convenience:
public TypeBinding createNonNullAnnotatedType(TypeBinding type) {
return createAnnotatedType(type, new AnnotationBinding[] { getNonNullAnnotation() } );
}

public RawTypeBinding createRawType(ReferenceBinding genericType, ReferenceBinding enclosingType) {
AnnotationBinding[] annotations = genericType.typeAnnotations;
if (annotations != Binding.NO_ANNOTATIONS)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ protected void fillInDefaultNonNullness18(AbstractMethodDeclaration sourceMethod
if (existing == 0L) {
added = true;
if (!parameter.isBaseType()) {
this.parameters[i] = env.createAnnotatedType(parameter, new AnnotationBinding[]{env.getNonNullAnnotation()});
this.parameters[i] = env.createNonNullAnnotatedType(parameter);
if (sourceMethod != null)
sourceMethod.arguments[i].binding.type = this.parameters[i];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -316,14 +316,14 @@ public static ParameterizedMethodBinding instantiateGetClass(TypeBinding receive
LookupEnvironment environment = scope.environment();
TypeBinding rawType = environment.convertToRawType(receiverType.erasure(), false /*do not force conversion of enclosing types*/);
if (environment.usesNullTypeAnnotations())
rawType = environment.createAnnotatedType(rawType, new AnnotationBinding[] { environment.getNonNullAnnotation() });
rawType = environment.createNonNullAnnotatedType(rawType);
method.returnType = environment.createParameterizedType(
genericClassType,
new TypeBinding[] { environment.createWildcard(genericClassType, 0, rawType, null /*no extra bound*/, Wildcard.EXTENDS) },
null);
if (environment.globalOptions.isAnnotationBasedNullAnalysisEnabled) {
if (environment.usesNullTypeAnnotations())
method.returnType = environment.createAnnotatedType(method.returnType, new AnnotationBinding[] { environment.getNonNullAnnotation() });
method.returnType = environment.createNonNullAnnotatedType(method.returnType);
else
method.tagBits |= TagBits.AnnotationNonNull;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ public SyntheticMethodBinding(int purpose, ArrayBinding arrayType, char [] selec
if (environment.globalOptions.isAnnotationBasedNullAnalysisEnabled) {
// mark X[]::new and X[]::clone as returning 'X @NonNull' (don't wait (cf. markNonNull()), because we're called as late as codeGen):
if (environment.usesNullTypeAnnotations())
this.returnType = environment.createAnnotatedType(this.returnType, new AnnotationBinding[]{ environment.getNonNullAnnotation() });
this.returnType = environment.createNonNullAnnotatedType(this.returnType);
else
this.tagBits |= TagBits.AnnotationNonNull;
}
Expand Down Expand Up @@ -686,15 +686,15 @@ static void markNonNull(MethodBinding method, int purpose, LookupEnvironment env
if (environment.usesNullTypeAnnotations()) {
TypeBinding elementType = ((ArrayBinding)method.returnType).leafComponentType();
AnnotationBinding nonNullAnnotation = environment.getNonNullAnnotation();
elementType = environment.createAnnotatedType(elementType, new AnnotationBinding[]{ environment.getNonNullAnnotation() });
elementType = environment.createNonNullAnnotatedType(elementType);
method.returnType = environment.createArrayType(elementType, 1, new AnnotationBinding[]{ nonNullAnnotation, null });
} else {
method.tagBits |= TagBits.AnnotationNonNull;
}
return;
case EnumValueOf:
if (environment.usesNullTypeAnnotations()) {
method.returnType = environment.createAnnotatedType(method.returnType, new AnnotationBinding[]{ environment.getNonNullAnnotation() });
method.returnType = environment.createNonNullAnnotatedType(method.returnType);
} else {
method.tagBits |= TagBits.AnnotationNonNull;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -945,4 +945,35 @@ public abstract static class InnerBlah extends Blah<OutputStream, @NonNull MyExc
runner.classLibraries = this.LIBS;
runner.runConformTest();
}
public void testGH1009() {
Runner runner = new Runner();
Map<String, String> options = getCompilerOptions();
options.put(CompilerOptions.OPTION_ReportAnnotatedTypeArgumentToUnannotated, CompilerOptions.ERROR);
runner.customOptions = options;
runner.testFiles = new String[] {
"UnsafeNullTypeConversionFalsePositive.java",
"""
import java.util.ArrayList;
import java.util.List;
import java.util.stream.Collectors;
import org.eclipse.jdt.annotation.NonNull;
public class UnsafeNullTypeConversionFalsePositive {
public static void main(final String[] args) {
final List<@NonNull StringBuffer> someList = new ArrayList<>();
List<@NonNull String> results;
// was buggy:
results = someList.stream().map(String::new).collect(Collectors.toList());
// was OK:
results = someList.stream().map(buff -> new String(buff)).collect(Collectors.toList());
results = someList.stream().<@NonNull String>map(String::new).collect(Collectors.toList());
}
}
"""
};
runner.classLibraries = this.LIBS;
runner.runConformTest();
}
}

0 comments on commit 2e4d0ca

Please sign in to comment.