Support records

[michaelhixson]

Add support for JEP 359: Records (Preview) to NullAway.

This depends on upstream support in Checker Framework.

I don't think that support for records in Error Prone is strictly required. There are bug patterns that will throw exceptions or print incorrect warnings when compiling code with records. However, since those bug patterns can be disabled, I don't think that NullAway depends on those issues being fixed in the same way it depends on the issues in Checker Framework being fixed.

As I mentioned in the upstream issue, my modified copy of NullAway is giving me useful feedback about records.

record Foo(String a, @Nullable String b) {}

Foo x = new Foo(null, ""); // error because `a` is non-null
Foo y = new Foo("", null); // ok
System.out.println(y.a().length()); // ok
System.out.println(y.b().length()); // error because `b` is nullable

That's a toy example, but it's also working with real examples. I changed all of my record-like classes in one project to be actual records -- 30 classes or so with a mix of nullable and non-null properties -- and the compile-time feedback regarding nulls seems to be as good as it was before. If I introduce an error like removing a null check on a property of a record before dereferencing that property, NullAway correctly identifies the error.

To give a sense of scope, these are the changes I made to NullAway to get there.

diff --git a/nullaway/src/main/java/com/uber/nullaway/NullAway.java b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
index a600c5d..bf71b88 100644
--- a/nullaway/src/main/java/com/uber/nullaway/NullAway.java
+++ b/nullaway/src/main/java/com/uber/nullaway/NullAway.java
@@ -1441,7 +1441,10 @@ public class NullAway extends BugChecker
     // set of all non-null instance fields f such that *some* constructor does not initialize f
     Set<Symbol> notInitializedInConstructors;
     SetMultimap<MethodTree, Symbol> constructorInitInfo;
-    if (entities.constructors().isEmpty()) {
+    if (classSymbol.getKind().name().equals("RECORD")) {
+      constructorInitInfo = null;
+      notInitializedInConstructors = ImmutableSet.of();
+    } else if (entities.constructors().isEmpty()) {
       constructorInitInfo = null;
       notInitializedInConstructors = entities.nonnullInstanceFields();
     } else {
@@ -1809,6 +1812,10 @@ public class NullAway extends BugChecker
           // do nothing
           break;
         default:
+          if (memberTree.getKind().name().equals("RECORD")) {
+            // do nothing
+            break;
+          }
           throw new RuntimeException(
               memberTree.getKind().toString() + " " + state.getSourceForNode(memberTree));
       }
@@ -2021,6 +2028,9 @@ public class NullAway extends BugChecker
       case ANNOTATION_TYPE:
         return false;
       default:
+        if (kind.name().equals("RECORD")) {
+          return false;
+        }
         return true;
     }
   }

Not too bad, right? I'm sure that I've missed corner cases and there'll be more to it, but for the most part it seems like records can be treated like regular classes.

[msridhar]

The approach sounds reasonable. A PR would be welcome when it makes sense. Thanks @michaelhixson

[pentlander]

I'm getting and exception when using records with JDK15. It seems like #377 would have fixed the exception:

 An unhandled exception was thrown by the Error Prone static analysis plugin.
public class JdbiFactory {
       ^
     Please report this at https://github.com/google/error-prone/issues/new and include the following:

     error-prone version: 2.4.0
     BugPattern: NullAway
     Stack Trace:
     java.lang.RuntimeException: RECORD record Foo(String bar) {}
  	at com.uber.nullaway.NullAway.collectEntities(NullAway.java:1787)
  	at com.uber.nullaway.NullAway.checkFieldInitialization(NullAway.java:1412)
  	at com.uber.nullaway.NullAway.matchClass(NullAway.java:1152)
  	at com.google.errorprone.scanner.ErrorProneScanner.processMatchers(ErrorProneScanner.java:451)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:549)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitClass(ErrorProneScanner.java:152)
  	at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCClassDecl.accept(JCTree.java:832)
  	at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:82)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:74)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:48)
  	at jdk.compiler/com.sun.source.util.TreeScanner.scan(TreeScanner.java:105)
  	at jdk.compiler/com.sun.source.util.TreeScanner.scanAndReduce(TreeScanner.java:113)
  	at jdk.compiler/com.sun.source.util.TreeScanner.visitCompilationUnit(TreeScanner.java:144)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:562)
  	at com.google.errorprone.scanner.ErrorProneScanner.visitCompilationUnit(ErrorProneScanner.java:152)
  	at jdk.compiler/com.sun.tools.javac.tree.JCTree$JCCompilationUnit.accept(JCTree.java:603)
  	at jdk.compiler/com.sun.source.util.TreePathScanner.scan(TreePathScanner.java:56)
  	at com.google.errorprone.scanner.Scanner.scan(Scanner.java:58)
  	at com.google.errorprone.scanner.ErrorProneScannerTransformer.apply(ErrorProneScannerTransformer.java:43)
  	at com.google.errorprone.ErrorProneAnalyzer.finished(ErrorProneAnalyzer.java:152)
  	at com.google.devtools.build.buildjar.javac.plugins.errorprone.ErrorPronePlugin.postFlow(ErrorPronePlugin.java:161)
  	at com.google.devtools.build.buildjar.javac.BlazeJavaCompiler.flow(BlazeJavaCompiler.java:115)
  	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.flow(JavaCompiler.java:1368)
  	at jdk.compiler/com.sun.tools.javac.main.JavaCompiler.compile(JavaCompiler.java:960)
  	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.lambda$doCall$0(JavacTaskImpl.java:104)
  	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.handleExceptions(JavacTaskImpl.java:147)
  	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.doCall(JavacTaskImpl.java:100)
  	at jdk.compiler/com.sun.tools.javac.api.JavacTaskImpl.call(JavacTaskImpl.java:94)
  	at com.google.devtools.build.buildjar.javac.BlazeJavacMain.compile(BlazeJavacMain.java:123)
  	at com.google.devtools.build.buildjar.ReducedClasspathJavaLibraryBuilder.fallback(ReducedClasspathJavaLibraryBuilder.java:103)
  	at com.google.devtools.build.buildjar.ReducedClasspathJavaLibraryBuilder.compileSources(ReducedClasspathJavaLibraryBuilder.java:65)
  	at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.compileJavaLibrary(SimpleJavaLibraryBuilder.java:110)
  	at com.google.devtools.build.buildjar.SimpleJavaLibraryBuilder.run(SimpleJavaLibraryBuilder.java:118)
  	at com.google.devtools.build.buildjar.BazelJavaBuilder.build(BazelJavaBuilder.java:97)
  	at com.google.devtools.build.buildjar.BazelJavaBuilder.parseAndBuild(BazelJavaBuilder.java:77)
  	at com.google.devtools.build.lib.worker.WorkRequestHandler.respondToRequest(WorkRequestHandler.java:115)
  	at com.google.devtools.build.lib.worker.WorkRequestHandler.lambda$createResponseThread$0(WorkRequestHandler.java:98)
  	at java.base/java.lang.Thread.run(Thread.java:832)

[msridhar]

@pentlander #377 never landed. I think it might make sense to re-open and land now. Let me re-open.