Enable UnnecessaryFinal and PreferredInterfaceType EP checks (#991)

Autofix extant issues. This PR should have no impact on runtime
behavior.

build.gradle

@@ -80,6 +80,8 @@ subprojects { project ->
             check("PackageLocation", CheckSeverity.ERROR)
             check("UnnecessaryAnonymousClass", CheckSeverity.ERROR)
             check("UnusedException", CheckSeverity.ERROR)
+            check("UnnecessaryFinal", CheckSeverity.ERROR)
+            check("PreferredInterfaceType", CheckSeverity.ERROR)
             // To enable auto-patching, uncomment the line below, replace [CheckerName] with
             // the checker(s) you want to apply patches for (comma-separated), and above, disable
             // "-Werror"

jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParams.java

@@ -144,7 +144,7 @@ private Set<Integer> computeDerefParamList(int numParam, int firstParamIndex) {
     // only basic blocks which do not require p being non-null (e.g. by dereferencing it), then
     // mark p as @NonNull
     for (int i = firstParamIndex; i <= numParam; i++) {
-      final Integer param = i - 1;
+      Integer param = i - 1;
       if (!DFS.getReachableNodes(
               prunedCFG,
               ImmutableList.of(prunedCFG.entry()),

jar-infer/jar-infer-lib/src/main/java/com/uber/nullaway/jarinfer/DefinitelyDerefedParamsDriver.java

@@ -419,7 +419,7 @@ private void writeModelJAR(String outPath) throws IOException {
   //  Note: Need version compatibility check between generated stub files and when reading models
   //    StubxWriter.VERSION_0_FILE_MAGIC_NUMBER (?)
   private void writeModel(DataOutputStream out) throws IOException {
-    Map<String, String> importedAnnotations =
+    ImmutableMap<String, String> importedAnnotations =
         ImmutableMap.<String, String>builder()
             .put("Nonnull", "javax.annotation.Nonnull")
             .put("Nullable", "javax.annotation.Nullable")
@@ -527,7 +527,7 @@ private static String getAstubxSignature(IMethod mtd) {
    * @return String Unqualified type name.
    */
   private static String getSimpleTypeName(TypeReference typ) {
-    final Map<String, String> mapFullTypeName =
+    ImmutableMap<String, String> mapFullTypeName =
         ImmutableMap.<String, String>builder()
             .put("B", "byte")
             .put("C", "char")

jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/EntriesComparator.java

@@ -45,9 +45,9 @@ public static boolean compareEntriesInJars(String jarFile1, String jarFile2) thr
     Preconditions.checkArgument(jarFile2.endsWith(".jar"), "invalid jar file: " + jarFile2);
     JarFile jar1 = new JarFile(jarFile1);
     JarFile jar2 = new JarFile(jarFile2);
-    Set<String> jar1Entries =
+    ImmutableSet<String> jar1Entries =
         jar1.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
-    Set<String> jar2Entries =
+    ImmutableSet<String> jar2Entries =
         jar2.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
     return jar1Entries.equals(jar2Entries);
   }
@@ -67,9 +67,9 @@ public static boolean compareEntriesInAars(String aarFile1, String aarFile2) thr
     Preconditions.checkArgument(aarFile2.endsWith(".aar"), "invalid aar file: " + aarFile2);
     ZipFile zip1 = new ZipFile(aarFile1);
     ZipFile zip2 = new ZipFile(aarFile2);
-    Set<String> zip1Entries =
+    ImmutableSet<String> zip1Entries =
         zip1.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
-    Set<String> zip2Entries =
+    ImmutableSet<String> zip2Entries =
         zip2.stream().map(ZipEntry::getName).collect(ImmutableSet.toImmutableSet());
     if (!zip1Entries.equals(zip2Entries)) {
       return false;

jar-infer/jar-infer-lib/src/test/java/com/uber/nullaway/jarinfer/JarInferTest.java

@@ -468,12 +468,12 @@ public void jarinferOutputJarIsBytePerByteDeterministic() throws Exception {
   @Test
   public void testSignedJars() throws Exception {
     // Set test configuration paths / options
-    final String baseJarPath = "../test-java-lib-jarinfer/build/libs/test-java-lib-jarinfer.jar";
-    final String pkg = "com.uber.nullaway.jarinfer.toys.unannotated";
-    final String baseJarName = FilenameUtils.getBaseName(baseJarPath);
-    final String workingFolderPath = outputFolder.newFolder("signed_" + pkg).getAbsolutePath();
-    final String inputJarPath = workingFolderPath + "/" + baseJarName + ".jar";
-    final String outputJarPath = workingFolderPath + "/" + baseJarName + "-annotated.jar";
+    String baseJarPath = "../test-java-lib-jarinfer/build/libs/test-java-lib-jarinfer.jar";
+    String pkg = "com.uber.nullaway.jarinfer.toys.unannotated";
+    String baseJarName = FilenameUtils.getBaseName(baseJarPath);
+    String workingFolderPath = outputFolder.newFolder("signed_" + pkg).getAbsolutePath();
+    String inputJarPath = workingFolderPath + "/" + baseJarName + ".jar";
+    String outputJarPath = workingFolderPath + "/" + baseJarName + "-annotated.jar";
 
     copyAndSignJar(baseJarPath, inputJarPath);
 

library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/LibraryModelGenerator.java

@@ -106,7 +106,7 @@ private void writeToAstubx(
     if (methodRecords.isEmpty() && nullableUpperBounds.isEmpty()) {
       return;
     }
-    Map<String, String> importedAnnotations =
+    ImmutableMap<String, String> importedAnnotations =
         ImmutableMap.of(
             "NonNull", "org.jspecify.annotations.NonNull",
             "Nullable", "org.jspecify.annotations.Nullable");
@@ -208,7 +208,8 @@ public void visit(ClassOrInterfaceDeclaration cid, Void arg) {
                 }
               });
       if (this.isNullMarked) {
-        Set<Integer> nullableUpperBoundParams = getGenericTypeParameterNullableUpperBounds(cid);
+        ImmutableSet<Integer> nullableUpperBoundParams =
+            getGenericTypeParameterNullableUpperBounds(cid);
         if (!nullableUpperBoundParams.isEmpty()) {
           nullableUpperBounds.put(parentName, nullableUpperBoundParams);
         }

library-model/library-model-generator/src/main/java/com/uber/nullaway/libmodel/StubxWriter.java

@@ -45,7 +45,7 @@ public static void write(
     int numStringEntries = 0;
     Map<String, Integer> encodingDictionary = new LinkedHashMap<>();
     List<String> strings = new ArrayList<String>();
-    List<Collection<String>> keysets =
+    ImmutableList<Collection<String>> keysets =
         ImmutableList.of(
             importedAnnotations.values(),
             packageAnnotations.keySet(),

nullaway/src/main/java/com/uber/nullaway/CodeAnnotationInfo.java

@@ -80,7 +80,7 @@ public static CodeAnnotationInfo instance(Context context) {
    */
   private static boolean fromAnnotatedPackage(
       Symbol.ClassSymbol outermostClassSymbol, Config config) {
-    final String className = outermostClassSymbol.getQualifiedName().toString();
+    String className = outermostClassSymbol.getQualifiedName().toString();
     Symbol.PackageSymbol enclosingPackage = ASTHelpers.enclosingPackage(outermostClassSymbol);
     if (!config.fromExplicitlyAnnotatedPackage(className)
         && !(enclosingPackage != null
@@ -165,7 +165,7 @@ public boolean isSymbolUnannotated(Symbol symbol, Config config, @Nullable Handl
     } else {
       classSymbol = castToNonNull(ASTHelpers.enclosingClass(symbol));
     }
-    final ClassCacheRecord classCacheRecord = get(classSymbol, config, handler);
+    ClassCacheRecord classCacheRecord = get(classSymbol, config, handler);
     boolean inAnnotatedClass = classCacheRecord.isNullnessAnnotated;
     if (symbol.getKind().equals(ElementKind.METHOD)
         || symbol.getKind().equals(ElementKind.CONSTRUCTOR)) {

nullaway/src/main/java/com/uber/nullaway/ErrorBuilder.java

@@ -285,14 +285,14 @@ Description.Builder addSuppressWarningsFix(
                   + config.getAutofixSuppressionComment());
     } else {
       // need to update the existing list of warnings
-      final List<String> suppressions = Lists.newArrayList(extantSuppressWarnings.value());
+      List<String> suppressions = Lists.newArrayList(extantSuppressWarnings.value());
       suppressions.add(suppressionName);
       // find the existing annotation, so we can replace it
-      final ModifiersTree modifiers =
+      ModifiersTree modifiers =
           (suggestTree instanceof MethodTree)
               ? ((MethodTree) suggestTree).getModifiers()
               : ((VariableTree) suggestTree).getModifiers();
-      final List<? extends AnnotationTree> annotations = modifiers.getAnnotations();
+      List<? extends AnnotationTree> annotations = modifiers.getAnnotations();
       // noinspection ConstantConditions
       com.google.common.base.Optional<? extends AnnotationTree> suppressWarningsAnnot =
           Iterables.tryFind(
@@ -301,7 +301,7 @@ Description.Builder addSuppressWarningsFix(
       if (!suppressWarningsAnnot.isPresent()) {
         throw new AssertionError("something went horribly wrong");
       }
-      final String replacement =
+      String replacement =
           "@SuppressWarnings({"
               + Joiner.on(',').join(Iterables.transform(suppressions, s -> '"' + s + '"'))
               + "}) "
@@ -358,15 +358,15 @@ private boolean canBeCastToNonNull(Tree tree) {
 
   private Description.Builder addCastToNonNullFix(
       Tree suggestTree, Description.Builder builder, VisitorState state) {
-    final String fullMethodName = config.getCastToNonNullMethod();
+    String fullMethodName = config.getCastToNonNullMethod();
     if (fullMethodName == null) {
       throw new IllegalStateException("cast-to-non-null method not set");
     }
     // Add a call to castToNonNull around suggestTree:
-    final String[] parts = fullMethodName.split("\\.");
-    final String shortMethodName = parts[parts.length - 1];
-    final String replacement = shortMethodName + "(" + state.getSourceForNode(suggestTree) + ")";
-    final SuggestedFix fix =
+    String[] parts = fullMethodName.split("\\.");
+    String shortMethodName = parts[parts.length - 1];
+    String replacement = shortMethodName + "(" + state.getSourceForNode(suggestTree) + ")";
+    SuggestedFix fix =
         SuggestedFix.builder()
             .replace(suggestTree, replacement)
             .addStaticImport(fullMethodName) // ensure castToNonNull static import
@@ -383,14 +383,14 @@ private Description.Builder removeCastToNonNullFix(
     Preconditions.checkArgument(
         currTree.getKind() == Tree.Kind.METHOD_INVOCATION,
         String.format("Expected castToNonNull invocation expression, found:\n%s", currTree));
-    final MethodInvocationTree invTree = (MethodInvocationTree) currTree;
+    MethodInvocationTree invTree = (MethodInvocationTree) currTree;
     Preconditions.checkArgument(
         invTree.getArguments().contains(suggestTree),
         String.format(
             "Method invocation tree %s does not contain the expression %s as an argument being cast",
             invTree, suggestTree));
     // Remove the call to castToNonNull:
-    final SuggestedFix fix =
+    SuggestedFix fix =
         SuggestedFix.builder().replace(invTree, state.getSourceForNode(suggestTree)).build();
     return builder.addFix(fix);
   }

nullaway/src/main/java/com/uber/nullaway/NullAway.java

@@ -491,7 +491,7 @@ public Description matchAssignment(AssignmentTree tree, VisitorState state) {
       if (arraySymbol != null) {
         boolean isElementNullable = isArrayElementNullable(arraySymbol, config);
         if (!isElementNullable && mayBeNullExpr(state, expression)) {
-          final String message = "Writing @Nullable expression into array with @NonNull contents.";
+          String message = "Writing @Nullable expression into array with @NonNull contents.";
           ErrorMessage errorMessage =
               new ErrorMessage(MessageTypes.ASSIGN_NULLABLE_TO_NONNULL_ARRAY, message);
           // Future enhancements which auto-fix such warnings will require modification to this
@@ -678,7 +678,7 @@ public Description matchSwitch(SwitchTree tree, VisitorState state) {
     }
 
     if (mayBeNullExpr(state, switchSelectorExpression)) {
-      final String message =
+      String message =
           "switch expression " + state.getSourceForNode(switchSelectorExpression) + " is @Nullable";
       ErrorMessage errorMessage =
           new ErrorMessage(MessageTypes.SWITCH_EXPRESSION_NULLABLE, message);
@@ -743,10 +743,10 @@ private Description checkParamOverriding(
       @Nullable MemberReferenceTree memberReferenceTree,
       VisitorState state) {
     com.sun.tools.javac.util.List<VarSymbol> superParamSymbols = overriddenMethod.getParameters();
-    final boolean unboundMemberRef =
+    boolean unboundMemberRef =
         (memberReferenceTree != null)
             && ((JCTree.JCMemberReference) memberReferenceTree).kind.isUnbound();
-    final boolean isOverriddenMethodAnnotated =
+    boolean isOverriddenMethodAnnotated =
         !codeAnnotationInfo.isSymbolUnannotated(overriddenMethod, config, handler);
 
     // Get argument nullability for the overridden method.  If overriddenMethodArgNullnessMap[i] is
@@ -812,7 +812,7 @@ private Description checkParamOverriding(
 
     // for unbound member references, we need to adjust parameter indices by 1 when matching with
     // overridden method
-    final int startParam = unboundMemberRef ? 1 : 0;
+    int startParam = unboundMemberRef ? 1 : 0;
 
     for (int i = 0; i < superParamSymbols.size(); i++) {
       if (!Objects.equals(overriddenMethodArgNullnessMap[i], Nullness.NULLABLE)) {
@@ -832,7 +832,7 @@ private Description checkParamOverriding(
               && NullabilityUtil.lambdaParamIsImplicitlyTyped(
                   lambdaExpressionTree.getParameters().get(methodParamInd));
       if (!Nullness.hasNullableAnnotation(paramSymbol, config) && !implicitlyTypedLambdaParam) {
-        final String message =
+        String message =
             "parameter "
                 + paramSymbol.name.toString()
                 + (memberReferenceTree != null ? " of referenced method" : "")
@@ -867,7 +867,7 @@ static Trees getTreesInstance(VisitorState state) {
 
   private Nullness getMethodReturnNullness(
       Symbol.MethodSymbol methodSymbol, VisitorState state, Nullness defaultForUnannotated) {
-    final boolean isMethodAnnotated =
+    boolean isMethodAnnotated =
         !codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config, handler);
     Nullness methodReturnNullness =
         defaultForUnannotated; // Permissive default for unannotated code.
@@ -1189,7 +1189,7 @@ private boolean relevantInitializerMethodOrBlock(
         return true;
       }
 
-      final Symbol.ClassSymbol enclClassSymbol = enclosingClassSymbol(enclosingBlockPath);
+      Symbol.ClassSymbol enclClassSymbol = enclosingClassSymbol(enclosingBlockPath);
 
       // Checking for initialization is only meaningful if the full class is null-annotated, which
       // might not be the case with @NullMarked methods inside @NullUnmarked classes (note that,
@@ -1201,12 +1201,12 @@ private boolean relevantInitializerMethodOrBlock(
       }
 
       if (ASTHelpers.getSymbol(methodTree).isStatic()) {
-        Set<MethodTree> staticInitializerMethods =
+        ImmutableSet<MethodTree> staticInitializerMethods =
             castToNonNull(class2Entities.get(enclClassSymbol)).staticInitializerMethods();
         return staticInitializerMethods.size() == 1
             && staticInitializerMethods.contains(methodTree);
       } else {
-        Set<MethodTree> instanceInitializerMethods =
+        ImmutableSet<MethodTree> instanceInitializerMethods =
             castToNonNull(class2Entities.get(enclClassSymbol)).instanceInitializerMethods();
         return instanceInitializerMethods.size() == 1
             && instanceInitializerMethods.contains(methodTree);
@@ -1352,7 +1352,7 @@ private boolean fieldInitializedByPreviousInitializer(
    * @return a map from each initializer <em>i</em> to the fields known to be initialized before
    *     <em>i</em> executes
    */
-  private Multimap<Tree, Element> computeTree2Init(
+  private ImmutableMultimap<Tree, Element> computeTree2Init(
       TreePath enclosingClassPath, VisitorState state) {
     ClassTree enclosingClass = (ClassTree) enclosingClassPath.getLeaf();
     ImmutableMultimap.Builder<Tree, Element> builder = ImmutableMultimap.builder();
@@ -1477,7 +1477,7 @@ public Description matchVariable(VariableTree tree, VisitorState state) {
     if (initializer != null) {
       if (!symbol.type.isPrimitive() && !skipDueToFieldAnnotation(symbol)) {
         if (mayBeNullExpr(state, initializer)) {
-          final ErrorMessage errorMessage =
+          ErrorMessage errorMessage =
               new ErrorMessage(
                   MessageTypes.ASSIGN_FIELD_NULLABLE,
                   "assigning @Nullable expression to @NonNull field");
@@ -1654,7 +1654,7 @@ public Description matchEnhancedForLoop(EnhancedForLoopTree tree, VisitorState s
       return Description.NO_MATCH;
     }
     ExpressionTree expr = tree.getExpression();
-    final ErrorMessage errorMessage =
+    ErrorMessage errorMessage =
         new ErrorMessage(
             MessageTypes.DEREFERENCE_NULLABLE,
             "enhanced-for expression " + state.getSourceForNode(expr) + " is @Nullable");
@@ -1680,7 +1680,7 @@ private void doUnboxingCheck(VisitorState state, ExpressionTree... expressions)
       }
       if (!type.isPrimitive()) {
         if (mayBeNullExpr(state, tree)) {
-          final ErrorMessage errorMessage =
+          ErrorMessage errorMessage =
               new ErrorMessage(MessageTypes.UNBOX_NULLABLE, "unboxing of a @Nullable value");
           state.reportMatch(
               errorBuilder.createErrorDescription(
@@ -1717,7 +1717,7 @@ private Description handleInvocation(
       return Description.NO_MATCH;
     }
 
-    final boolean isMethodAnnotated =
+    boolean isMethodAnnotated =
         !codeAnnotationInfo.isSymbolUnannotated(methodSymbol, config, handler);
     // If argumentPositionNullness[i] == null, parameter i is unannotated
     Nullness[] argumentPositionNullness = new Nullness[formalParams.size()];
@@ -1872,7 +1872,7 @@ private void checkFieldInitialization(ClassTree tree, VisitorState state) {
     Symbol.ClassSymbol classSymbol = ASTHelpers.getSymbol(tree);
     class2Entities.put(classSymbol, entities);
     // set of all non-null instance fields f such that *some* constructor does not initialize f
-    Set<Symbol> notInitializedInConstructors;
+    ImmutableSet<Symbol> notInitializedInConstructors;
     SetMultimap<MethodTree, Symbol> constructorInitInfo;
     if (entities.constructors().isEmpty()) {
       constructorInitInfo = null;
@@ -2014,7 +2014,7 @@ private void addInitializedFieldsForBlock(
   private SetMultimap<MethodTree, Symbol> checkConstructorInitialization(
       FieldInitEntities entities, VisitorState state) {
     SetMultimap<MethodTree, Symbol> result = LinkedHashMultimap.create();
-    Set<Symbol> nonnullInstanceFields = entities.nonnullInstanceFields();
+    ImmutableSet<Symbol> nonnullInstanceFields = entities.nonnullInstanceFields();
     Trees trees = getTreesInstance(state);
     boolean isExternalInitClass = symbolHasExternalInitAnnotation(entities.classSymbol());
     for (MethodTree constructor : entities.constructors()) {
@@ -2027,7 +2027,7 @@ private SetMultimap<MethodTree, Symbol> checkConstructorInitialization(
         // external framework initializes fields in this case
         continue;
       }
-      Set<Element> guaranteedNonNull =
+      ImmutableSet<Element> guaranteedNonNull =
           guaranteedNonNullForConstructor(entities, state, trees, constructor);
       for (Symbol fieldSymbol : nonnullInstanceFields) {
         if (!guaranteedNonNull.contains(fieldSymbol)) {
@@ -2073,7 +2073,7 @@ private boolean constructorInvokesAnother(MethodTree constructor, VisitorState s
   }
 
   private Set<Symbol> notInitializedStatic(FieldInitEntities entities, VisitorState state) {
-    Set<Symbol> nonNullStaticFields = entities.nonnullStaticFields();
+    ImmutableSet<Symbol> nonNullStaticFields = entities.nonnullStaticFields();
     Set<Element> initializedInStaticInitializers = new LinkedHashSet<Element>();
     AccessPathNullnessAnalysis nullnessAnalysis = getNullnessAnalysis(state);
     for (BlockTree initializer : entities.staticInitializerBlocks()) {
@@ -2156,7 +2156,7 @@ private Set<Element> getSafeInitMethods(
    */
   @Nullable
   private Element getInvokeOfSafeInitMethod(
-      StatementTree stmt, final Symbol.ClassSymbol enclosingClassSymbol, VisitorState state) {
+      StatementTree stmt, Symbol.ClassSymbol enclosingClassSymbol, VisitorState state) {
     Matcher<ExpressionTree> invokeMatcher =
         (expressionTree, s) -> {
           if (!(expressionTree instanceof MethodInvocationTree)) {
@@ -2498,7 +2498,7 @@ private Description matchDereference(
       }
     }
     if (mayBeNullExpr(state, baseExpression)) {
-      final String message =
+      String message =
           "dereferenced expression " + state.getSourceForNode(baseExpression) + " is @Nullable";
       ErrorMessage errorMessage = new ErrorMessage(MessageTypes.DEREFERENCE_NULLABLE, message);