FIR CFA: remove direction from ControlFlowGraph.traverse.

If you care about direction, then you also care about labels. 3 out of 6 places that use `traverse` are incorrect, by the way. Node order does not correspond to scoping boundaries.
2022-12-16 16:42:16 +01:00
parent de105e602d
commit 5d4b44500a
8 changed files with 25 additions and 36 deletions
@@ -27,7 +27,6 @@ import org.jetbrains.kotlin.fir.expressions.FirFunctionCall
 import org.jetbrains.kotlin.fir.expressions.FirQualifiedAccess
 import org.jetbrains.kotlin.fir.expressions.impl.FirResolvedArgumentList
 import org.jetbrains.kotlin.fir.expressions.toResolvedCallableSymbol
-import org.jetbrains.kotlin.fir.references.toResolvedCallableSymbol
 import org.jetbrains.kotlin.fir.references.FirReference
 import org.jetbrains.kotlin.fir.references.FirResolvedNamedReference
 import org.jetbrains.kotlin.fir.references.FirThisReference
@@ -71,7 +70,6 @@ object FirCallsEffectAnalyzer : FirControlFlowChecker() {

        val leakedSymbols = mutableMapOf<FirBasedSymbol<*>, MutableList<KtSourceElement>>()
        graph.traverse(
-            TraverseDirection.Forward,
            CapturedLambdaFinder(function),
            IllegalScopeContext(functionalTypeEffects.keys, leakedSymbols)
        )
@@ -163,6 +161,8 @@ object FirCallsEffectAnalyzer : FirControlFlowChecker() {
        override fun <T> visitUnionNode(node: T, data: IllegalScopeContext) where T : CFGNode<*>, T : UnionNodeMarker {}

        override fun visitFunctionEnterNode(node: FunctionEnterNode, data: IllegalScopeContext) {
+            // TODO: this is not how CFG works, this should be done by FIR tree traversal. Especially considering that
+            //  none of these methods use anything from the CFG other than `node.fir`, which should've been a hint.
            data.enterScope(node.fir === rootFunction || node.fir.isInPlaceLambda())
        }

@@ -9,10 +9,7 @@ import org.jetbrains.kotlin.contracts.description.canBeRevisited
 import org.jetbrains.kotlin.contracts.description.isDefinitelyVisited
 import org.jetbrains.kotlin.diagnostics.DiagnosticReporter
 import org.jetbrains.kotlin.diagnostics.reportOn
-import org.jetbrains.kotlin.fir.analysis.cfa.util.PathAwarePropertyInitializationInfo
 import org.jetbrains.kotlin.fir.analysis.cfa.util.PropertyInitializationInfoData
-import org.jetbrains.kotlin.fir.analysis.cfa.util.TraverseDirection
-import org.jetbrains.kotlin.fir.analysis.cfa.util.traverse
 import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors
 import org.jetbrains.kotlin.fir.declarations.utils.isLateInit
@@ -38,7 +35,7 @@ object FirPropertyInitializationAnalyzer : AbstractFirPropertyInitializationChec
    ) {
        val localProperties = properties.filterNotTo(mutableSetOf()) { it.isInitialized() }
        val reporterVisitor = PropertyReporter(data, localProperties, capturedWrites, reporter, context)
-        graph.traverse(TraverseDirection.Forward, reporterVisitor)
+        graph.traverse(reporterVisitor)
    }

    private fun FirVariableSymbol<*>.isInitialized(): Boolean {
@@ -7,32 +7,10 @@ package org.jetbrains.kotlin.fir.analysis.cfa.util

 import org.jetbrains.kotlin.fir.resolve.dfa.cfg.*

-// ------------------------------ Graph Traversal ------------------------------
-
 enum class TraverseDirection {
    Forward, Backward
 }

-fun <D> ControlFlowGraph.traverse(
-    direction: TraverseDirection,
-    visitor: ControlFlowGraphVisitor<*, D>,
-    data: D
-) {
-    for (node in getNodesInOrder(direction)) {
-        node.accept(visitor, data)
-        (node as? CFGNodeWithSubgraphs<*>)?.subGraphs?.forEach { it.traverse(direction, visitor, data) }
-    }
-}
-
-fun ControlFlowGraph.traverse(
-    direction: TraverseDirection,
-    visitor: ControlFlowGraphVisitorVoid
-) {
-    traverse(direction, visitor, null)
-}
-
-// ---------------------- Path-sensitive data collection -----------------------
-
 fun <I : ControlFlowInfo<I, *, *>> ControlFlowGraph.collectDataForNode(
    direction: TraverseDirection,
    visitor: PathAwareControlFlowGraphVisitor<I>,
@@ -14,11 +14,12 @@ import org.jetbrains.kotlin.fir.resolve.dfa.cfg.*
 import org.jetbrains.kotlin.fir.symbols.impl.FirPropertySymbol
 import org.jetbrains.kotlin.fir.symbols.impl.FirSyntheticPropertySymbol

+// TODO: this should be a FIR tree visitor, not a control flow graph visitor.
 class LocalPropertyAndCapturedWriteCollector private constructor() : ControlFlowGraphVisitorVoid() {
    companion object {
        fun collect(graph: ControlFlowGraph): Pair<Set<FirPropertySymbol>, Set<FirVariableAssignment>> {
            val collector = LocalPropertyAndCapturedWriteCollector()
-            graph.traverse(TraverseDirection.Forward, collector)
+            graph.traverse(collector)
            return collector.symbols.keys to collector.capturedWrites
        }
    }
@@ -6,8 +6,6 @@
 package org.jetbrains.kotlin.fir.analysis.checkers.declaration

 import org.jetbrains.kotlin.descriptors.Visibilities
-import org.jetbrains.kotlin.fir.analysis.cfa.util.TraverseDirection
-import org.jetbrains.kotlin.fir.analysis.cfa.util.traverse
 import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
 import org.jetbrains.kotlin.diagnostics.DiagnosticReporter
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors
@@ -34,11 +32,12 @@ object FirTailrecFunctionChecker : FirSimpleFunctionChecker() {
        }
        val graph = declaration.controlFlowGraphReference?.controlFlowGraph ?: return

+        // TODO: this is not how CFG works, tail calls inside try-catch should be detected by FIR tree traversal.
        var tryScopeCount = 0
        var catchScopeCount = 0
        var finallyScopeCount = 0
        var tailrecCount = 0
-        graph.traverse(TraverseDirection.Forward, object : ControlFlowGraphVisitorVoid() {
+        graph.traverse(object : ControlFlowGraphVisitorVoid() {
            override fun visitNode(node: CFGNode<*>) {}

            override fun <T> visitUnionNode(node: T) where T : CFGNode<*>, T : UnionNodeMarker {}
@@ -12,8 +12,6 @@ import org.jetbrains.kotlin.contracts.description.EventOccurrencesRange
 import org.jetbrains.kotlin.diagnostics.DiagnosticReporter
 import org.jetbrains.kotlin.diagnostics.reportOn
 import org.jetbrains.kotlin.fir.analysis.cfa.AbstractFirPropertyInitializationChecker
-import org.jetbrains.kotlin.fir.analysis.cfa.util.TraverseDirection
-import org.jetbrains.kotlin.fir.analysis.cfa.util.traverse
 import org.jetbrains.kotlin.fir.analysis.checkers.context.CheckerContext
 import org.jetbrains.kotlin.fir.analysis.checkers.getChildren
 import org.jetbrains.kotlin.fir.analysis.diagnostics.FirErrors
@@ -36,7 +34,7 @@ object CanBeValChecker : AbstractFirPropertyInitializationChecker() {
        val propertiesCharacteristics = mutableMapOf<FirPropertySymbol, EventOccurrencesRange>()

        val reporterVisitor = UninitializedPropertyReporter(data, properties, unprocessedProperties, propertiesCharacteristics)
-        graph.traverse(TraverseDirection.Forward, reporterVisitor)
+        graph.traverse(reporterVisitor)

        for (property in unprocessedProperties) {
            val source = property.source
@@ -40,7 +40,7 @@ object UnusedChecker : AbstractFirPropertyInitializationChecker() {
        context: CheckerContext
    ) {
        val ownData = ValueWritesWithoutReading(context.session, properties).getData(graph)
-        graph.traverse(TraverseDirection.Backward, CfaVisitor(ownData, reporter, context))
+        graph.traverse(CfaVisitor(ownData, reporter, context))
    }

    class CfaVisitor(
@@ -72,6 +72,7 @@ object UnusedChecker : AbstractFirPropertyInitializationChecker() {
            if (node.fir.source == null) return
            if (variableSymbol.isLoopIterator) return
            val dataPerNode = data[node] ?: return
+            // TODO: merge values for labels, otherwise diagnostics are inconsistent
            for (dataPerLabel in dataPerNode.values) {
                val data = dataPerLabel[variableSymbol] ?: continue

@@ -43,6 +43,21 @@ class ControlFlowGraph(val declaration: FirDeclaration?, val name: String, val k
        FakeCall,
        DefaultArgument,
    }
+
+    // NOTE: this is only for dynamic dispatch on node types. If you're collecting data from predecessors,
+    // use `collectDataForNode` instead to account for `finally` block deduplication. If you don't need that,
+    // then you probably don't need this either. Hint: if the only thing you need from nodes is the corresponding
+    // FIR structure, then traverse the `FirFile` instead.
+    fun <D> traverse(visitor: ControlFlowGraphVisitor<*, D>, data: D) {
+        for (node in nodes) {
+            node.accept(visitor, data)
+            (node as? CFGNodeWithSubgraphs<*>)?.subGraphs?.forEach { it.traverse(visitor, data) }
+        }
+    }
+
+    fun traverse(visitor: ControlFlowGraphVisitorVoid) {
+        traverse(visitor, null)
+    }
 }

 data class Edge(