fixed prematurely read issue of assignability analysis

DEVELOPING_OPAL/validate/src/test/java/org/opalj/fpcf/fixtures/immutability/openworld/assignability/advanced_counter_examples/PrematurelyReadOfFinalField.java

@@ -0,0 +1,36 @@
+/* BSD 2-Clause License - see OPAL/LICENSE for details. */
+package org.opalj.fpcf.fixtures.immutability.openworld.assignability.advanced_counter_examples;
+
+import org.opalj.fpcf.properties.immutability.field_assignability.AssignableField;
+
+/**
+ * The default value of the field x is assigned to another field n during construction and as
+ * a result seen with two different values.
+ */
+public class PrematurelyReadOfFinalField {
+
+    @AssignableField("Field n is assigned with different values.")
+    static int n = 5;
+
+    public static void main(String[] args) {
+        C c = new C();
+    }
+
+}
+
+class B {
+    B() {
+        PrematurelyReadOfFinalField.n = ((C) this).x;
+    }
+}
+
+class C extends B{
+
+    @AssignableField("Is seen with two different values during construction.")
+    public final int x;
+
+    C() {
+        super();
+        x = 3;
+    }
+}

DEVELOPING_OPAL/validate/src/test/java/org/opalj/fpcf/fixtures/immutability/openworld/assignability/advanced_counter_examples/ThisEscapesDuringConstruction.java

@@ -0,0 +1,23 @@
+/* BSD 2-Clause License - see OPAL/LICENSE for details. */
+package org.opalj.fpcf.fixtures.immutability.openworld.assignability.advanced_counter_examples;
+
+import org.opalj.fpcf.properties.immutability.field_assignability.AssignableField;
+
+/**
+ * This test case simulates the fact that the this object escapes in the constructor before (final) fields
+ * are assigned.
+ */
+public class ThisEscapesDuringConstruction {
+
+   @AssignableField("The this object escapes in the constructor before the field is assigned.")
+   final int n;
+
+    public ThisEscapesDuringConstruction(){
+        C2.m(this);
+        n = 7;
+    }
+}
+
+class C2{
+    public static void m(ThisEscapesDuringConstruction c){}
+}

DEVELOPING_OPAL/validate/src/test/java/org/opalj/fpcf/fixtures/immutability/openworld/assignability/advanced_counter_examples/ValueReadBeforeAssignment.java

@@ -0,0 +1,25 @@
+/* BSD 2-Clause License - see OPAL/LICENSE for details. */
+package org.opalj.fpcf.fixtures.immutability.openworld.assignability.advanced_counter_examples;
+
+import org.opalj.fpcf.properties.immutability.field_assignability.AssignableField;
+
+/**
+ * The value of the field x is read with its default value (0)
+ * in the constructor before assignment and assigned to a public field.
+ * Thus, the value can be accessed from everywhere.
+ */
+public class ValueReadBeforeAssignment {
+    @AssignableField("Field value is read before assignment.")
+    private int x;
+    @AssignableField("Field y is public and not final.")
+    public int y;
+
+    public ValueReadBeforeAssignment() {
+        y = x;
+        x = 42;
+    }
+
+    public ValueReadBeforeAssignment foo() {
+        return new ValueReadBeforeAssignment();
+    }
+}

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/AbstractFieldAssignabilityAnalysis.scala

@@ -128,30 +128,23 @@ trait AbstractFieldAssignabilityAnalysis extends FPCFAnalysis {
 
         implicit val state: AnalysisState = createState(field)
 
-        if (field.isFinal)
-            return Result(field, NonAssignable);
-        else
-            state.fieldAssignability = EffectivelyNonAssignable
-
-        if (field.isPublic)
-            return Result(field, Assignable);
+        state.fieldAssignability =
+            if (field.isFinal)
+                NonAssignable
+            else
+                EffectivelyNonAssignable
 
         val thisType = field.classFile.thisType
-
-        if (field.isPublic) {
-            if (typeExtensibility(ObjectType.Object).isYesOrUnknown) {
-                return Result(field, Assignable);
-            }
-        } else if (field.isProtected) {
-            if (typeExtensibility(thisType).isYesOrUnknown) {
-                return Result(field, Assignable);
-            }
-            if (!closedPackages(thisType.packageName)) {
-                return Result(field, Assignable);
+        if (!field.isFinal) {
+            if (field.isPublic) {
+                return Result(field, Assignable)
+            } else if (field.isProtected) {
+                if (typeExtensibility(thisType).isYesOrUnknown || !closedPackages(thisType.packageName)) {
+                    return Result(field, Assignable);
+                }
             }
-        }
-        if (field.isPackagePrivate) {
-            if (!closedPackages(thisType.packageName)) {
+
+            if (field.isPackagePrivate && !closedPackages(thisType.packageName)) {
                 return Result(field, Assignable);
             }
         }

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/fieldassignability/L2FieldAssignabilityAnalysis.scala

@@ -83,7 +83,7 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject)
 
     /**
      * Analyzes field writes for a single method, returning false if the field may still be
-     * effectively final and true otherwise.
+     * effectively non assignable and true otherwise.
      */
     def methodUpdatesField(
         definedMethod: DefinedMethod,
@@ -92,25 +92,24 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject)
         pc:            PC,
         receiver:      AccessReceiver
     )(implicit state: AnalysisState): Boolean = {
+
         val field = state.field
         val method = definedMethod.definedMethod
         val stmts = taCode.stmts
         val receiverVar = receiver.map(uVarForDefSites(_, taCode.pcToIndex))
 
         val index = taCode.pcToIndex(pc)
-        if (method.isInitializer) {
-            if (field.isStatic) {
-                method.isConstructor
-            } else {
-                receiverVar.isDefined && receiverVar.get.definedBy != SelfReferenceParameter
-            }
+        if (method.isInitializer && method.classFile == field.classFile) {
+            field.isStatic && method.isConstructor ||
+            receiverVar.isDefined && receiverVar.get.definedBy != SelfReferenceParameter ||
+            checkWriteDominance(definedMethod, taCode, receiverVar, index)
         } else {
             if (field.isStatic || receiverVar.isDefined && receiverVar.get.definedBy == SelfReferenceParameter) {
-                // We consider lazy initialization if there is only single write
-                // outside an initializer, so we can ignore synchronization
+                // We consider lazy initialization if there is only a single write
+                // outside an initializer, so we can ignore synchronization.
                 state.fieldAssignability == LazilyInitialized ||
                 state.fieldAssignability == UnsafelyLazilyInitialized ||
-                // A field written outside an initializer must be lazily initialized or it is assignable
+                // A field written outside an initializer must be lazily initialized, or it is assignable
                 {
                     if (considerLazyInitialization) {
                         isAssignable(index, getDefaultValues(), method, taCode)
@@ -119,14 +118,13 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject)
                 }
             } else if (receiverVar.isDefined && !referenceHasNotEscaped(receiverVar.get, stmts, definedMethod, callers)) {
                 // Here the clone pattern is determined among others
-                //
                 // note that here we assume real three address code (flat hierarchy)
 
                 // for instance fields it is okay if they are written in the
                 // constructor (w.r.t. the currently initialized object!)
 
                 // If the field that is written is not the one referred to by the
-                // self reference, it is not effectively final.
+                // self reference, it is not effectively non assignable.
 
                 // However, a method (e.g. clone) may instantiate a new object and
                 // write the field as long as that new object did not yet escape.
@@ -149,30 +147,59 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject)
         val writes = state.fieldWriteAccessDependee.get.ub.accesses
         val writesInMethod = writes.filter { w => contextProvider.contextFromId(w._1).method eq definedMethod }.toSeq
 
-        if (writesInMethod.distinctBy(_._2).size > 1)
-            return true; // Field is written in multiple locations, thus must be assignable
+        if (writesInMethod.distinctBy(_._2).size > 1) {
+            // There can be multiple assignments of final fields in the constructor
+            // in different branches
+            if (!definedMethod.definedMethod.isConstructor)
+                return true
+        }; // Otherwise: field is written in multiple locations, thus must be assignable
 
-        // If we have no information about the receiver, we soundly return
-        if (receiverVar.isEmpty)
+        // If we have no information about the receiver, we soundly return true
+        // However, a static field has no receiver
+        if (receiverVar.isEmpty && !state.field.isStatic)
             return true;
 
-        val assignedValueObject = receiverVar.get
-        if (assignedValueObject.definedBy.exists(_ < 0))
+        val assignedValueObject =
+            if (index > 0 && stmts(index).isPutStatic) {
+                stmts(index).asPutStatic.value.asVar
+            } else if (
+                index > 0 && stmts(index).isPutField && stmts(index).asPutField.value.asVar.value.isArrayValue.isYes
+            ) {
+                stmts(index).asPutField.value.asVar
+            } else
+                receiverVar.get
+
+        // If there is more than 1 definitionsite, we soundly return true
+        if (assignedValueObject.definedBy.size != 1)
+            return true;
+
+        val definitionSite = assignedValueObject.definedBy.head
+
+        if (definitionSite < -1 ||
+            (definitionSite == -1 && !definedMethod.definedMethod.isConstructor)
+        )
             return true;
 
-        val assignedValueObjectVar = stmts(assignedValueObject.definedBy.head).asAssignment.targetVar.asVar
+        val uses = if (definitionSite == -1)
+            taCode.params.thisParameter.useSites
+        else {
+            val assignedValueObjectVar = stmts(definitionSite).asAssignment.targetVar.asVar
+            if (assignedValueObjectVar != null)
+                assignedValueObjectVar.usedBy
+            else IntTrieSet.empty
+        }
 
         val fieldWriteInMethodIndex = taCode.pcToIndex(writesInMethod.head._2)
-        if (assignedValueObjectVar != null && !assignedValueObjectVar.usedBy.forall { index =>
+        if (!uses.forall { index =>
                 val stmt = stmts(index)
 
                 fieldWriteInMethodIndex == index || // The value is itself written to another object
                     // IMPROVE: Can we use field access information to care about reflective accesses here?
                     stmt.isPutField && stmt.asPutField.name != state.field.name ||
-                    stmt.isAssignment && stmt.asAssignment.targetVar == assignedValueObjectVar ||
                     stmt.isMethodCall && stmt.asMethodCall.name == "<init>" ||
                     // CHECK do we really need the taCode here?
-                    dominates(fieldWriteInMethodIndex, index, taCode)
+                    !dominates(index, fieldWriteInMethodIndex, taCode) || stmt.isArrayStore // TODO check
+
             }
         )
             return true;
@@ -256,15 +283,17 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject)
                 fieldReadAccessInformation.numIndirectAccesses - seenIndirectAccesses
             ).exists { readAccess =>
                 val method = contextProvider.contextFromId(readAccess._1).method
-                (writeAccess._1 eq method) && {
-                    val taCode = state.tacDependees(method.asDefinedMethod).ub.tac.get
 
-                    if (readAccess._3.isDefined && readAccess._3.get._2.forall(isFormalParameter)) {
-                        false
-                    } else {
-                        !dominates(writeAccess._4, taCode.pcToIndex(readAccess._2), taCode)
+                method.definedMethod.classFile != state.field.classFile ||
+                    (writeAccess._1 eq method) && {
+                        val taCode = state.tacDependees(method.asDefinedMethod).ub.tac.get
+
+                        if (readAccess._3.isDefined && readAccess._3.get._2.forall(isFormalParameter)) {
+                            false
+                        } else {
+                            !dominates(writeAccess._4, taCode.pcToIndex(readAccess._2), taCode)
+                        }
                     }
-                }
             }
         }
     }
@@ -327,7 +356,7 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject)
             val accessingMethod = contextProvider.contextFromId(w._1).method.definedMethod
             (accessingMethod ne method) && !accessingMethod.isInitializer
         }) ||
-            writes.iterator.distinctBy(_._1).size < writes.size // More than one write per method was detected
+            writes.iterator.distinctBy(_._1).size < writes.size // More than one field write per method was detected
 
         false
     }