Improve interface of reachable method analysis and related states to prevent misuse #220

maximilianruesch · 2024-09-16T22:37:00Z

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:

opal/OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala

Line 101 in c6d8224

InterimPartialResult(Set(eOptP), continuationForCallers(eOptP.ub, tacEP)),

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).

errt · 2024-09-17T09:06:27Z

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.

maximilianruesch · 2024-09-17T20:53:31Z

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.

errt · 2024-09-18T04:18:16Z

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.

errt · 2024-09-18T04:20:51Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala

            results
        )
    }

+    /**
+     * @deprecated Use [[processMethod]](ContextType, Option[_]) instead


Doesn't this lead to compile errors since we have warnings-as-errors active? This should produce a deprecation warning after all for the existing subclasses, right?

That is true. I now decided to, instead of deprecating it, remove it entirely since we only do major updates for now anyways and the next release is allowed to break existing analyses. During this I will make it easier for analyses to define which portions of the reachable method analysis they implement (i.e. with / without body). This also includes a refactoring of the state handling for the only analysis that processes methods without a body but still implements TACAIBasedAnalysisState regardless (which does not really make sense)...

errt · 2024-09-19T08:54:20Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/TACAIBasedAnalysisState.scala

-        else
-            otherDependees
-    }
+    abstract override def dependees: Set[SomeEOptionP] = super.dependees ++ Some(_tacDependee).filter(_.isRefinable)


I understand the wish to make this more 'functional', but I think this creates more overhead than necessary.

errt · 2024-09-19T08:56:22Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CGState.scala

+ * @author Maximilian Rüsch
+ */
+class TACAIBasedCGState[ContextType <: Context](
+    override val callContext:                  ContextType,


This looks weird. Does this really have to be an override val? Can't it just be a constructor parameter that is passed to the CGState constructor?

errt · 2024-09-19T08:59:22Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala

-            returnResult(new DirectCalls(), enforceCalleesResult = true)(state)
+        } else {
+            val state = new CGState[ContextType](callContext)
+            Results {


Use round parentheses

errt · 2024-09-19T09:00:07Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala

+        } else {
+            val state = new CGState[ContextType](callContext)
+            Results {
+                new DirectCalls().partialResults(state.callContext, enforceCalleesResult = true)


Does this actually do anything? Doesn't look like it.
I think this produces no results at all while missing what the original returnResults call would have returned: the InterimPartialResult for the state's dependees.

It does do something: Enforce a partial callees result to be produced as it would by an empty direct calls helper instance. If you look at the previous code, it was simply calling returnResult with the state and a newly instantiated helper object. Since the state cannot have a dependee since we did not provide it any (and it does not have access to the project or propertyStore), no InterimResult could be ever produced. Thus, we can specialize returnResult to use the TACAI-Based state to use in the continuation and return our result independently here.

I will make this clearer by removing the newly created state entirely, since we only use the callContext from it anyways.

Oh, right, I didn't see that.

errt · 2024-09-19T09:01:26Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/CallGraphAnalysis.scala

-        else
-            returnResult(new DirectCalls(), enforceCalleesResult = true)(state)
+        } else {
+            val state = new CGState[ContextType](callContext)


Having two different kinds of state here looks strange. If the code below actually did anything (see other comment), I'd expect this to fail in the continuation, because the continuation would get an unexpected kind of state.

Since there is no continuation (see #220 (comment)), it cannot fail. I will remove the state entirely (see the other comment again).

errt · 2024-09-19T09:05:35Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala

-            // the method is reachable, so we analyze it!
+
+        if (callersEOptP.isFinal && callersEOptP.ub == NoCallers) {
+            return NoResult;


Shouldn't we keep the check for an illegal NoCallers interim state at least as an assertion? Right now it is not clearly visible that the method is reachable and should be analyzed.

The NoCallers interim state was illegal since the code at the top did not know the class could use the continuation without a depending caller. Since the class can invoke and handle continuations just fine without using a depender constructed from a caller, the NoCallers interim state is not illegal anymore.

But it's an illegal state in general, isn't it? I don't think call graph analyses are supposed to ever create such an interim state.

If it is an illegal state in general, then it should be asserted by the call graph generating framework and not the framework for analysis that use the call graph (even if they at least partially overlap). This class seems like the wrong area of code to make such an assertion (e.g. it would only be triggered when using such an analysis even though the result should be illegal regardless of using this class or not).

errt · 2024-09-19T09:07:31Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala

    ): ProperPropertyComputationResult = {
+        val newCallers = if (eOptP.hasUBP) eOptP.ub else NoCallers


Wouldn't it be more sensible to turn this into an if, only executing the forNewCalleeContexts if there is a UBP?

Since we need the newCallers for constructing a continuation and forNewCalleeContexts does not do anything when NoCallers is given, I view this as reasonable and less verbose as well.

errt · 2024-09-19T09:08:46Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala

-        tacEP: EPS[Method, TACAI]
+        eOptP:      EOptionP[DeclaredMethod, Callers],
+        oldCallers: Callers,
+        tacEPOpt:   Option[EPS[Method, TACAI]]


An EPS wrapped into an Option looks wrong. EPS is already a subtype of EOptionP which captures the possibility of no value being present.

It kind of depends on whether you understand EPK as an interim / refinable state (which it says it is since the class expresses it is not final) that may produce a value if awaited via a continuation OR you understand it as a simple "there is no property here and there will never be one".

Since my understanding of the class is the former, I think an Option[EPS] is more reasonable than an EOptionP here.

Well, EPK is also used for queries, just representing a pair of entity and propertykey as the name suggest.

I tried to use an EPK, but there is a problem: Since the processMethodWithoutBody is also invoked when no single defined method exists, there may be no Method to derive from the given DeclaredMethod with which you would create an EPK[Method, TACAI]. See https://github.com/opalj/opal/actions/runs/10939312309/job/30369296437?pr=220.

So we probably have to go back to using an Option, be it Option[EPS] or Option[EOptionP]. Or do you have another idea?

errt · 2024-09-19T09:13:19Z

OPAL/tac/src/main/scala/org/opalj/tac/fpcf/analyses/cg/ReachableMethodAnalysis.scala


+trait DefinedBodyReachableMethodAnalysis extends ReachableMethodAnalysis {


Since there seems to be only one ReachableMethodAnalysis that does process methods without body, might it be more sensible to switch the logic around, with the base trait not doing that and the specialized trait doing it?

I pondered this and did could not answer this question. On one hand, if the specialized trait is used far more than the base trait, something might be wrong. Additionally, the specialized trait right now restricts the abilities of the base trait (if you want to view "does not process but just returns no result" as a restriction) which feels a bit against the core principles of inheritance.

On the other hand, even if the base trait does not handle unknown bodies, it still needs to define extension points for

The unknown body specialised trait to handle such unknown bodies

A processMethod or similar that has an EOptionP as a parameter.

In short (using the flipped version): If you only extend the ReachableMethodAnalysis you should only need to implement the EPS version of processMethod since you know you have a body. If you extend the UnknownBodyReachableMethodAnalysis, you should only need to implement the EOptionP version of processMethod which can handle both scenarios.

All in all, I feel more comfortable with the current solution, but I am happy to replace it with a better one if you have some ideas.

Improve misuse handling of reachable method analysis

3f8735c

errt reviewed Sep 18, 2024

View reviewed changes

maximilianruesch force-pushed the improvement/better-error-for-reachable-method-analysis-misuse branch from eac46e6 to ec68ef8 Compare September 18, 2024 16:08

maximilianruesch changed the title ~~Improve misuse handling of reachable method analysis~~ Improve interface of reachable method analysis and related states to prevent misuse Sep 18, 2024

maximilianruesch requested a review from errt September 18, 2024 16:22

errt requested changes Sep 19, 2024

View reviewed changes

Refactor reachable method analyses and their state / TAC handling

30bec19

maximilianruesch force-pushed the improvement/better-error-for-reachable-method-analysis-misuse branch from ec68ef8 to 30bec19 Compare September 19, 2024 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve interface of reachable method analysis and related states to prevent misuse #220

Improve interface of reachable method analysis and related states to prevent misuse #220

maximilianruesch commented Sep 16, 2024

errt commented Sep 17, 2024

maximilianruesch commented Sep 17, 2024

errt commented Sep 18, 2024

errt Sep 18, 2024

maximilianruesch Sep 18, 2024

errt Sep 19, 2024

errt Sep 19, 2024

errt Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

errt Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

errt Sep 19, 2024

maximilianruesch Sep 19, 2024 •

edited

Loading

errt Sep 19, 2024

maximilianruesch Sep 19, 2024

		): ProperPropertyComputationResult = {
		val newCallers = if (eOptP.hasUBP) eOptP.ub else NoCallers


		trait DefinedBodyReachableMethodAnalysis extends ReachableMethodAnalysis {

Improve interface of reachable method analysis and related states to prevent misuse #220

Are you sure you want to change the base?

Improve interface of reachable method analysis and related states to prevent misuse #220

Conversation

maximilianruesch commented Sep 16, 2024

errt commented Sep 17, 2024

maximilianruesch commented Sep 17, 2024

errt commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilianruesch Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximilianruesch Sep 19, 2024 •

edited

Loading