Skip to content

Commit

Permalink
Fix crash when querying null-markedness of $primitive.class expressio…
Browse files Browse the repository at this point in the history
…ns. (#654)

NullAway 0.10.0 introduces a crash when performing dataflow and encountering expressions of the form `${primitive}.class` (e.g. `int.class`, `boolean.class`, `void.class`).

These expressions look to dataflow and AST analysis like standard field accesses, but calling `ASTHelpers.enclosingClass(symbol)` on the symbol representing e.g. `boolean.class`, returns `null`. 

This is despite `symbol.owner` for these expressions being a non-null `ClassSymbol`. What happens there is that the class symbol for int/boolean/etc is one for which `encClass()` returns `null` rather than itself, which is the behavior for "real" classes. 

Before 0.10.0, we were  skipping this kind of expressions via `NullabilityUtil.mayBeNullFieldFromType(...)`, but after #652, we have a new path through handlers that might attempt to query them for null-markedness.

We _could_ skip these fields by partitioning `NullabilityUtil.mayBeNullFieldFromType(...)` into two different methods performing two separate checks:

- One checking for `symbol.getSimpleName().toString().equals("class") || symbol.isEnum()` and aborting `visitFieldAccess(...)` early, without worrying about `handler.onDataflowVisitFieldAccess(...)`
- One checking for `!codeAnnotationInfo.isSymbolUnannotated(symbol, config) && Nullness.hasNullableAnnotation(symbol, config)` and combined with the results from any handler.

That said, a potentially more robust approach (and the one this PR takes), requiring less careful maintenance of invariants, is to add handling of `${primitive}.class` directly into  `CodeAnnotationInfo::isSymbolUnannotated`. This should prevent a similar bug from being re-introduced through a different checking path, which is important given the difficultly of debugging dataflow issues.

This PR also contains a small unrelated test case for static import that appeared to be related during triage of this bug, but ultimately turned out not to be. It remains a useful test to have.
  • Loading branch information
lazaroclapp authored Sep 12, 2022
1 parent 99fc67b commit de1a12b
Show file tree
Hide file tree
Showing 2 changed files with 118 additions and 0 deletions.
23 changes: 23 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,22 @@ public boolean isGenerated(Symbol symbol, Config config) {
return ASTHelpers.hasDirectAnnotationWithSimpleName(outermostClassSymbol, "Generated");
}

/**
* Check if the symbol represents the .class field of a primitive type.
*
* <p>e.g. int.class, boolean.class, void.class, etc.
*
* @param symbol symbol for entity
* @return true iff this symbol represents t.class for a primitive type t.
*/
private static boolean isClassFieldOfPrimitiveType(Symbol symbol) {
return symbol.name.contentEquals("class")
&& symbol.owner != null
&& symbol.owner.getKind().equals(ElementKind.CLASS)
&& symbol.owner.getQualifiedName().equals(symbol.owner.getSimpleName())
&& symbol.owner.enclClass() == null;
}

/**
* Check if a symbol comes from unannotated code.
*
Expand All @@ -123,6 +139,13 @@ public boolean isSymbolUnannotated(Symbol symbol, Config config) {
Symbol.ClassSymbol classSymbol;
if (symbol instanceof Symbol.ClassSymbol) {
classSymbol = (Symbol.ClassSymbol) symbol;
} else if (isClassFieldOfPrimitiveType(symbol)) {
// As a special case, int.class, boolean.class, etc, cause ASTHelpers.enclosingClass(...) to
// return null, even though int/boolean/etc. are technically ClassSymbols. We consider this
// class "field" of primitive types to be always unannotated. (In the future, we could check
// here for whether java.lang is in the annotated packages, but if it is, I suspect we will
// have weirder problems than this)
return true;
} else {
classSymbol = castToNonNull(ASTHelpers.enclosingClass(symbol));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -986,4 +986,99 @@ public void nullUnmarkedAndAcknowledgeRestrictiveAnnotations() {
"}")
.doTest();
}

@Test
public void nullMarkedStaticImports() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
// Flag is required for now, but might no longer be need with @NullMarked!
"-XepOpt:NullAway:AnnotatedPackages=com.uber.dontcare",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"StaticMethods.java",
"package com.uber;",
"import com.example.jspecify.future.annotations.NullMarked;",
"import org.jspecify.nullness.Nullable;",
"public final class StaticMethods {",
" private StaticMethods() {}",
" @NullMarked",
" public static Object nonNullCallee(Object o) {",
" return o;",
" }",
" @NullMarked",
" @Nullable",
" public static Object nullableCallee(@Nullable Object o) {",
" return o;",
" }",
" public static Object unmarkedCallee(@Nullable Object o) {",
" // no error, because unmarked",
" return o;",
" }",
" @Nullable",
" public static Object unmarkedNullableCallee(@Nullable Object o) {",
" return o;",
" }",
"}")
.addSourceLines(
"Test.java",
"package com.uber;",
"import static com.uber.StaticMethods.nonNullCallee;",
"import static com.uber.StaticMethods.nullableCallee;",
"import static com.uber.StaticMethods.unmarkedCallee;",
"import static com.uber.StaticMethods.unmarkedNullableCallee;",
"import com.example.jspecify.future.annotations.NullMarked;",
"import org.jspecify.nullness.Nullable;",
"@NullMarked",
"public class Test {",
" public Object getNewObject() {",
" return new Object();",
" }",
" public void test() {",
" Object o = getNewObject();",
" nonNullCallee(o).toString();",
" // BUG: Diagnostic contains: dereferenced expression nullableCallee(o) is @Nullable",
" nullableCallee(o).toString();",
" unmarkedCallee(o).toString();",
" // BUG: Diagnostic contains: dereferenced expression unmarkedNullableCallee(o) is @Nullable",
" unmarkedNullableCallee(o).toString();",
" }",
"}")
.doTest();
}

@Test
public void dotClassSanityTest() {
// Check that we do not crash while determining the nullmarked-ness of primitive.class (e.g.
// int.class)
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
// Flag is required for now, but might no longer be need with @NullMarked!
"-XepOpt:NullAway:AnnotatedPackages=com.uber.dontcare",
"-XepOpt:NullAway:AcknowledgeRestrictiveAnnotations=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import com.example.jspecify.future.annotations.NullMarked;",
"import org.jspecify.nullness.Nullable;",
"@NullMarked",
"public class Test {",
" public void takesClass(Class c) {",
" }",
" public Object test(boolean flag) {",
" takesClass(Test.class);",
" takesClass(String.class);",
" takesClass(int.class);",
" takesClass(boolean.class);",
" takesClass(float.class);",
" takesClass(void.class);",
" // NEEDED TO TRIGGER DATAFLOW:",
" return flag ? Test.class : new Object();",
" }",
"}")
.doTest();
}
}

0 comments on commit de1a12b

Please sign in to comment.