-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fixed prematurely read issue of assignability analysis #190
base: develop
Are you sure you want to change the base?
Changes from all commits
d80463c
52754a0
84281b7
624d671
8244423
fef5c6d
0d76243
253c4fc
2f1d9de
62aed1f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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){} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,18 +92,17 @@ 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 | ||
|
@@ -152,24 +151,45 @@ class L2FieldAssignabilityAnalysis private[analyses] (val project: SomeProject) | |
if (writesInMethod.distinctBy(_._2).size > 1) | ||
return true; // 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 | ||
receiverVar.get | ||
|
||
// When there are more than 1 definitionsite, we soundly return true | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please fix grammar ("If there is more than one definition site, 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.isAssignment && stmt.asAssignment.targetVar == assignedValueObjectVar || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this code commented out? Was this done for debugging reasons and should be reinstantiated? Or is the commented out code wrong and not needed and should be removed? |
||
stmt.isMethodCall && stmt.asMethodCall.name == "<init>" || | ||
// CHECK do we really need the taCode here? | ||
dominates(fieldWriteInMethodIndex, index, taCode) | ||
|
@@ -256,15 +276,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) | ||
} | ||
} | ||
} | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part of the code needs documentation to understand what all of the different conditions do