diff --git a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/ExternalGenericNullability.java b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/ExternalGenericNullability.java new file mode 100644 index 00000000000..dc27d6efd10 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/ExternalGenericNullability.java @@ -0,0 +1,45 @@ +package annotations.nullability.no_default; + +import java.util.List; +import org.eclipse.jdt.annotation.NonNull; +import org.eclipse.jdt.annotation.Nullable; + +class ExternalGenericParent { + + T inherited(T value) { + return value; + } +} + +public class ExternalGenericNullability extends ExternalGenericParent { + + public ExternalGenericNullability(T value) { + } + + @NonNull + public T declaredNonNull(@NonNull T value) { + return value; + } + + @Nullable + public T declaredNullable(@Nullable T value) { + return value; + } + + public List<@Nullable T> nestedNullable(List<@Nullable T> values) { + return values; + } + + public U genericMethod(U value) { + return value; + } + + public U genericVarargs(U... values) { + return values[0]; + } + + @org.eclipse.jdt.annotation.NonNullByDefault + public T defaulted(T value) { + return value; + } +} diff --git a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/ExternalGenericNullabilityUsage.java b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/ExternalGenericNullabilityUsage.java new file mode 100644 index 00000000000..44c538c3eb4 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/ExternalGenericNullabilityUsage.java @@ -0,0 +1,18 @@ +package annotations.nullability.no_default; + +import java.util.List; +import org.eclipse.jdt.annotation.NonNull; + +public class ExternalGenericNullabilityUsage { + + public void calls(ExternalGenericNullability<@NonNull String> dependency) { + dependency.declaredNonNull("value"); + dependency.declaredNullable(null); + dependency.nestedNullable(List.of("value")); + dependency.inherited(null); + new ExternalGenericNullability<@NonNull String>(null); + dependency.<@NonNull String>genericMethod("value"); + dependency.<@NonNull String>genericVarargs("value"); + dependency.defaulted("value"); + } +} diff --git a/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityWithInferredTypeArgument.java b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityWithInferredTypeArgument.java new file mode 100644 index 00000000000..3104f6fb2a2 --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/annotations/nullability/no_default/NullabilityWithInferredTypeArgument.java @@ -0,0 +1,29 @@ +package annotations.nullability.no_default; + +import java.util.Optional; +import org.eclipse.jdt.annotation.NonNull; + +public class NullabilityWithInferredTypeArgument { + + private final String value = "value"; + + @NonNull + public String getValue() { + return value; + } + + public String callOrElse(NullabilityWithInferredTypeArgument a) { + // "map" infers "Optional<@NonNull String>", the @NonNull is part of the type argument and says nothing about the parameter of "orElse" + return Optional.ofNullable(a) + .map(NullabilityWithInferredTypeArgument::getValue) + .orElse(null); + } + + public boolean nullCheckAfterOrElse(NullabilityWithInferredTypeArgument a) { + String result = Optional.ofNullable(a) + .map(NullabilityWithInferredTypeArgument::getValue) + .orElse(null); + // the inferred @NonNull must not make "orElse" look like it never returns null, the check below is not always false + return result == null; + } +} diff --git a/java-frontend/src/main/java/org/sonar/java/model/JMethodSymbol.java b/java-frontend/src/main/java/org/sonar/java/model/JMethodSymbol.java index 5ae3d502130..ba3c3808247 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/JMethodSymbol.java +++ b/java-frontend/src/main/java/org/sonar/java/model/JMethodSymbol.java @@ -92,9 +92,18 @@ public List declarationParameters() { } else { parameters = new ArrayList<>(); IMethodBinding methodBinding = methodBinding(); + IMethodBinding methodDeclaration = methodBinding.getMethodDeclaration(); ITypeBinding[] parameterTypeBindings = methodBinding.getParameterTypes(); + ITypeBinding[] declaredParameterTypeBindings = methodDeclaration.getParameterTypes(); for (int i = 0; i < parameterTypeBindings.length; i++) { - parameters.add(new JVariableSymbol.ParameterPlaceholderSymbol(i, sema, methodBinding.getMethodDeclaration(), parameterTypeBindings[i])); + // Annotations must come from the declared type: type annotations of the substituted type can be inferred from the call site + // (e.g. "Optional.of(x).map(A::nonNullMethod).orElse(null)" infers "Optional<@NonNull String>", making "orElse" look like it takes + // a @NonNull argument), which does not tell anything about the parameter of the declared method. + // When semantic recovery does not provide the corresponding declared parameter, keep the previous behavior instead of + // treating its nullability as unknown. This preserves existing detections and avoids introducing false negatives while + // refining the common case to remove false positives. + ITypeBinding declaredParameterType = i < declaredParameterTypeBindings.length ? declaredParameterTypeBindings[i] : parameterTypeBindings[i]; + parameters.add(new JVariableSymbol.ParameterPlaceholderSymbol(i, sema, methodDeclaration, parameterTypeBindings[i], declaredParameterType)); } } } diff --git a/java-frontend/src/main/java/org/sonar/java/model/JSymbol.java b/java-frontend/src/main/java/org/sonar/java/model/JSymbol.java index dcd11673412..49f0c2bb552 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/JSymbol.java +++ b/java-frontend/src/main/java/org/sonar/java/model/JSymbol.java @@ -386,12 +386,21 @@ private SymbolMetadata convertMetadata() { } return convertMetadata(type); case IBinding.METHOD: - ITypeBinding returnType = ((IMethodBinding) binding).getReturnType(); + IMethodBinding methodBinding = (IMethodBinding) binding; + ITypeBinding returnType = methodBinding.getReturnType(); // In rare circumstances, when the semantic information is incomplete, returnType can be null. if (returnType == null) { return Symbols.EMPTY_METADATA; } - return convertMetadata(returnType); + // Annotations are read from the declared return type, so that annotations inferred for a type variable at the call site + // (e.g. "Optional.of(x).map(A::nonNullMethod)" infers "Optional<@NonNull String>", making "orElse" look like it never returns null) + // are not mistaken for annotations of the method itself. + IMethodBinding methodDeclaration = methodBinding.getMethodDeclaration(); + ITypeBinding declaredReturnType = methodDeclaration == null ? null : methodDeclaration.getReturnType(); + // When semantic recovery does not provide a declared return type, keep the previous behavior instead of treating its + // nullability as unknown. This preserves existing detections and avoids introducing false negatives while refining the + // common case to remove false positives. + return convertMetadata(declaredReturnType == null ? returnType : declaredReturnType); default: return new JSymbolMetadata(sema, this, binding.getAnnotations()); } diff --git a/java-frontend/src/main/java/org/sonar/java/model/JVariableSymbol.java b/java-frontend/src/main/java/org/sonar/java/model/JVariableSymbol.java index ae6458f39de..0e5bf9fbdb3 100644 --- a/java-frontend/src/main/java/org/sonar/java/model/JVariableSymbol.java +++ b/java-frontend/src/main/java/org/sonar/java/model/JVariableSymbol.java @@ -89,11 +89,16 @@ static class ParameterPlaceholderSymbol extends Symbols.DefaultSymbol implements private final Type type; private final SymbolMetadata metadata; - ParameterPlaceholderSymbol(int index, JSema sema, IMethodBinding owner, ITypeBinding typeBinding) { + /** + * @param typeBinding the type of the parameter at the call site, after type substitution + * @param declaredTypeBinding the type of the parameter as declared, before type substitution. Annotations are read from it, so that + * annotations inferred for a type variable are not mistaken for annotations of the parameter itself. + */ + ParameterPlaceholderSymbol(int index, JSema sema, IMethodBinding owner, ITypeBinding typeBinding, ITypeBinding declaredTypeBinding) { this.name = "arg" + index; this.owner = sema.methodSymbol(owner); this.type = sema.type(typeBinding); - this.metadata = JSymbolMetadata.of(sema, this, typeBinding, owner.getParameterAnnotations(index)); + this.metadata = JSymbolMetadata.of(sema, this, declaredTypeBinding, owner.getParameterAnnotations(index)); } @Override diff --git a/java-frontend/src/test/java/org/sonar/java/model/JSymbolMetadataTest.java b/java-frontend/src/test/java/org/sonar/java/model/JSymbolMetadataTest.java index 60a2f838c2f..914a31355bf 100644 --- a/java-frontend/src/test/java/org/sonar/java/model/JSymbolMetadataTest.java +++ b/java-frontend/src/test/java/org/sonar/java/model/JSymbolMetadataTest.java @@ -41,6 +41,8 @@ import org.sonar.plugins.java.api.tree.IdentifierTree; import org.sonar.plugins.java.api.tree.MethodInvocationTree; import org.sonar.plugins.java.api.tree.MethodTree; +import org.sonar.plugins.java.api.tree.NewClassTree; +import org.sonar.plugins.java.api.tree.ReturnStatementTree; import org.sonar.plugins.java.api.tree.Tree; import static org.assertj.core.api.Assertions.assertThat; @@ -237,6 +239,112 @@ void generics_nullability() throws IOException { .isSameAs(invocation2ParamData); } + @Test + void nullability_is_not_inferred_from_type_arguments() throws IOException { + Path sourceFile = NULLABILITY_SOURCE_DIR.resolve(Paths.get("no_default", "NullabilityWithInferredTypeArgument.java")); + CompilationUnitTree cut = JParserTestUtils.parse(sourceFile.toRealPath().toFile(), JParserTestUtils.checksTestClassPath()); + ClassTree classTree = (ClassTree) cut.types().get(0); + MethodTree callOrElse = (MethodTree) classTree.members().get(2); + + MethodInvocationTree orElseInvocation = (MethodInvocationTree) ((ReturnStatementTree) callOrElse.block().body().get(0)).expression(); + Symbol orElseParameter = orElseInvocation.methodSymbol().declarationParameters().get(0); + + assertThat(orElseParameter.metadata().annotations()).isEmpty(); + assertThat(orElseParameter.metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + } + + @Test + void method_nullability_is_not_inferred_from_type_arguments() throws IOException { + Path sourceFile = NULLABILITY_SOURCE_DIR.resolve(Paths.get("no_default", "NullabilityWithInferredTypeArgument.java")); + CompilationUnitTree cut = JParserTestUtils.parse(sourceFile.toRealPath().toFile(), JParserTestUtils.checksTestClassPath()); + ClassTree classTree = (ClassTree) cut.types().get(0); + MethodTree callOrElse = (MethodTree) classTree.members().get(2); + + MethodInvocationTree orElseInvocation = (MethodInvocationTree) ((ReturnStatementTree) callOrElse.block().body().get(0)).expression(); + SymbolMetadata orElseMetadata = orElseInvocation.methodSymbol().metadata(); + + assertThat(orElseMetadata.symbolAnnotations()).isEmpty(); + assertThat(orElseMetadata.nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + } + + @Test + void generic_external_method_metadata_comes_from_declaration() throws IOException { + MethodTree calls = externalGenericCalls(); + MethodInvocationTree declaredNonNull = invocationAt(calls, 0); + MethodInvocationTree declaredNullable = invocationAt(calls, 1); + MethodInvocationTree inherited = invocationAt(calls, 3); + + assertThat(declaredNonNull.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String"); + assertThat(declaredNonNull.methodSymbol().returnType().name()).isEqualTo("String"); + assertThat(declaredNonNull.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(NON_NULL); + assertThat(declaredNonNull.methodSymbol().metadata().nullabilityData().type()).isEqualTo(NON_NULL); + + assertThat(declaredNullable.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String"); + assertThat(declaredNullable.methodSymbol().returnType().name()).isEqualTo("String"); + assertThat(declaredNullable.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(STRONG_NULLABLE); + assertThat(declaredNullable.methodSymbol().metadata().nullabilityData().type()).isEqualTo(STRONG_NULLABLE); + + assertThat(inherited.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String"); + assertThat(inherited.methodSymbol().returnType().name()).isEqualTo("String"); + assertThat(inherited.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + assertThat(inherited.methodSymbol().metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + } + + @Test + void generic_external_constructor_and_nested_type_metadata_come_from_declaration() throws IOException { + MethodTree calls = externalGenericCalls(); + MethodInvocationTree nestedNullable = invocationAt(calls, 2); + NewClassTree constructor = (NewClassTree) ((ExpressionStatementTree) calls.block().body().get(4)).expression(); + + assertThat(nestedNullable.methodSymbol().parameterTypes().get(0).name()).isEqualTo("List"); + assertThat(nestedNullable.methodSymbol().returnType().name()).isEqualTo("List"); + assertThat(nestedNullable.methodSymbol().metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + assertThat(nestedNullable.methodSymbol().metadata().parametersMetadata()).singleElement() + .extracting(metadata -> metadata.nullabilityData().type()) + .isEqualTo(STRONG_NULLABLE); + + assertThat(constructor.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String"); + assertThat(constructor.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + } + + @Test + void generic_external_method_type_parameter_metadata_comes_from_declaration() throws IOException { + MethodTree calls = externalGenericCalls(); + MethodInvocationTree genericMethod = invocationAt(calls, 5); + MethodInvocationTree genericVarargs = invocationAt(calls, 6); + + assertThat(genericMethod.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String"); + assertThat(genericMethod.methodSymbol().returnType().name()).isEqualTo("String"); + assertThat(genericMethod.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + assertThat(genericMethod.methodSymbol().metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + + assertThat(genericVarargs.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String[]"); + assertThat(genericVarargs.methodSymbol().returnType().name()).isEqualTo("String"); + assertThat(genericVarargs.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + assertThat(genericVarargs.methodSymbol().metadata().nullabilityData().type()).isEqualTo(NullabilityType.NO_ANNOTATION); + } + + @Test + void generic_external_method_default_nullability_is_preserved() throws IOException { + MethodInvocationTree defaulted = invocationAt(externalGenericCalls(), 7); + + assertThat(defaulted.methodSymbol().parameterTypes().get(0).name()).isEqualTo("String"); + assertThat(defaulted.methodSymbol().returnType().name()).isEqualTo("String"); + assertThat(defaulted.methodSymbol().declarationParameters().get(0).metadata().nullabilityData().type()).isEqualTo(NON_NULL); + assertThat(defaulted.methodSymbol().metadata().nullabilityData().type()).isEqualTo(NON_NULL); + } + + private static MethodTree externalGenericCalls() throws IOException { + Path sourceFile = NULLABILITY_SOURCE_DIR.resolve(Paths.get("no_default", "ExternalGenericNullabilityUsage.java")); + CompilationUnitTree cut = JParserTestUtils.parse(sourceFile.toRealPath().toFile(), JParserTestUtils.checksTestClassPath()); + ClassTree classTree = (ClassTree) cut.types().get(0); + return (MethodTree) classTree.members().get(0); + } + + private static MethodInvocationTree invocationAt(MethodTree method, int index) { + return (MethodInvocationTree) ((ExpressionStatementTree) method.block().body().get(index)).expression(); + } + @Nested class NullabilityDataTest {