Skip to content

Commit

Permalink
JSpecify: read upper bound annotations from bytecode and add tests (#…
Browse files Browse the repository at this point in the history
…1004)

We add support for reading upper bound annotations from bytecodes for
JSpecify mode generics checks. Many more types of annotations need to be
read with special handling on JDK versions earlier than 22, due to
https://bugs.openjdk.org/browse/JDK-8225377. For now, we disable some
tests on earlier JDKs. We may add this special handling in the future,
but it will be a significant amount of work. We are still hoping the
relevant javac fix gets backported, which will fix this for us; see
jspecify/jspecify#365.
  • Loading branch information
msridhar authored Jul 26, 2024
1 parent 43bbb79 commit 5584291
Show file tree
Hide file tree
Showing 8 changed files with 318 additions and 13 deletions.
12 changes: 12 additions & 0 deletions nullaway/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,15 @@ tasks.register('buildWithNullAway', JavaCompile) {
project.tasks.named('check').configure {
dependsOn 'buildWithNullAway'
}

tasks.withType(Test).configureEach { test ->
if (test.javaVersion < JavaVersion.VERSION_22) {
// Certain tests involving reading annotations from bytecode will not pass on pre-JDK-22 javac
// until the fix for https://bugs.openjdk.org/browse/JDK-8225377 is backported or until we add
// workarounds. See https://github.com/uber/NullAway/issues/1005.
test.filter {
excludeTestsMatching "com.uber.nullaway.jspecify.BytecodeGenericsTests.genericsChecksForParamPassingAndReturns"
excludeTestsMatching "com.uber.nullaway.jspecify.BytecodeGenericsTests.genericsChecksForFieldAssignments"
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import com.sun.source.util.TreePath;
import com.sun.tools.javac.code.Attribute;
import com.sun.tools.javac.code.Symbol;
import com.sun.tools.javac.code.TargetType;
import com.sun.tools.javac.code.Type;
import com.uber.nullaway.CodeAnnotationInfo;
import com.uber.nullaway.Config;
Expand Down Expand Up @@ -97,25 +98,51 @@ public static void checkInstantiationForParameterizedTypedTree(
if (baseType == null) {
return;
}
boolean[] typeParamsWithNullableUpperBound =
getTypeParamsWithNullableUpperBound(baseType, config, state, handler);
com.sun.tools.javac.util.List<Type> baseTypeArgs = baseType.tsym.type.getTypeArguments();
for (int i = 0; i < baseTypeArgs.size(); i++) {
if (nullableTypeArguments.containsKey(i)) {

Type typeVariable = baseTypeArgs.get(i);
Type upperBound = typeVariable.getUpperBound();
com.sun.tools.javac.util.List<Attribute.TypeCompound> annotationMirrors =
upperBound.getAnnotationMirrors();
boolean hasNullableAnnotation =
Nullness.hasNullableAnnotation(annotationMirrors.stream(), config)
|| handler.onOverrideTypeParameterUpperBound(baseType.tsym.toString(), i);
// if base type argument does not have @Nullable annotation then the instantiation is
if (nullableTypeArguments.containsKey(i) && !typeParamsWithNullableUpperBound[i]) {
// if base type variable does not have @Nullable upper bound then the instantiation is
// invalid
if (!hasNullableAnnotation) {
reportInvalidInstantiationError(
nullableTypeArguments.get(i), baseType, typeVariable, state, analysis);
reportInvalidInstantiationError(
nullableTypeArguments.get(i), baseType, baseTypeArgs.get(i), state, analysis);
}
}
}

private static boolean[] getTypeParamsWithNullableUpperBound(
Type type, Config config, VisitorState state, Handler handler) {
Symbol.TypeSymbol tsym = type.tsym;
com.sun.tools.javac.util.List<Type> baseTypeArgs = tsym.type.getTypeArguments();
boolean[] result = new boolean[baseTypeArgs.size()];
for (int i = 0; i < baseTypeArgs.size(); i++) {
Type typeVariable = baseTypeArgs.get(i);
Type upperBound = typeVariable.getUpperBound();
com.sun.tools.javac.util.List<Attribute.TypeCompound> annotationMirrors =
upperBound.getAnnotationMirrors();
if (Nullness.hasNullableAnnotation(annotationMirrors.stream(), config)
|| handler.onOverrideTypeParameterUpperBound(type.tsym.toString(), i)) {
result[i] = true;
}
}
// For handling types declared in bytecode rather than source code.
// Due to a bug in javac versions before JDK 22 (https://bugs.openjdk.org/browse/JDK-8225377),
// the above code does not work for types declared in bytecode. We need to read the raw type
// attributes instead.
com.sun.tools.javac.util.List<Attribute.TypeCompound> rawTypeAttributes =
tsym.getRawTypeAttributes();
if (rawTypeAttributes != null) {
for (Attribute.TypeCompound typeCompound : rawTypeAttributes) {
if (typeCompound.position.type.equals(TargetType.CLASS_TYPE_PARAMETER_BOUND)
&& ASTHelpers.isSameType(
typeCompound.type, JSPECIFY_NULLABLE_TYPE_SUPPLIER.get(state), state)) {
int index = typeCompound.position.parameter_index;
result[index] = true;
}
}
}
return result;
}

private static void reportInvalidInstantiationError(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,230 @@
package com.uber.nullaway.jspecify;

import com.google.errorprone.CompilationTestHelper;
import com.uber.nullaway.NullAwayTestsBase;
import java.util.Arrays;
import org.junit.Test;

public class BytecodeGenericsTests extends NullAwayTestsBase {

@Test
public void basicTypeParamInstantiation() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.NonNullTypeParam;",
"import com.uber.lib.generics.NullableTypeParam;",
"class Test {",
" // BUG: Diagnostic contains: Generic type parameter",
" static void testBadNonNull(NonNullTypeParam<@Nullable String> t1) {",
" // BUG: Diagnostic contains: Generic type parameter",
" NonNullTypeParam<@Nullable String> t2 = null;",
" NullableTypeParam<@Nullable String> t3 = null;",
" }",
"}")
.doTest();
}

@Test
public void multipleTypeParametersInstantiation() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.MixedTypeParam;",
"class Test {",
" static class PartiallyInvalidSubclass",
" // BUG: Diagnostic contains: Generic type parameter",
" extends MixedTypeParam<@Nullable String, String, String, @Nullable String> {}",
" static class ValidSubclass1",
" extends MixedTypeParam<String, @Nullable String, @Nullable String, String> {}",
" static class PartiallyInvalidSubclass2",
" extends MixedTypeParam<",
" String,",
" String,",
" String,",
" // BUG: Diagnostic contains: Generic type parameter",
" @Nullable String> {}",
" static class ValidSubclass2 extends MixedTypeParam<String, String, String, String> {}",
"}")
.doTest();
}

@Test
public void genericsChecksForAssignments() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.NullableTypeParam;",
"class Test {",
" static void testPositive(NullableTypeParam<@Nullable String> t1) {",
" // BUG: Diagnostic contains: Cannot assign from type NullableTypeParam<@Nullable String>",
" NullableTypeParam<String> t2 = t1;",
" }",
" static void testNegative(NullableTypeParam<@Nullable String> t1) {",
" NullableTypeParam<@Nullable String> t2 = t1;",
" }",
"}")
.doTest();
}

@Test
public void genericsChecksForFieldAssignments() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.NullableTypeParam;",
"class Test {",
" static void testPositive(NullableTypeParam<String> t1) {",
" // BUG: Diagnostic contains: Cannot assign from type NullableTypeParam<String>",
" NullableTypeParam.staticField = t1;",
" // BUG: Diagnostic contains: Cannot assign from type NullableTypeParam<@Nullable String>",
" NullableTypeParam<String> t2 = NullableTypeParam.staticField;",
" }",
" static void testNegative(NullableTypeParam<@Nullable String> t1) {",
" NullableTypeParam.staticField = t1;",
" NullableTypeParam<@Nullable String> t2 = NullableTypeParam.staticField;",
" }",
"}")
.doTest();
}

@Test
public void genericsChecksForParamPassingAndReturns() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.NullableTypeParam;",
"import com.uber.lib.generics.GenericTypeArgMethods;",
"class Test {",
" static void testPositive(NullableTypeParam<String> t1) {",
" // BUG: Diagnostic contains: Cannot pass parameter of type NullableTypeParam<String>",
" GenericTypeArgMethods.nullableTypeParamArg(t1);",
" // BUG: Diagnostic contains: Cannot assign from type NullableTypeParam<@Nullable String>",
" NullableTypeParam<String> t2 = GenericTypeArgMethods.nullableTypeParamReturn();",
" }",
" static void testNegative(NullableTypeParam<@Nullable String> t1) {",
" GenericTypeArgMethods.nullableTypeParamArg(t1);",
" NullableTypeParam<@Nullable String> t2 = GenericTypeArgMethods.nullableTypeParamReturn();",
" }",
"}")
.doTest();
}

@Test
public void overrideParameterType() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.Fn;",
"class Test {",
" static class TestFunc1 implements Fn<@Nullable String, String> {",
" @Override",
" // BUG: Diagnostic contains: parameter s is",
" public String apply(String s) {",
" return s;",
" }",
" }",
" static class TestFunc2 implements Fn<@Nullable String, String> {",
" @Override",
" public String apply(@Nullable String s) {",
" return \"hi\";",
" }",
" }",
" static class TestFunc3 implements Fn<String, String> {",
" @Override",
" public String apply(String s) {",
" return \"hi\";",
" }",
" }",
" static class TestFunc4 implements Fn<String, String> {",
" // this override is legal, we should get no error",
" @Override",
" public String apply(@Nullable String s) {",
" return \"hi\";",
" }",
" }",
" static void useTestFunc(String s) {",
" Fn<@Nullable String, String> f1 = new TestFunc2();",
" // should get no error here",
" f1.apply(null);",
" Fn<String, String> f2 = new TestFunc3();",
" // BUG: Diagnostic contains: passing @Nullable parameter",
" f2.apply(null);",
" }",
"}")
.doTest();
}

@Test
public void overrideReturnTypes() {
makeHelper()
.addSourceLines(
"Test.java",
"package com.uber;",
"import org.jspecify.annotations.Nullable;",
"import com.uber.lib.generics.Fn;",
"class Test {",
" static class TestFunc1 implements Fn<String, @Nullable String> {",
" @Override",
" public @Nullable String apply(String s) {",
" return s;",
" }",
" }",
" static class TestFunc2 implements Fn<String, @Nullable String> {",
" @Override",
" public String apply(String s) {",
" return s;",
" }",
" }",
" static class TestFunc3 implements Fn<String, String> {",
" @Override",
" // BUG: Diagnostic contains: method returns @Nullable, but superclass",
" public @Nullable String apply(String s) {",
" return s;",
" }",
" }",
" static class TestFunc4 implements Fn<@Nullable String, String> {",
" @Override",
" // BUG: Diagnostic contains: method returns @Nullable, but superclass",
" public @Nullable String apply(String s) {",
" return s;",
" }",
" }",
" static void useTestFunc(String s) {",
" Fn<String, @Nullable String> f1 = new TestFunc1();",
" String t1 = f1.apply(s);",
" // BUG: Diagnostic contains: dereferenced expression",
" t1.hashCode();",
" TestFunc2 f2 = new TestFunc2();",
" String t2 = f2.apply(s);",
" // There should not be an error here",
" t2.hashCode();",
" Fn<String, @Nullable String> f3 = new TestFunc2();",
" String t3 = f3.apply(s);",
" // BUG: Diagnostic contains: dereferenced expression",
" t3.hashCode();",
" // BUG: Diagnostic contains: dereferenced expression",
" f3.apply(s).hashCode();",
" }",
"}")
.doTest();
}

private CompilationTestHelper makeHelper() {
return makeTestHelperWithArgs(
Arrays.asList(
"-XepOpt:NullAway:AnnotatedPackages=com.uber", "-XepOpt:NullAway:JSpecifyMode=true"));
}
}
7 changes: 7 additions & 0 deletions test-java-lib/src/main/java/com/uber/lib/generics/Fn.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package com.uber.lib.generics;

import org.jspecify.annotations.Nullable;

public interface Fn<P extends @Nullable Object, R extends @Nullable Object> {
R apply(P p);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.uber.lib.generics;

import org.jspecify.annotations.Nullable;

public class GenericTypeArgMethods {

public static void nullableTypeParamArg(NullableTypeParam<@Nullable String> s) {}

public static NullableTypeParam<@Nullable String> nullableTypeParamReturn() {
return new NullableTypeParam<@Nullable String>();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
package com.uber.lib.generics;

import org.jspecify.annotations.Nullable;

public class MixedTypeParam<E1, E2 extends @Nullable Object, E3 extends @Nullable Object, E4> {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
package com.uber.lib.generics;

public class NonNullTypeParam<E> {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package com.uber.lib.generics;

import org.jspecify.annotations.Nullable;

public class NullableTypeParam<E extends @Nullable Object> {

public static NullableTypeParam<@Nullable String> staticField =
new NullableTypeParam<@Nullable String>();
}

0 comments on commit 5584291

Please sign in to comment.