opalj
Improve interface of reachable method analysis and related states to prevent misuse
During development on #1 a triggered analysis that is based on the ReachableMethodAnalysis trait got an eager scheduler. Turns out the ReachableMethodAnalysis trait implicitly expects to only implement triggered analyses and is not (yet) compatible with eager analyses.
It expects at least some ub on the Callers property on the entity (since that would be the only time it would be triggered), but with an eager analysis that might not be the case yet when the analysis values are computed at the same time as the call graph. The rest of the analysis actually returns a dependency on the callers property just fine, even if no callers are present: https://github.com/opalj/opal/blob/c6d8224f35629750092931809f6c0b9abb1ca3b3/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala#L101
This PR updates the ReachableMethodAnalysis to stop throwing a match exception when used with an eager analysis and adds compatibility. At the same time, some other handling of the analysis is improved to add clarity or prevent further misuse (such as throwing around null values which are discouraged in Scala).
[!NOTE] This PR does not yet update the usages of the reachable method analysis to not use the deprecated method since they can be pretty expansive. Actually, the whole deprecation is up for discussion, so a second PR changing all the usages may not be created yet until we agree on a solution (which can also be revert the changes made to the public interface).
Code changes look reasonable, but I'm not sure what the purpose of an eager ReachableMethodsAnalysis would be. ReachableMethodsAnalysis is meant for analyses that only want to analyze methods that are definitely reachable, and that is only known once there is a respective Callers property.
That is true, now that I think about it. Lets look at it this way:
Initially, there is no purpose an eager ReachableMethodAnalysis could serve that a triggered one could not. But because of #112, using an eager analysis can be used as a workaround when computing properties outside of the computation of e.g. the call graph (like e.g. computing the RTACallGraphKey and then computing the string analysis from #1 in the next step).
Theoretically, we could just fix #112 and be fine. Since this code change does make sense regardless since it simply updates a class to reflect its own abilities, I'd still opt for merging this.
Agreed, but #112 should really be fixed, using eager schedulers to work around this is merely a band aid and will not be sufficient once analysis batching is implemented.
I went over this PR and tried several options, none satisfactory. I ended up moving the null based handling of methods without bodies into the only analysis that actually uses it. This avoids having Option[EOptionP[...]] or EPKs that will never be computed. It still uses null, but not as part of the interface of ReachableMethodAnalysis, but only internally in CallGraphAnalysis. This seems the cleanest solution for me for now, but please voice your opinions on this.
I think moving the null handling into the CallGraphAnalysis is a good idea. In the current version however, the null value still traverses into code from the ReachableMethodsAnalysis trait and might cause unexpected behaviour. Lets take your idea one step further.
Surprisingly, with slightly reshuffling processing of methods, the API that needs adaptation for new analyses to support methods without bodies is now as close to the "with-body" analogy as can be: The same interface, simply without a TAC EP.
Additionally, this has the benefit of removing the null value invocation alltogether. I have attached a sample patch (inline, since uploading .patch files from Linux is currently not supported on GitHub), so please have a look. If youd like me to push it, ill be happy to.
Subject: [PATCH] Reduce null escape surface in reachable method analysis
---
Index: OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala
<+>UTF-8
===================================================================
diff --git a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala b/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala
--- a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala (revision 2c2043406aa0cd849b07a9a46c2329adeeff7bcd)
+++ b/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala (revision edaf1d76c78dc223a4e2d06cdc8f4c95eb2d3c19)
@@ -5,7 +5,6 @@
package analyses
package cg
-import org.opalj.br.DeclaredMethod
import org.opalj.br.Method
import org.opalj.br.MethodDescriptor
import org.opalj.br.ObjectType
@@ -21,7 +20,6 @@
import org.opalj.br.fpcf.properties.cg.Callers
import org.opalj.br.fpcf.properties.cg.OnlyCallersWithUnknownContext
import org.opalj.fpcf.Entity
-import org.opalj.fpcf.EOptionP
import org.opalj.fpcf.EPK
import org.opalj.fpcf.EPS
import org.opalj.fpcf.InterimEUBP
@@ -29,7 +27,6 @@
import org.opalj.fpcf.InterimUBP
import org.opalj.fpcf.ProperPropertyComputationResult
import org.opalj.fpcf.PropertyBounds
-import org.opalj.fpcf.PropertyComputationResult
import org.opalj.fpcf.PropertyKind
import org.opalj.fpcf.PropertyStore
import org.opalj.fpcf.Results
@@ -121,20 +
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP118,16 @@
}
}
- override final def processMethodWithoutBody(eOptP: EOptionP[DeclaredMethod, Callers]): PropertyComputationResult = {
- processMethodWithoutBody(eOptP, null)
+ override final def processMethodWithoutBody(callContext: ContextType): ProperPropertyComputationResult = {
+ Results(new DirectCalls().partialResults(callContext, enforceCalleesResult = true))
}
override final def processMethod(
callContext: ContextType,
tacEP: EPS[Method, TACAI]
): ProperPropertyComputationResult = {
- if (tacEP ne null) {
- val state = new TACAIBasedCGState[ContextType](callContext, tacEP)
- processMethod(state, new DirectCalls())
- } else {
- Results(new DirectCalls().partialResults(callContext, enforceCalleesResult = true))
- }
+ val state = new TACAIBasedCGState[ContextType](callContext, tacEP)
+ processMethod(state, new DirectCalls())
}
protected[this] def doHandleVirtualCall(
Index: OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala
<+>UTF-8
===================================================================
diff --git a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala b/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala
--- a/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala (revision 2c2043406aa0cd849b07a9a46c2329adeeff7bcd)
+++ b/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala (revision edaf1d76c78dc223a4e2d06cdc8f4c95eb2d3c19)
@@ -27,7 +27,7 @@
* Base trait for analyses that are executed for every method that is reachable.
* The analysis is performed by `processMethod`.
*
- * Note that methods without a body are not processed unless `processMethodWithoutBody` is overridden
+ * Note that methods without a body are not processed unless `processMethodWithoutBody` is overridden.
*
* @author Florian Kuebler
*/
@@ -44,7 +44,7 @@
// we only allow defined methods
if (!declaredMethod.hasSingleDefinedMethod)
- return processMethodWithoutBody(callersEOptP);
+ return processNewContexts(callersEOptP, NoCallers) { processMethodWithoutBody };
val method = declaredMethod.definedMethod
@@ -52,34 +52,31 @@
if (method.classFile.thisType != declaredMethod.declaringClassType)
return NoResult;
- if (method.body.isEmpty)
+ if (method.body.isEmpty) {
// happens in particular for native methods
- return processMethodWithoutBody(callersEOptP);
+ return processNewContexts(callersEOptP, NoCallers) { processMethodWithoutBody };
+ };
val tacEP = propertyStore(method, TACAI.key)
if (tacEP.hasUBP && tacEP.ub.tac.isDefined) {
- processMethod(callersEOptP, NoCallers, tacEP.asEPS)
+ processMethodWithTAC(callersEOptP, NoCallers, tacEP.asEPS)
} else {
InterimPartialResult(Set(tacEP), continuationForTAC(declaredMethod))
}
}
- protected def processMethodWithoutBody(
- eOptP: EOptionP[DeclaredMethod, Callers]
- ): PropertyComputationResult = NoResult
-
- protected def processMethodWithoutBody(
- eOptP: EOptionP[DeclaredMethod, Callers],
- tacEP: EPS[Method, TACAI]
+ private def processMethodWithTAC(
+ eOptP: EOptionP[DeclaredMethod, Callers],
+ oldCallers: Callers,
+ tacEP: EPS[Method, TACAI]
): ProperPropertyComputationResult = {
- processMethod(eOptP, NoCallers, tacEP)
+ processNewContexts(eOptP, oldCallers) { processMethod(_, tacEP) }
}
- protected def processMethod(
+ private def processNewContexts(
eOptP: EOptionP[DeclaredMethod, Callers],
oldCallers: Callers,
- tacEP: EPS[Method, TACAI]
- ): ProperPropertyComputationResult = {
+ )(processMethod: ContextType => PropertyComputationResult): ProperPropertyComputationResult = {
val newCallers = if (eOptP.hasUBP) eOptP.ub else NoCallers
var results: List[ProperPropertyComputationResult] = Nil
@@ -87,15 +84,19 @@
val theCalleeContext =
if (calleeContext.hasContext) calleeContext.asInstanceOf[ContextType]
else typeIterator.newContext(eOptP.e)
- results ::= processMethod(theCalleeContext, tacEP)
+ results ::= processMethod(theCalleeContext)
}
Results(
- InterimPartialResult(Set(eOptP), continuationForCallers(newCallers, tacEP)),
+ InterimPartialResult(Set(eOptP), continuationForCallers(processNewContexts(_, newCallers)(processMethod))),
results
)
}
+ protected def processMethodWithoutBody(
+ callContext: ContextType,
+ ): PropertyComputationResult = NoResult
+
def processMethod(
callContext: ContextType,
tacEP: EPS[Method, TACAI]
@@ -106,7 +107,7 @@
)(someEPS: SomeEPS): ProperPropertyComputationResult = {
someEPS match {
case UBP(tac: TACAI) if tac.tac.isDefined =>
- processMethod(
+ processMethodWithTAC(
propertyStore(declaredMethod, Callers.key),
NoCallers,
someEPS.asInstanceOf[EPS[Method, TACAI]]
@@ -117,12 +118,8 @@
}
private[this] def continuationForCallers(
- oldCallers: Callers,
- tacEP: EPS[Method, TACAI]
- )(
- update: SomeEPS
- ): ProperPropertyComputationResult = {
- val newCallers = update.asInstanceOf[EPS[DeclaredMethod, Callers]]
- processMethod(newCallers, oldCallers, tacEP)
+ processMethod: EOptionP[DeclaredMethod, Callers] => ProperPropertyComputationResult
+ )(someEPS: SomeEPS): ProperPropertyComputationResult = {
+ processMethod(someEPS.asInstanceOf[EPS[DeclaredMethod, Callers]])
}
}
@maximilianruesch Thank you, this looks sensible. I was looking for a way to reuse the method that you now named processNewContexts, but didn't find a suitable way. You're right that it works nicely with the closure. You can push the change if you'd like to.
I had to adjust the return type of the processMethodWithoutBody slightly to be able to properly assert when it does not return a real result (since Results only accepts lists of actual results, i.e. NoResult is not allowed).
As an alternative to the current solution with Option[ProperPropertyComputationResult] we could revert to PropertyComputationResult and instead introduce isProperResult (or similar) on the latter to allow for testing if an object is NoResult (without an instance check). Let me know what you think.
Otherwise, this PR should be good to go!
It would probably be simpler to to just return something that is a ProperPropertyComputationResult but does nothing. E.g. an empty Results or InterimPartialResult.