From de1a12bfd9cab0edcba69e2ec87daffba6f64a51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A1zaro=20Clapp?= Date: Mon, 12 Sep 2022 19:18:38 -0400 Subject: [PATCH] Fix crash when querying null-markedness of $primitive.class expressions. (#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. --- .../com/uber/nullaway/CodeAnnotationInfo.java | 23 +++++ .../uber/nullaway/NullAwayJSpecifyTests.java | 95 +++++++++++++++++++ 2 files changed, 118 insertions(+) diff --git a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java index 3e1d093137..849d3e938d 100644 --- a/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java +++ b/nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java @@ -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. + * + *

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. * @@ -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)); } diff --git a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java index 7a66e18d1e..166da7262e 100644 --- a/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java +++ b/nullaway/src/test/java/com/uber/nullaway/NullAwayJSpecifyTests.java @@ -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(); + } }