Improved scope handling in ScopedWalker (#1308)

This PR simplifies and improves the scope handling in the `ScopedWalker`. Previously, the walker tried to enter/leave scopes with a certain heuristic when nodes were iterated or their parent changed. While this worked for most cases, it could diverge from the actual scoping that was established during a frontend run. One such example was when method declarations were declared outside the AST of a class (a common use-case in Go or C++). In this case, the walker left the record scope open after existing the function, since it did not know of its existence (only of the existence of the function scope).

I therefore changed the behaviour in a way that the walker "jumps" directly to the scope of the node, which is recorded in its `scope` variable. I suspect, that at the time of wiriting of the old behaviour, we did not have the `scope` variable yet. This now guarantees that exactly the scope that the frontend intended for the given variable is now "replayed" in the walker

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/ScopeManager.kt

@@ -681,7 +681,7 @@ class ScopeManager : ScopeProvider {
      * Handle with care, here be dragons. Should not be exposed outside of the cpg-core module.
      */
     @PleaseBeCareful
-    private fun jumpTo(scope: Scope?): Scope? {
+    internal fun jumpTo(scope: Scope?): Scope? {
         val oldScope = currentScope
         currentScope = scope
         return oldScope

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/helpers/SubgraphWalker.kt

@@ -29,14 +29,10 @@ import de.fraunhofer.aisec.cpg.ScopeManager
 import de.fraunhofer.aisec.cpg.frontends.LanguageFrontend
 import de.fraunhofer.aisec.cpg.graph.AST
 import de.fraunhofer.aisec.cpg.graph.Node
-import de.fraunhofer.aisec.cpg.graph.declarations.FunctionDeclaration
 import de.fraunhofer.aisec.cpg.graph.declarations.RecordDeclaration
-import de.fraunhofer.aisec.cpg.graph.declarations.TranslationUnitDeclaration
-import de.fraunhofer.aisec.cpg.graph.declarations.ValueDeclaration
 import de.fraunhofer.aisec.cpg.graph.edge.PropertyEdge
 import de.fraunhofer.aisec.cpg.graph.edge.PropertyEdge.Companion.checkForPropertyEdge
 import de.fraunhofer.aisec.cpg.graph.edge.PropertyEdge.Companion.unwrap
-import de.fraunhofer.aisec.cpg.graph.statements.CompoundStatement
 import de.fraunhofer.aisec.cpg.processing.strategy.Strategy
 import java.lang.annotation.AnnotationFormatError
 import java.lang.reflect.Field
@@ -45,7 +41,6 @@ import java.util.function.BiConsumer
 import java.util.function.Consumer
 import java.util.function.Predicate
 import java.util.stream.Collectors
-import org.apache.commons.lang3.tuple.MutablePair
 import org.neo4j.ogm.annotation.Relationship
 import org.slf4j.LoggerFactory
 
@@ -289,11 +284,9 @@ object SubgraphWalker {
          * Once "parent" has been visited, we continue descending into its children. First into
          * "child1", followed by "subchild". Once we are done there, we return to "child1". At this
          * point, the exit handler notifies the user that "subchild" is being exited. Afterwards we
-         * exit "child1", and after "child2" is done, "parent" is exited. This callback is important
-         * for tracking declaration scopes, as e.g. anything declared in "child1" is also visible to
-         * "subchild", but not to "child2".
+         * exit "child1", and after "child2" is done, "parent" is exited.
          */
-        private val onScopeExit: MutableList<Consumer<Node>> = ArrayList()
+        private val onNodeExit: MutableList<Consumer<Node>> = ArrayList()
 
         /**
          * The core iterative AST traversal algorithm: In a depth-first way we descend into the
@@ -313,7 +306,7 @@ object SubgraphWalker {
                         (backlog as ArrayDeque<Node>).peek() == current
                 ) {
                     val exiting = (backlog as ArrayDeque<Node>).pop()
-                    onScopeExit.forEach(Consumer { c: Consumer<Node> -> c.accept(exiting) })
+                    onNodeExit.forEach(Consumer { c: Consumer<Node> -> c.accept(exiting) })
                 } else {
                     // re-place the current node as a marker for the above check to find out when we
                     // need to exit a scope
@@ -344,13 +337,13 @@ object SubgraphWalker {
             onNodeVisit2.add(callback)
         }
 
-        fun registerOnScopeExit(callback: Consumer<Node>) {
-            onScopeExit.add(callback)
+        fun registerOnNodeExit(callback: Consumer<Node>) {
+            onNodeExit.add(callback)
         }
 
         fun clearCallbacks() {
             onNodeVisit.clear()
-            onScopeExit.clear()
+            onNodeExit.clear()
         }
 
         fun getTodo(): Deque<Node> {
@@ -359,21 +352,13 @@ object SubgraphWalker {
     }
 
     /**
-     * Handles declaration scope monitoring for iterative traversals. If this is not required, use
-     * [IterativeGraphWalker] for less overhead.
-     *
-     * Declaration scopes are similar to [de.fraunhofer.aisec.cpg.ScopeManager] scopes:
-     * [ValueDeclaration]s located inside a scope (i.e. are children of the scope root) are visible
-     * to any children of the scope root. Scopes can be layered, where declarations from parent
-     * scopes are visible to the children but not the other way around.
+     * This class traverses the graph in a similar way as the [IterativeGraphWalker], but with the
+     * added feature, that a [ScopeManager] is populated with the scope information of the current
+     * node. This way, we can call functions on the supplied [scopeManager] and emulate that we are
+     * currently in the scope of the "consumed" node in the callback. This can be useful for
+     * resolving declarations or other scope-related tasks.
      */
     class ScopedWalker {
-        // declarationScope -> (parentScope, declarations)
-        private val nodeToParentBlockAndContainedValueDeclarations:
-            MutableMap<
-                Node, org.apache.commons.lang3.tuple.Pair<Node, MutableList<ValueDeclaration>>
-            > =
-            IdentityHashMap()
         private var walker: IterativeGraphWalker? = null
         private val scopeManager: ScopeManager
 
@@ -415,81 +400,21 @@ object SubgraphWalker {
         fun iterate(root: Node) {
             walker = IterativeGraphWalker()
             handlers.forEach { h -> walker?.registerOnNodeVisit { n -> handleNode(n, h) } }
-            walker?.registerOnScopeExit { exiting: Node -> leaveScope(exiting) }
             walker?.iterate(root)
         }
 
         private fun handleNode(
             current: Node,
             handler: TriConsumer<RecordDeclaration?, Node?, Node?>
         ) {
-            scopeManager.enterScopeIfExists(current)
+            // Jump to the node's scope, if it is different from ours.
+            if (scopeManager.currentScope != current.scope) {
+                scopeManager.jumpTo(current.scope)
+            }
+
             val parent = walker?.backlog?.peek()
 
-            // TODO: actually we should not handle this in handleNode but have something similar to
-            // onScopeEnter because the method declaration already correctly sets the scope
             handler.accept(scopeManager.currentRecord, parent, current)
         }
-
-        private fun leaveScope(exiting: Node) {
-            scopeManager.leaveScope(exiting)
-        }
-
-        fun collectDeclarations(current: Node?) {
-            if (current == null) return
-
-            var parentBlock: Node? = null
-
-            // get containing Record or Compound
-            for (node in walker?.backlog ?: listOf()) {
-                if (
-                    node is RecordDeclaration ||
-                        node is CompoundStatement ||
-                        node is FunctionDeclaration ||
-                        node is
-                            TranslationUnitDeclaration // can also be a translation unit for global
-                // (C) functions
-                ) {
-                    parentBlock = node
-                    break
-                }
-            }
-            nodeToParentBlockAndContainedValueDeclarations[current] =
-                MutablePair(parentBlock, ArrayList())
-            if (current is ValueDeclaration) {
-                LOGGER.trace("Adding variable {}", current.code)
-                if (parentBlock == null) {
-                    LOGGER.warn("Parent block is empty during subgraph run")
-                } else {
-                    nodeToParentBlockAndContainedValueDeclarations[parentBlock]?.right?.add(current)
-                }
-            }
-        }
-
-        /**
-         * @param scope
-         * @param predicate
-         * @return
-         */
-        @Deprecated("""The scope manager should be used instead.
-      """)
-        fun getDeclarationForScope(
-            scope: Node,
-            predicate: Predicate<ValueDeclaration?>
-        ): Optional<out ValueDeclaration?> {
-            var currentScope = scope
-
-            // iterate all declarations from the current scope and all its parent scopes
-            while (nodeToParentBlockAndContainedValueDeclarations.containsKey(scope)) {
-                val entry = nodeToParentBlockAndContainedValueDeclarations[currentScope]
-                for (`val` in entry?.right ?: listOf()) {
-                    if (predicate.test(`val`)) {
-                        return Optional.of(`val`)
-                    }
-                }
-                entry?.left?.let { currentScope = it }
-            }
-            return Optional.empty()
-        }
     }
 }

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/CallResolver.kt

@@ -74,7 +74,6 @@ open class CallResolver(ctx: TranslationContext) : SymbolResolverPass(ctx) {
 
     override fun accept(component: Component) {
         walker = ScopedWalker(scopeManager)
-        walker.registerHandler { _, _, currNode -> walker.collectDeclarations(currNode) }
         walker.registerHandler { node, _ -> findRecords(node) }
         walker.registerHandler { node, _ -> findTemplates(node) }
         walker.registerHandler { currentClass, _, currentNode ->

cpg-core/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/VariableUsageResolver.kt

@@ -66,7 +66,6 @@ open class VariableUsageResolver(ctx: TranslationContext) : SymbolResolverPass(c
         for (tu in component.translationUnits) {
             currentTU = tu
             walker.clearCallbacks()
-            walker.registerHandler { _, _, currNode -> walker.collectDeclarations(currNode) }
             walker.registerHandler { node, _ -> findRecords(node) }
             walker.registerHandler { node, _ -> findEnums(node) }
             walker.iterate(currentTU)

cpg-language-cxx/src/main/kotlin/de/fraunhofer/aisec/cpg/passes/FunctionPointerCallResolver.kt

@@ -30,7 +30,6 @@ import de.fraunhofer.aisec.cpg.frontends.cxx.CXXLanguageFrontend
 import de.fraunhofer.aisec.cpg.graph.Component
 import de.fraunhofer.aisec.cpg.graph.Node
 import de.fraunhofer.aisec.cpg.graph.declarations.FunctionDeclaration
-import de.fraunhofer.aisec.cpg.graph.declarations.RecordDeclaration
 import de.fraunhofer.aisec.cpg.graph.declarations.VariableDeclaration
 import de.fraunhofer.aisec.cpg.graph.pointer
 import de.fraunhofer.aisec.cpg.graph.statements.expressions.*
@@ -63,9 +62,6 @@ class FunctionPointerCallResolver(ctx: TranslationContext) : ComponentPass(ctx)
     override fun accept(component: Component) {
         inferDfgForUnresolvedCalls = config.inferenceConfiguration.inferDfgForUnresolvedSymbols
         walker = ScopedWalker(scopeManager)
-        walker.registerHandler { _: RecordDeclaration?, _: Node?, currNode: Node? ->
-            walker.collectDeclarations(currNode)
-        }
         walker.registerHandler { node, _ -> resolve(node) }
 
         for (tu in component.translationUnits) {