Don't add initializers to final fields assigned by a target construct…

…or (#333)

~~Should~~Does fix na-347 (confirmed locally that na-347 passes).

src/main/java/org/checkerframework/specimin/PrunerVisitor.java

@@ -438,9 +438,13 @@ public Visitable visit(FieldDeclaration fieldDecl, Void p) {
       if (targetFields.contains(varFullName)) {
         continue;
       } else if (usedMembers.contains(varFullName)) {
-        declarator.removeInitializer();
         if (isFinal) {
-          declarator.setInitializer(getBasicInitializer(declarator.getType()));
+          if (!fieldsAssignedByTargetCtors.contains(varFullName)) {
+            declarator.removeInitializer();
+            declarator.setInitializer(getBasicInitializer(declarator.getType()));
+          }
+        } else {
+          declarator.removeInitializer();
         }
       } else {
         iterator.remove();

src/main/java/org/checkerframework/specimin/SpeciminStateVisitor.java

@@ -66,12 +66,27 @@ public abstract class SpeciminStateVisitor extends ModifierVisitor<Void> {
    */
   protected boolean insideTargetMember = false;
 
+  /**
+   * Is the visitor inside a target constructor? If this boolean is true, then {@link
+   * #insideTargetMember} is also guaranteed to be true.
+   */
+  protected boolean insideTargetCtor = false;
+
   /** The simple name of the class currently visited */
   protected @ClassGetSimpleName String className = "";
 
   /** The qualified name of the class currently being visited. */
   protected String currentClassQualifiedName = "";
 
+  /**
+   * The fully-qualified names of each field that is assigned by a target constructor. The
+   * assignments to these fields will be preserved, so Specimin needs to avoid adding an initializer
+   * for them if they are final (as it does for other, non-assigned-by-target final fields). Set by
+   * {@link TargetMethodFinderVisitor} but stored here so that it is easily available later when
+   * pruning.
+   */
+  protected final Set<String> fieldsAssignedByTargetCtors;
+
   /**
    * Constructs a new instance with the provided sets. Use this constructor only for the first
    * visitor to run.
@@ -100,6 +115,7 @@ public SpeciminStateVisitor(
     this.usedMembers = usedMembers;
     this.usedTypeElements = usedTypeElements;
     this.existingClassesToFilePath = existingClassesToFilePath;
+    this.fieldsAssignedByTargetCtors = new HashSet<>();
   }
 
   /**
@@ -117,6 +133,7 @@ public SpeciminStateVisitor(SpeciminStateVisitor previous) {
     this.insideTargetMember = previous.insideTargetMember;
     this.className = previous.className;
     this.currentClassQualifiedName = previous.currentClassQualifiedName;
+    this.fieldsAssignedByTargetCtors = previous.fieldsAssignedByTargetCtors;
   }
 
   /**
@@ -169,7 +186,10 @@ public Visitable visit(ConstructorDeclaration ctorDecl, Void p) {
     String methodQualifiedSignature = getSignature(ctorDecl);
     boolean oldInsideTargetMember = insideTargetMember;
     insideTargetMember = oldInsideTargetMember || targetMethods.contains(methodQualifiedSignature);
+    boolean oldInsideTargetCtor = insideTargetCtor;
+    insideTargetCtor = oldInsideTargetCtor || targetMethods.contains(methodQualifiedSignature);
     Visitable result = super.visit(ctorDecl, p);
+    insideTargetCtor = oldInsideTargetCtor;
     insideTargetMember = oldInsideTargetMember;
     return result;
   }

src/main/java/org/checkerframework/specimin/TargetMethodFinderVisitor.java

@@ -10,6 +10,7 @@
 import com.github.javaparser.ast.body.MethodDeclaration;
 import com.github.javaparser.ast.body.Parameter;
 import com.github.javaparser.ast.body.VariableDeclarator;
+import com.github.javaparser.ast.expr.AssignExpr;
 import com.github.javaparser.ast.expr.Expression;
 import com.github.javaparser.ast.expr.FieldAccessExpr;
 import com.github.javaparser.ast.expr.LambdaExpr;
@@ -242,6 +243,31 @@ public Visitable visit(VariableDeclarator node, Void arg) {
     return super.visit(node, arg);
   }
 
+  @Override
+  public Visitable visit(AssignExpr node, Void p) {
+    if (insideTargetCtor) {
+      // check if the LHS is a field
+      Expression lhs = node.getTarget();
+      if (lhs.isFieldAccessExpr()) {
+        FieldAccessExpr asFieldAccess = lhs.asFieldAccessExpr();
+        Expression scope = asFieldAccess.getScope();
+        if (scope.toString().equals("this")) {
+          fieldsAssignedByTargetCtors.add(
+              currentClassQualifiedName + "#" + asFieldAccess.getNameAsString());
+        }
+      } else if (lhs.isNameExpr()) {
+        // could be a field of "this"
+        NameExpr asName = lhs.asNameExpr();
+        ResolvedValueDeclaration resolved = asName.resolve();
+        if (resolved.isField()) {
+          fieldsAssignedByTargetCtors.add(
+              currentClassQualifiedName + "#" + asName.getNameAsString());
+        }
+      }
+    }
+    return super.visit(node, p);
+  }
+
   @Override
   public Visitable visit(MethodDeclaration method, Void p) {
     boolean oldInsideTargetMember = insideTargetMember;

src/test/java/org/checkerframework/specimin/FinalFieldAssignTest.java

@@ -0,0 +1,18 @@
+package org.checkerframework.specimin;
+
+import java.io.IOException;
+import org.junit.Test;
+
+/**
+ * This test checks that when Specimin targets a constructor that assigns a final field, it doesn't
+ * introduce an incorrect and unneeded assignment to that field.
+ */
+public class FinalFieldAssignTest {
+  @Test
+  public void runTest() throws IOException {
+    SpeciminTestExecutor.runTestWithoutJarPaths(
+        "finalfieldassign",
+        new String[] {"com/example/Simple.java"},
+        new String[] {"com.example.Simple#Simple(int)"});
+  }
+}

src/test/resources/finalfieldassign/expected/com/example/Simple.java

@@ -0,0 +1,13 @@
+package com.example;
+
+class Simple {
+
+    private final int x;
+
+    private final int y;
+
+    private Simple(int x) {
+        this.x = x;
+        y = 4;
+    }
+}

src/test/resources/finalfieldassign/input/com/example/Simple.java

@@ -0,0 +1,13 @@
+package com.example;
+
+class Simple {
+
+    private final int x;
+
+    private final int y;
+
+    private Simple(int x) {
+        this.x = x;
+        y = 4;
+    }
+}