Control flow graph implementation #2687

artemmukhin · 2018-07-19T14:02:38Z

Rust control flow graph implementation. Will be used for future data flow analysis and borrow checker.

sirgl · 2018-07-19T15:30:33Z

It would be nice to have tests, that covers Rust control flow construction. You can see example for java here and test data here
Probably you have to linearise cfg to do it, but I think correctness definitely worth it.
Also, as I understand, you want to attach some attributes to cfg in future, so debugging borrow checker and analysis will be simpler with good testing infrastructure (I mean, these attributes should be visible in test result too).

artemmukhin · 2018-07-20T12:41:39Z

@sirgl Good idea! What do you think about tests based on depth-first traversal traces?

sirgl · 2018-07-23T03:23:15Z

Yes, I think, that any stable traversal is enough.
It is rather hard to understand test output (especially with non direct control flow), so I would add indices to nodes and explicitly mention to what exactly node control flow is transferred if it is not trivial.

mchernyavsky · 2018-07-25T10:32:04Z

src/main/kotlin/org/rust/lang/core/Graph.kt

+    }
+}
+
+class Node<N>(val data: N,


This should be formatted according to the convention.

mchernyavsky · 2018-07-25T11:12:12Z

src/main/kotlin/org/rust/lang/core/Graph.kt

+import java.util.*
+
+class Graph<N, E>(val nodes: MutableList<Node<N>>, val edges: MutableList<Edge<E>>) {
+    constructor() : this(mutableListOf(), mutableListOf())


It's better to use default arguments.

mchernyavsky · 2018-07-25T11:44:01Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+        preds.pop()
+        assert(preds.size == oldPredsSize)
+
+        return result!!


Maybe It's better to use checkNotNull:

return checkNotNull(result) { "Optional (lazy) error message" }

This method throws IllegalStateException instead of NullPointerException.

mchernyavsky · 2018-07-25T11:48:39Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+
+private class ExitPointVisitor(
+    private val sink: (ExitPoint) -> Unit
+) : RsVisitor() {


IMO it's better to rename all o inside visit methods to more understandable names.

fedochet · 2018-07-30T14:41:13Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+        val funcExit = process(func, pred)
+        return straightLine(callExpr, funcExit, args)
+    }
+


Why there is some space? If you want to separate some functions visually, I guess you better to leave a comment about it

fedochet · 2018-07-30T15:04:21Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+    }
+
+    fun depthFirstTraversalTrace(): String =
+        graph.depthFirstTraversal(this.entry).map { it.data.text }.joinToString("\n")


I guess you should use graph.depthFirstTraversal(this.entry).joinToString("\n") { it.data.text }

fedochet · 2018-07-30T15:45:12Z

src/main/kotlin/org/rust/lang/core/Graph.kt

+        }
+
+        val nodesWithEntry = listOf(entryNode) + nodes
+        nodesWithEntry.forEach {


We should use simple for-loops when we can, as it is stated here

fedochet · 2018-07-30T15:51:04Z

src/main/kotlin/org/rust/lang/core/Graph.kt

+            Direction.INCOMING -> incomingEdges(node)
+        }
+
+    fun forEachNode(f: (Node<N, E>) -> Unit) =


Those assignments are little misleading. Maybe you should convert those to bodies?

fedochet · 2018-07-30T16:02:54Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+        val block = exprStmt.parent as? RsBlock ?: return
+        if (!(block.expr == null && block.stmtList.lastOrNull() == exprStmt)) return
+        val parent = block.parent
+        if ((parent is RsFunction || parent is RsExpr && parent.isInTailPosition) && exprStmt.expr.type != TyNever) {


This condition seems heavy; maybe we should give it some name?

fedochet · 2018-07-30T16:07:08Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+    override fun visitExprStmt(exprStmt: RsExprStmt) {
+        exprStmt.acceptChildren(this)
+        val block = exprStmt.parent as? RsBlock ?: return
+        if (!(block.expr == null && block.stmtList.lastOrNull() == exprStmt)) return


Probably this will be better looking like this:

if (block.expr != null) return if (block.stmtList.lastOrNull() != exprStmt) return

fedochet · 2018-07-30T16:12:10Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+    class TailStatement(val stmt: RsExprStmt) : ExitPoint()
+
+    companion object {
+        fun process(fn: RsFunction, sink: (ExitPoint) -> Unit) = fn.block?.acceptChildren(ExitPointVisitor(sink))


I think you should use ordinary body functions instead of this. You don't really need process to return Unit?

fedochet · 2018-07-30T16:15:59Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+     * Creates graph description written in the DOT language.
+     * Usage: copy the output into `cfg.dot` file and run `dot -Tpng cfg.dot -o cfg.png`
+     */
+    fun createDotDescription(): String {


Maybe use buildString:

/** * Creates graph description written in the DOT language. * Usage: copy the output into `cfg.dot` file and run `dot -Tpng cfg.dot -o cfg.png` */ fun createDotDescription(): String = buildString { append("digraph {\n") graph.forEachEdge { edge -> val source = edge.source val target = edge.target val sourceNode = source.data val targetNode = target.data append(" \"${source.index}: ${sourceNode.text}\" -> \"${target.index}: ${targetNode.text}\";\n") } append("}\n") }

vlad20012 · 2018-08-08T15:27:56Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+        }
+}
+
+class CFGEdgeData(val exitingScopes: MutableList<RsElement>)


Why exitingScopes is mutable? It doesn't have usages in the project (that means, at least, it isn't tested)

vlad20012 · 2018-08-08T15:30:05Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+
+        val cfgBuilder = CFGBuilder(graph, entry, fnExit)
+        val bodyExit = cfgBuilder.process(body, entry)
+        cfgBuilder.addContainedEdge(bodyExit, fnExit)


I don't like such a complex logic inside constructors and prefer constructors that only assign the fields (like rust's struct literals). Maybe we can move the logic to some factory method (like ControlFlowGraph.buildFor())

vlad20012 · 2018-08-08T15:31:31Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+    }
+
+    fun addContainedEdge(source: CFGNode, target: CFGNode) {
+        val data = CFGEdgeData(mutableListOf())


Are we really want a mutable list here? Maybe emptyList()?

vlad20012 · 2018-08-08T15:32:18Z

src/main/kotlin/org/rust/lang/core/Graph.kt

+        for (node in nodesWithEntry) {
+            pushNode(node)
+            while (stack.isNotEmpty()) {
+                val (node, iter) = stack.pop()


Name shadowing (compiler warning)

vlad20012 · 2018-08-08T15:41:27Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+    private fun addUnreachableNode(): CFGNode =
+        addNode(CFGNodeData.Unreachable, emptyList())
+
+    private fun addNode(data: CFGNodeData, preds: List<CFGNode>): CFGNode {


I found a lot of invocations of these methods with listOf()/emptyList() last argument. So maybe it's better to use vararg here?

vlad20012 · 2018-08-08T15:45:15Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+    private fun finishWithAstNode(element: RsElement, pred: CFGNode) =
+        finishWith { addAstNode(element, listOf(pred)) }
+
+    private fun withLoopScope(loopScope: LoopScope, callable: () -> Unit) {


This is enough small method that accepts a closure. I think it's an obvious candidate to be inline method.

vlad20012 · 2018-08-08T15:56:37Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+        preds.pop()
+        assert(preds.size == oldPredsSize)
+
+        return checkNotNull(result) { "Processing ended inconclusively" }


Looks like we should set result to null after this. Otherwise, we can miss a bug and return a previous result.

Now result sets to null before element.accept(this).

vlad20012 · 2018-08-08T16:00:36Z

src/test/kotlin/org/rust/lang/core/RsControlFlowGraphTest.kt

+import org.rust.lang.core.psi.RsBlock
+import org.rust.lang.core.psi.ext.descendantsOfType
+
+class RsControlFlowGraphTest : RsTestBase() {


There are very few tests. Especially, there are no tests for loop expression and loop expression with breaks. I.e. loop expr without breaks is really an infinite loop and should be the last node in CFG.

Try to run these tests with coverage and ensure all methods in CFGBuilder are executed

I've added more tests.

vlad20012 · 2018-08-08T16:02:36Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+
+        withLoopScope(loopScope) {
+            val bodyExit = process(forExpr.block, loopBack)
+            addContainedEdge(bodyExit, loopBack)


Looks like we processing for expression just like it is loop expression, that is incorrect.

vlad20012 · 2018-08-08T16:11:35Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+    override fun visitTryExpr(tryExpr: RsTryExpr) {
+        val exprExit = addAstNode(tryExpr, emptyList())
+        val checkExpr = addDummyNode(listOf(pred))
+        val expr = addAstNode(tryExpr.expr, listOf(checkExpr))


Looks like we have to process that expr too? Also, add some tests for try expr

vlad20012 · 2018-08-13T13:32:34Z

src/main/kotlin/org/rust/lang/core/ControlFlowGraph.kt

+
+typealias CFGGraph = Graph<CFGNodeData, CFGEdgeData>
+typealias CFGNode = Node<CFGNodeData, CFGEdgeData>
+typealias CFGEdge = Edge<CFGNodeData, CFGEdgeData>


CFGGraph & CFGEdge are not used anywhere

Got rid of CFGGraph. CFGEdge will be used in data-flow analysis.

vlad20012 · 2018-08-13T13:37:24Z

src/main/kotlin/org/rust/lang/core/CFGBuilder.kt

+    }
+
+    override fun visitPatBinding(patBinding: RsPatBinding) =
+        finishWithAstNode(patBinding, pred)


Looks like this method is never executed. We process RsPatBinding in visitPatIdent method (b/c RsPatBinding is a child of RsPatIdent)

Not really. It is used in the case of RsPatField. But it's enough weird. Maybe it's better to override visitPatField? and also please add a test for the "PatField" case.

vlad20012 · 2018-08-15T15:33:59Z

bors r+

2687: Control flow graph implementation r=vlad20012 a=ortem Rust control flow graph implementation. Will be used for future data flow analysis and borrow checker. Co-authored-by: ortem <ortem00@gmail.com>

bors · 2018-08-15T16:31:43Z

Build failed

continuous-integration/travis-ci/push

mchernyavsky · 2018-08-15T16:34:13Z

bors retry

2687: Control flow graph implementation r=vlad20012 a=ortem Rust control flow graph implementation. Will be used for future data flow analysis and borrow checker. Co-authored-by: ortem <ortem00@gmail.com>

bors · 2018-08-15T17:17:47Z

Build failed

continuous-integration/travis-ci/push

vlad20012 · 2018-08-15T17:38:03Z

bors retry

Undin · 2018-08-15T18:16:57Z

bors r=vlad20012

2687: Control flow graph implementation r=vlad20012 a=ortem Rust control flow graph implementation. Will be used for future data flow analysis and borrow checker. Co-authored-by: ortem <ortem00@gmail.com>

bors · 2018-08-15T18:41:14Z

Build failed

continuous-integration/travis-ci/push

Undin · 2018-08-15T18:42:27Z

bors r=vlad20012

2687: Control flow graph implementation r=vlad20012 a=ortem Rust control flow graph implementation. Will be used for future data flow analysis and borrow checker. Co-authored-by: ortem <ortem00@gmail.com>

bors · 2018-08-15T19:27:29Z

Build failed

continuous-integration/travis-ci/push

vlad20012 · 2018-08-15T19:29:42Z

bors retry

2687: Control flow graph implementation r=vlad20012 a=ortem Rust control flow graph implementation. Will be used for future data flow analysis and borrow checker. Co-authored-by: ortem <ortem00@gmail.com>

bors · 2018-08-15T20:09:07Z

Build succeeded

artemmukhin force-pushed the cfg branch from 1e0d655 to ebef647 Compare July 20, 2018 12:27

artemmukhin force-pushed the cfg branch 3 times, most recently from 8b0b5fb to bf4de0c Compare July 20, 2018 15:38

artemmukhin force-pushed the cfg branch from bf4de0c to b79a847 Compare July 23, 2018 13:28

artemmukhin requested review from mchernyavsky and fedochet July 24, 2018 15:25

mchernyavsky reviewed Jul 25, 2018

View reviewed changes

artemmukhin force-pushed the cfg branch 2 times, most recently from 56b50cb to 6aef71b Compare July 25, 2018 13:00

artemmukhin requested a review from vlad20012 July 25, 2018 13:15

artemmukhin force-pushed the cfg branch 13 times, most recently from f4f6fb9 to 6dfb899 Compare July 30, 2018 14:15

fedochet reviewed Jul 30, 2018

View reviewed changes

artemmukhin force-pushed the cfg branch 2 times, most recently from 2f5cf4a to 446db83 Compare July 31, 2018 08:39

vlad20012 reviewed Aug 8, 2018

View reviewed changes

artemmukhin force-pushed the cfg branch 4 times, most recently from d7537f0 to 3fd209d Compare August 9, 2018 13:51

vlad20012 reviewed Aug 13, 2018

View reviewed changes

artemmukhin force-pushed the cfg branch from e8916f9 to 225ecde Compare August 14, 2018 17:54

Implement Graph and ControlFlowGraph

48113e6

artemmukhin force-pushed the cfg branch from 225ecde to 48113e6 Compare August 15, 2018 15:33

bors bot merged commit 48113e6 into intellij-rust:master Aug 15, 2018

artemmukhin deleted the cfg branch August 16, 2018 09:53

farodin91 mentioned this pull request Sep 4, 2020

Control flow analysis #1612

Open

11 tasks

Control flow graph implementation #2687

Control flow graph implementation #2687

Conversation

artemmukhin commented Jul 19, 2018

sirgl commented Jul 19, 2018 • edited

artemmukhin commented Jul 20, 2018

sirgl commented Jul 23, 2018

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

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

vlad20012 Aug 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlad20012 commented Aug 15, 2018

bors bot commented Aug 15, 2018

Build failed

mchernyavsky commented Aug 15, 2018

bors bot commented Aug 15, 2018

Build failed

vlad20012 commented Aug 15, 2018

Undin commented Aug 15, 2018

bors bot commented Aug 15, 2018

Build failed

Undin commented Aug 15, 2018

bors bot commented Aug 15, 2018

Build failed

vlad20012 commented Aug 15, 2018

bors bot commented Aug 15, 2018

Build succeeded

sirgl commented Jul 19, 2018 •

edited

vlad20012 Aug 13, 2018 •

edited