CFG: fix lastNode relating to assignments with underscores on the LHS

For example, "x, _ := a, b" would produce an incorrect CSV that branched to the next statement after evaluating "b", skipping the assignment to 'x'. We already had test coverage for function returns, so I'm reasonably confident this only affects parallel assigns, not destructuring ones like "x, y := f()".

Author: smowton

ql/src/semmle/go/controlflow/ControlFlowGraphImpl.qll

@@ -759,18 +759,11 @@ module CFG {
     override predicate lastNode(ControlFlow::Node last, Completion cmpl) {
       ControlFlowTree.super.lastNode(last, cmpl)
       or
-      exists(int nl, int nr | nl = getNumLhs() and nr = getNumRhs() |
-        last = MkAssignNode(this, nl - 1)
+      (
+        last = max(int i | | epilogueNode(i) order by i)
         or
-        not exists(MkAssignNode(this, nl - 1)) and
-        (
-          exists(ControlFlow::Node rhs | lastNode(getRhs(nr - 1), rhs, normalCompletion()) |
-            if nl = nr then last = rhs else last = MkExtractNode(this, nl - 1)
-          )
-          or
-          not exists(getRhs(nr - 1)) and
-          lastNode(getLhs(nl - 1), last, normalCompletion())
-        )
+        not exists(epilogueNode(_)) and
+        lastNode(getLastSubExprInEvalOrder(), last, normalCompletion())
       ) and
       cmpl = Done()
     }
@@ -787,7 +780,7 @@ module CFG {
           firstNode(getRhs(0), succ)
           or
           not exists(getRhs(_)) and
-          succ = epilogueNodeRanked(1)
+          succ = epilogueNodeRanked(0)
         )
       )
       or
@@ -796,6 +789,10 @@ module CFG {
         firstNode(getRhs(i + 1), succ)
       )
       or
+      not this instanceof RecvStmt and
+      lastNode(getRhs(getNumRhs() - 1), pred, normalCompletion()) and
+      succ = epilogueNodeRanked(0)
+      or
       exists(int i |
         pred = epilogueNodeRanked(i) and
         succ = epilogueNodeRanked(i + 1)
@@ -809,16 +806,17 @@ module CFG {
       )
     }
 
+    private Expr getSubExprInEvalOrder(int evalOrder) {
+      if evalOrder < getNumLhs()
+      then result = getLhs(evalOrder)
+      else result = getRhs(evalOrder - getNumLhs())
+    }
+
+    private Expr getLastSubExprInEvalOrder() {
+      result = max(int i | | getSubExprInEvalOrder(i) order by i)
+    }
+
     private ControlFlow::Node epilogueNode(int i) {
-      not this instanceof RecvStmt and
-      i = -2 and
-      (
-        lastNode(getRhs(getNumRhs() - 1), result, normalCompletion())
-        or
-        not exists(getRhs(_)) and
-        lastNode(getLhs(getNumLhs() - 1), result, normalCompletion())
-      )
-      or
       i = -1 and
       result = MkCompoundAssignRhsNode(this)
       or

ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/ControlFlowNode_getASuccessor.expected

@@ -635,7 +635,7 @@
 | main.go:79:2:79:13 | call to Print | main.go:80:1:80:1 | exit |
 | main.go:79:12:79:12 | z | main.go:79:2:79:13 | call to Print |
 | main.go:82:1:82:1 | entry | main.go:82:18:82:18 | zero value for a |
-| main.go:82:1:86:1 | function declaration | main.go:0:0:0:0 | exit |
+| main.go:82:1:86:1 | function declaration | main.go:88:6:88:23 | skip |
 | main.go:82:6:82:13 | skip | main.go:82:1:86:1 | function declaration |
 | main.go:82:18:82:18 | implicit read of a | main.go:82:25:82:25 | implicit read of b |
 | main.go:82:18:82:18 | initialization of a | main.go:82:25:82:25 | zero value for b |
@@ -655,6 +655,36 @@
 | main.go:84:11:84:12 | 19 | main.go:84:9:84:12 | ...+... |
 | main.go:84:15:84:15 | x | main.go:84:2:84:2 | assignment to x |
 | main.go:85:2:85:7 | return statement | main.go:82:18:82:18 | implicit read of a |
+| main.go:88:1:88:1 | entry | main.go:88:25:88:25 | argument corresponding to x |
+| main.go:88:1:96:1 | function declaration | main.go:0:0:0:0 | exit |
+| main.go:88:6:88:23 | skip | main.go:88:1:96:1 | function declaration |
+| main.go:88:25:88:25 | argument corresponding to x | main.go:88:25:88:25 | initialization of x |
+| main.go:88:25:88:25 | initialization of x | main.go:89:2:89:2 | skip |
+| main.go:89:2:89:2 | assignment to a | main.go:89:5:89:5 | assignment to b |
+| main.go:89:2:89:2 | skip | main.go:89:5:89:5 | skip |
+| main.go:89:5:89:5 | assignment to b | main.go:90:5:90:8 | cond |
+| main.go:89:5:89:5 | skip | main.go:89:10:89:10 | x |
+| main.go:89:10:89:10 | x | main.go:89:13:89:13 | 0 |
+| main.go:89:13:89:13 | 0 | main.go:89:2:89:2 | assignment to a |
+| main.go:90:5:90:8 | cond | main.go:90:5:90:10 | call to cond |
+| main.go:90:5:90:10 | call to cond | main.go:90:10:90:10 | call to cond is false |
+| main.go:90:5:90:10 | call to cond | main.go:90:10:90:10 | call to cond is true |
+| main.go:90:5:90:10 | call to cond | main.go:96:1:96:1 | exit |
+| main.go:90:10:90:10 | call to cond is false | main.go:93:3:93:3 | skip |
+| main.go:90:10:90:10 | call to cond is true | main.go:91:3:91:3 | skip |
+| main.go:91:3:91:3 | assignment to a | main.go:95:9:95:9 | a |
+| main.go:91:3:91:3 | skip | main.go:91:6:91:6 | skip |
+| main.go:91:6:91:6 | skip | main.go:91:10:91:10 | b |
+| main.go:91:10:91:10 | b | main.go:91:13:91:13 | a |
+| main.go:91:13:91:13 | a | main.go:91:3:91:3 | assignment to a |
+| main.go:93:3:93:3 | skip | main.go:93:6:93:6 | skip |
+| main.go:93:6:93:6 | assignment to b | main.go:95:9:95:9 | a |
+| main.go:93:6:93:6 | skip | main.go:93:10:93:10 | b |
+| main.go:93:10:93:10 | b | main.go:93:13:93:13 | a |
+| main.go:93:13:93:13 | a | main.go:93:6:93:6 | assignment to b |
+| main.go:95:2:95:12 | return statement | main.go:96:1:96:1 | exit |
+| main.go:95:9:95:9 | a | main.go:95:12:95:12 | b |
+| main.go:95:12:95:12 | b | main.go:95:2:95:12 | return statement |
 | noretfunctions.go:0:0:0:0 | entry | noretfunctions.go:3:1:6:1 | skip |
 | noretfunctions.go:3:1:6:1 | skip | noretfunctions.go:8:6:8:12 | skip |
 | noretfunctions.go:8:1:8:1 | entry | noretfunctions.go:9:2:9:8 | selection of Exit |
@@ -734,7 +764,7 @@
 | stmts2.go:12:9:12:13 | ...+... | stmts2.go:12:2:12:13 | return statement |
 | stmts2.go:12:13:12:13 | y | stmts2.go:12:9:12:13 | ...+... |
 | stmts2.go:15:1:15:1 | entry | stmts2.go:15:13:15:14 | argument corresponding to ch |
-| stmts2.go:15:1:28:1 | function declaration | stmts2.go:0:0:0:0 | exit |
+| stmts2.go:15:1:28:1 | function declaration | stmts2.go:30:6:30:12 | skip |
 | stmts2.go:15:6:15:11 | skip | stmts2.go:15:1:28:1 | function declaration |
 | stmts2.go:15:13:15:14 | argument corresponding to ch | stmts2.go:15:13:15:14 | initialization of ch |
 | stmts2.go:15:13:15:14 | initialization of ch | stmts2.go:17:13:17:14 | ch |
@@ -778,6 +808,29 @@
 | stmts2.go:25:16:25:17 | ch | stmts2.go:16:2:26:2 | select statement |
 | stmts2.go:27:2:27:9 | return statement | stmts2.go:28:1:28:1 | exit |
 | stmts2.go:27:9:27:9 | 1 | stmts2.go:27:2:27:9 | return statement |
+| stmts2.go:30:1:30:1 | entry | stmts2.go:31:2:31:2 | skip |
+| stmts2.go:30:1:34:1 | function declaration | stmts2.go:0:0:0:0 | exit |
+| stmts2.go:30:6:30:12 | skip | stmts2.go:30:1:34:1 | function declaration |
+| stmts2.go:31:2:31:2 | assignment to x | stmts2.go:31:2:31:14 | ... := ...[1] |
+| stmts2.go:31:2:31:2 | skip | stmts2.go:31:5:31:5 | skip |
+| stmts2.go:31:2:31:14 | ... := ...[0] | stmts2.go:31:2:31:2 | assignment to x |
+| stmts2.go:31:2:31:14 | ... := ...[1] | stmts2.go:32:6:32:6 | skip |
+| stmts2.go:31:5:31:5 | skip | stmts2.go:31:10:31:12 | gen |
+| stmts2.go:31:10:31:12 | gen | stmts2.go:31:10:31:14 | call to gen |
+| stmts2.go:31:10:31:14 | call to gen | stmts2.go:31:2:31:14 | ... := ...[0] |
+| stmts2.go:31:10:31:14 | call to gen | stmts2.go:34:1:34:1 | exit |
+| stmts2.go:32:6:32:6 | assignment to y | stmts2.go:32:6:32:17 | value declaration specifier[1] |
+| stmts2.go:32:6:32:6 | skip | stmts2.go:32:9:32:9 | skip |
+| stmts2.go:32:6:32:17 | value declaration specifier[0] | stmts2.go:32:6:32:6 | assignment to y |
+| stmts2.go:32:6:32:17 | value declaration specifier[1] | stmts2.go:33:9:33:9 | x |
+| stmts2.go:32:9:32:9 | skip | stmts2.go:32:13:32:15 | gen |
+| stmts2.go:32:13:32:15 | gen | stmts2.go:32:13:32:17 | call to gen |
+| stmts2.go:32:13:32:17 | call to gen | stmts2.go:32:6:32:17 | value declaration specifier[0] |
+| stmts2.go:32:13:32:17 | call to gen | stmts2.go:34:1:34:1 | exit |
+| stmts2.go:33:2:33:13 | return statement | stmts2.go:34:1:34:1 | exit |
+| stmts2.go:33:9:33:9 | x | stmts2.go:33:13:33:13 | y |
+| stmts2.go:33:9:33:13 | ...+... | stmts2.go:33:2:33:13 | return statement |
+| stmts2.go:33:13:33:13 | y | stmts2.go:33:9:33:13 | ...+... |
 | stmts3.go:0:0:0:0 | entry | stmts3.go:3:1:3:13 | skip |
 | stmts3.go:3:1:3:13 | skip | stmts3.go:5:6:5:11 | skip |
 | stmts3.go:5:1:5:1 | entry | stmts3.go:7:3:7:5 | skip |
@@ -814,7 +867,7 @@
 | stmts3.go:19:22:19:23 | skip | stmts3.go:19:23:19:23 | exit |
 | stmts4.go:0:0:0:0 | entry | stmts4.go:3:5:3:5 | skip |
 | stmts4.go:3:5:3:5 | skip | stmts4.go:3:5:3:5 | zero value for _ |
-| stmts4.go:3:5:3:5 | skip | stmts4.go:5:6:5:11 | skip |
+| stmts4.go:3:5:3:5 | zero value for _ | stmts4.go:5:6:5:11 | skip |
 | stmts4.go:5:1:5:26 | function declaration | stmts4.go:0:0:0:0 | exit |
 | stmts4.go:5:6:5:11 | skip | stmts4.go:5:1:5:26 | function declaration |
 | stmts5.go:0:0:0:0 | entry | stmts5.go:3:1:5:1 | skip |

ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/main.go

@@ -84,3 +84,13 @@ func multiRes() (a int, b float32) {
 	x, a = x+19, x
 	return
 }
+
+func fooWithUnderscores(x int) (int, int) {
+	a, b := x, 0
+	if cond() {
+		a, _ = b, a
+	} else {
+		_, b = b, a
+	}
+	return a, b
+}

ql/test/library-tests/semmle/go/controlflow/ControlFlowGraph/stmts2.go

@@ -26,3 +26,9 @@ func test14(ch chan int) int {
 	}
 	return 1
 }
+
+func test13b() int {
+	x, _ := gen()
+	var y, _ = gen()
+	return x + y
+}

ql/test/query-tests/RedundantCode/DeadStoreOfLocal/DeadStoreOfLocal.expected

@@ -27,3 +27,4 @@
 | testdata.go:488:3:488:3 | assignment to x | This definition of x is never used. |
 | testdata.go:542:3:542:3 | assignment to x | This definition of x is never used. |
 | testdata.go:580:4:580:4 | assignment to x | This definition of x is never used. |
+| testdata.go:629:3:629:4 | assignment to v1 | This definition of v1 is never used. |

ql/test/query-tests/RedundantCode/DeadStoreOfLocal/testdata.go

@@ -610,4 +610,26 @@ func _() {
 	_ = x
 }
 
+func _(v1, v2 int32) (int32, int32) {
+	if v1 > v2 {
+		v1, _ = v2, v1
+	}
+	return v1, v2
+}
+
+func _(v1, v2 int32) (int32, int32) {
+	if v1 > v2 {
+		_, v1 = v2, v1
+	}
+	return v1, v2
+}
+
+func _(v1, v2 int32) (int32, int32) {
+	if v1 > v2 {
+		v1, _ = v2, v1
+	}
+	v1, v2 = 0, 0
+	return v1, v2
+}
+
 func anyFunctionMightPanic()