Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug with computing direct type use annotations on parameters #864

Merged
merged 3 commits into from
Nov 19, 2023
Merged

Fix bug with computing direct type use annotations on parameters #864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
10536e2
failing test
msridhar Nov 19, 2023
3a7a4c3
fix
msridhar Nov 19, 2023
ae1cbb4
more tests
msridhar Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion gradle/dependencies.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ def test = [
"org.junit.jupiter:junit-jupiter-api:5.0.2",
"org.apiguardian:apiguardian-api:1.0.0"
],
jetbrainsAnnotations : "org.jetbrains:annotations:13.0",
jetbrainsAnnotations : "org.jetbrains:annotations:24.1.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed so that @NotNull is a type use annotation

cfQual : "org.checkerframework:checker-qual:${versions.checkerFramework}",
// 2.5.5 is the last release to contain this artifact
cfCompatQual : "org.checkerframework:checker-compat-qual:2.5.5",
Expand Down
9 changes: 6 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/NullabilityUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,8 @@ public static TreePath findEnclosingMethodOrLambdaOrInitializer(TreePath path) {

/**
* NOTE: this method does not work for getting all annotations of parameters of methods from class
* files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int)}
* files. For that case, use {@link #getAllAnnotationsForParameter(Symbol.MethodSymbol, int,
* Config)}
*
* @param symbol the symbol
* @return all annotations on the symbol and on the type of the symbol
Expand Down Expand Up @@ -259,18 +260,20 @@ public static Stream<? extends AnnotationMirror> getAllAnnotations(Symbol symbol
*
* @param symbol the method symbol
* @param paramInd index of the parameter
* @param config NullAway configuration
* @return all declaration and type-use annotations for the parameter
*/
public static Stream<? extends AnnotationMirror> getAllAnnotationsForParameter(
Symbol.MethodSymbol symbol, int paramInd) {
Symbol.MethodSymbol symbol, int paramInd, Config config) {
Symbol.VarSymbol varSymbol = symbol.getParameters().get(paramInd);
return Stream.concat(
varSymbol.getAnnotationMirrors().stream(),
symbol.getRawTypeAttributes().stream()
.filter(
t ->
t.position.type.equals(TargetType.METHOD_FORMAL_PARAMETER)
&& t.position.parameter_index == paramInd));
&& t.position.parameter_index == paramInd
&& NullabilityUtil.isDirectTypeUseAnnotation(t, config)));
}

/**
Expand Down
4 changes: 2 additions & 2 deletions nullaway/src/main/java/com/uber/nullaway/Nullness.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ public static boolean paramHasNullableAnnotation(
return true;
}
return hasNullableAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config);
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
}

private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int paramInd) {
Expand Down Expand Up @@ -249,6 +249,6 @@ private static boolean isRecordEqualsParam(Symbol.MethodSymbol symbol, int param
public static boolean paramHasNonNullAnnotation(
Symbol.MethodSymbol symbol, int paramInd, Config config) {
return hasNonNullAnnotation(
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd), config);
NullabilityUtil.getAllAnnotationsForParameter(symbol, paramInd, config), config);
}
}
25 changes: 25 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,6 +987,31 @@ public void nullUnmarkedAndAcknowledgeRestrictiveAnnotations() {
.doTest();
}

@Test
public void nullUnmarkedRestrictiveAnnotationsAndGenerics() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.NullUnmarked;",
"import org.jetbrains.annotations.Nullable;",
"import org.jetbrains.annotations.NotNull;",
"import java.util.List;",
"public class Test {",
" @NullUnmarked",
" public static void takesNullable(@Nullable List<@NotNull String> l) {}",
" public static void test() {",
" takesNullable(null);",
" }",
"}")
.doTest();
}

@Test
public void nullMarkedStaticImports() {
makeTestHelperWithArgs(
Expand Down
12 changes: 12 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayTypeUseAnnotationsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,28 @@ public void annotationAppliedToTypeParameter() {
"import java.util.List;",
"import java.util.ArrayList;",
"import org.checkerframework.checker.nullness.qual.Nullable;",
"import org.checkerframework.checker.nullness.qual.NonNull;",
"class TypeArgumentAnnotation {",
" List<@Nullable String> fSafe = new ArrayList<>();",
" @Nullable List<String> fUnsafe = new ArrayList<>();",
" void useParamSafe(List<@Nullable String> list) {",
" list.hashCode();",
" }",
" void unsafeCall() {",
" // BUG: Diagnostic contains: passing @Nullable parameter",
" useParamSafe(null);",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, this was a false negative? (Either way, great to have the test, but just curious)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's right, this was a false negative before

" }",
" void useParamUnsafe(@Nullable List<String> list) {",
" // BUG: Diagnostic contains: dereferenced",
" list.hashCode();",
" }",
" void useParamUnsafeNonNullElements(@Nullable List<@NonNull String> list) {",
" // BUG: Diagnostic contains: dereferenced",
" list.hashCode();",
" }",
" void safeCall() {",
" useParamUnsafeNonNullElements(null);",
" }",
" void useFieldSafe() {",
" fSafe.hashCode();",
" }",
Expand Down