Merge pull request #116 from github/aibaars/cfg-loop-post-order

CFG: make loop expressions post order
2026-02-20 00:43:44 +01:00 · 2021-02-08 09:53:25 +01:00
parent 37c4e6cbdf f2a6f3aadc
commit bde04d48a2
4 changed files with 131 additions and 108 deletions
--- a/ql/src/codeql_ruby/controlflow/internal/Completion.qll
+++ b/ql/src/codeql_ruby/controlflow/internal/Completion.qll
@@ -54,6 +54,9 @@ private predicate nestedEnsureCompletion(Completion outer, int nestLevel) {

 pragma[noinline]
 private predicate completionIsValidForStmt(AstNode n, Completion c) {
+  n instanceof In and
+  c instanceof EmptinessCompletion
+  or
  n instanceof Break and
  c = TBreakCompletion()
  or
--- a/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll
@@ -339,7 +339,7 @@ private class LeftToRightPostOrderTree extends StandardPostOrderTree, LeftToRigh
 }

 private class LeftToRightPreOrderNodes =
-  @alias or @block_parameters or @class or @do or @else or @ensure or @in or @lambda_parameters or
+  @alias or @block_parameters or @class or @do or @else or @ensure or @lambda_parameters or
      @method_parameters or @pattern or @program or @then or @undef or @yield;

 private class LeftToRightPreOrderTree extends StandardPreOrderTree, LeftToRightPreOrderNodes {
@@ -347,7 +347,6 @@ private class LeftToRightPreOrderTree extends StandardPreOrderTree, LeftToRightP
    this instanceof BlockParameters or
    this instanceof Do or
    this instanceof Else or
-    this instanceof In or
    this instanceof LambdaParameters or
    this instanceof MethodParameters or
    this instanceof Pattern or
@@ -593,66 +592,71 @@ module Trees {
   * ```
   *           args
   *            |
-   *          for------<-----
+   *           in------<-----
   *           / \           \
   *          /   \          |
   *         /     \         |
   *        /       \        |
   *     empty    non-empty  |
   *       |          \      |
-   *  puts "done"      \     |
-   *                  arg    |
-   *                    |    |
-   *                puts arg |
+   *      for          \     |
+   *       |          arg    |
+   *       |            |    |
+   *  puts "done"   puts arg |
   *                     \___/
   * ```
   */
-  private class ForTree extends ControlFlowTree, For {
+  private class ForTree extends PostOrderTree, For {
    final override predicate propagatesAbnormal(AstNode child) {
-      child = this.getPattern() or child = this.getValue()
+      child = this.getPattern() or child = this.getArray()
    }

-    final override predicate first(AstNode node) { node = this.getValue() }
+    final override predicate first(AstNode first) { first(this.getArray(), first) }

-    final override predicate last(AstNode last, Completion c) {
-      last = this and
-      c.(EmptinessCompletion).getValue() = true
-      or
-      last(this.getBody(), last, c) and
-      not c.continuesLoop() and
-      not c instanceof BreakCompletion and
-      not c instanceof RedoCompletion
-      or
-      last(this.getBody(), last, c.(NestedBreakCompletion).getAnInnerCompatibleCompletion())
-    }
+    private In getIn() { result = this.getValue() }
+
+    private UnderscoreArg getArray() { result = this.getValue().getChild() }

    /**
-     * for pattern in value do body end
+     * for pattern in array do body end
     * ```
-     * value +-> for +--[non empty]--> pattern -> body -> for
-     *               |--[empty]--> exit
+     * array +-> in +--[non empty]--> pattern -> body -> in
+     *              |--[empty]--> for
     * ```
     */
    final override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      last(this.getValue(), pred, c) and
-      succ = this and
+      last(this.getArray(), pred, c) and
+      first(this.getIn(), succ) and
      c instanceof SimpleCompletion
      or
-      pred = this and
+      last(this.getIn(), pred, c) and
      first(this.getPattern(), succ) and
      c.(EmptinessCompletion).getValue() = false
      or
-      first(this.getBody(), succ) and
      last(this.getPattern(), pred, c) and
+      first(this.getBody(), succ) and
      c instanceof NormalCompletion
      or
      last(this.getBody(), pred, c) and
-      succ = this and
+      first(this.getIn(), succ) and
      c.continuesLoop()
      or
      last(this.getBody(), pred, c) and
      first(this.getBody(), succ) and
      c instanceof RedoCompletion
+      or
+      succ = this and
+      (
+        last(this.getIn(), pred, c) and
+        c.(EmptinessCompletion).getValue() = true
+        or
+        last(this.getBody(), pred, c) and
+        not c.continuesLoop() and
+        not c instanceof BreakCompletion and
+        not c instanceof RedoCompletion
+        or
+        last(this.getBody(), pred, c.(NestedBreakCompletion).getAnInnerCompatibleCompletion())
+      )
    }
  }

@@ -712,6 +716,8 @@ module Trees {
    }
  }

+  private class InTree extends LeafTree, In { }
+
  private class InstanceVariableTree extends LeafTree, InstanceVariable { }

  private class IntegerTree extends LeafTree, Integer { }
@@ -1224,26 +1230,12 @@ module Trees {
    }
  }

-  private class ConditionalLoopTree extends PreOrderTree, ConditionalLoopAstNode {
+  private class ConditionalLoopTree extends PostOrderTree, ConditionalLoopAstNode {
    final override predicate propagatesAbnormal(AstNode child) { child = this.getConditionNode() }

-    final override predicate last(AstNode last, Completion c) {
-      last(this.getConditionNode(), last, c) and
-      this.endLoop(c)
-      or
-      last(this.getBodyNode(), last, c) and
-      not c.continuesLoop() and
-      not c instanceof BreakCompletion and
-      not c instanceof RedoCompletion
-      or
-      last(this.getBodyNode(), last, c.(NestedBreakCompletion).getAnInnerCompatibleCompletion())
-    }
+    final override predicate first(AstNode first) { first(this.getConditionNode(), first) }

    final override predicate succ(AstNode pred, AstNode succ, Completion c) {
-      pred = this and
-      first(this.getConditionNode(), succ) and
-      c instanceof SimpleCompletion
-      or
      last(this.getConditionNode(), pred, c) and
      this.continueLoop(c) and
      first(this.getBodyNode(), succ)
@@ -1255,6 +1247,19 @@ module Trees {
      last(this.getBodyNode(), pred, c) and
      first(this.getBodyNode(), succ) and
      c instanceof RedoCompletion
+      or
+      succ = this and
+      (
+        last(this.getConditionNode(), pred, c) and
+        this.endLoop(c)
+        or
+        last(this.getBodyNode(), pred, c) and
+        not c.continuesLoop() and
+        not c instanceof BreakCompletion and
+        not c instanceof RedoCompletion
+        or
+        last(this.getBodyNode(), pred, c.(NestedBreakCompletion).getAnInnerCompatibleCompletion())
+      )
    }
  }
 }
--- a/ql/test/library-tests/controlflow/graph/Cfg.expected
+++ b/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -182,17 +182,20 @@ break_ensure.rb:
 #-----|  -> elements

 #    2| for ... in ...
-#-----| non-empty -> element
-#-----| empty -> Ensure
+#-----|  -> Ensure

 #    2| element
 #-----|  -> element

+#    2| In
+#-----| empty -> for ... in ...
+#-----| non-empty -> element
+
 #    2| elements
-#-----|  -> for ... in ...
+#-----|  -> In

 #    3| if ...
-#-----|  -> for ... in ...
+#-----|  -> In

 #    3| ... > ...
 #-----| false -> if ...
@@ -205,7 +208,7 @@ break_ensure.rb:
 #-----|  -> ... > ...

 #    4| Break
-#-----| break -> Ensure
+#-----| break -> for ... in ...

 #    7| Ensure
 #-----|  -> elements
@@ -242,14 +245,17 @@ break_ensure.rb:
 #-----|  -> elements

 #   14| for ... in ...
-#-----| non-empty -> element
-#-----| empty -> exit m2 (normal)
+#-----|  -> exit m2 (normal)

 #   14| element
 #-----|  -> element

+#   14| In
+#-----| empty -> for ... in ...
+#-----| non-empty -> element
+
 #   14| elements
-#-----|  -> for ... in ...
+#-----|  -> In

 #   16| if ...
 #-----|  -> Ensure
@@ -274,10 +280,10 @@ break_ensure.rb:
 #-----|  -> [ensure: break] elements

 #   20| if ...
-#-----|  -> for ... in ...
+#-----|  -> In

 #   20| [ensure: break] if ...
-#-----| break -> exit m2 (normal)
+#-----| break -> for ... in ...

 #   20| call to nil?
 #-----| false -> if ...
@@ -349,11 +355,9 @@ break_ensure.rb:
 #-----|  -> [ensure: return] elements

 #   33| for ... in ...
-#-----| non-empty -> element
-#-----| empty -> puts
+#-----|  -> puts

 #   33| [ensure: return] for ... in ...
-#-----| non-empty -> [ensure: return] element
 #-----| return -> exit m3 (normal)

 #   33| element
@@ -362,17 +366,25 @@ break_ensure.rb:
 #   33| [ensure: return] element
 #-----|  -> [ensure: return] call to x

+#   33| In
+#-----| empty -> for ... in ...
+#-----| non-empty -> element
+
+#   33| [ensure: return] In
+#-----| empty -> [ensure: return] for ... in ...
+#-----| non-empty -> [ensure: return] element
+
 #   33| elements
-#-----|  -> for ... in ...
+#-----|  -> In

 #   33| [ensure: return] elements
-#-----|  -> [ensure: return] for ... in ...
+#-----|  -> [ensure: return] In

 #   35| if ...
-#-----|  -> for ... in ...
+#-----|  -> In

 #   35| [ensure: return] if ...
-#-----|  -> [ensure: return] for ... in ...
+#-----|  -> [ensure: return] In

 #   35| ... > ...
 #-----| false -> if ...
@@ -395,10 +407,10 @@ break_ensure.rb:
 #-----|  -> [ensure: return] ... > ...

 #   36| Break
-#-----| break -> puts
+#-----| break -> for ... in ...

 #   36| [ensure: return] Break
-#-----| return -> exit m3 (normal)
+#-----| break -> [ensure: return] for ... in ...

 #   41| call to puts
 #-----|  -> exit m3 (normal)
@@ -419,14 +431,17 @@ break_ensure.rb:
 #-----|  -> elements

 #   45| for ... in ...
-#-----| non-empty -> element
-#-----| empty -> exit m4 (normal)
+#-----|  -> exit m4 (normal)

 #   45| element
 #-----|  -> element

+#   45| In
+#-----| empty -> for ... in ...
+#-----| non-empty -> element
+
 #   45| elements
-#-----|  -> for ... in ...
+#-----|  -> In

 #   47| if ...
 #-----|  -> Ensure
@@ -457,10 +472,10 @@ break_ensure.rb:
 #-----|  -> [ensure: raise] element

 #   51| if ...
-#-----|  -> for ... in ...
+#-----|  -> In

 #   51| [ensure: raise] if ...
-#-----| raise -> exit m4 (abnormal)
+#-----| raise -> for ... in ...

 #   51| ... > ...
 #-----| false -> if ...
@@ -483,10 +498,10 @@ break_ensure.rb:
 #-----|  -> [ensure: raise] ... > ...

 #   52| Break
-#-----| break -> exit m4 (normal)
+#-----| break -> for ... in ...

 #   52| [ensure: raise] Break
-#-----| break -> exit m4 (normal)
+#-----| break -> for ... in ...

 #   52| 10
 #-----|  -> Break
@@ -683,7 +698,7 @@ cfg.rb:
 #-----|  -> &...

 #   29| call to new
-#-----|  -> while ...
+#-----|  -> true

 #   29| Proc
 #-----|  -> new
@@ -707,13 +722,13 @@ cfg.rb:
 #-----|  -> call to call

 #   31| while ...
-#-----|  -> true
+#-----|  -> false

 #   31| true
 #-----| true -> 1

 #   32| Break
-#-----| break -> false
+#-----| break -> while ...

 #   32| 1
 #-----|  -> Break
@@ -1131,14 +1146,17 @@ cfg.rb:
 #-----|  -> Interpolation

 #   90| for ... in ...
-#-----| non-empty -> x
-#-----| empty -> 42
+#-----|  -> 42

 #   90| x
 #-----|  -> x

+#   90| In
+#-----| empty -> for ... in ...
+#-----| non-empty -> x
+
 #   90| Array
-#-----|  -> for ... in ...
+#-----|  -> In

 #   90| 1.4
 #-----|  -> 2.5
@@ -1163,10 +1181,10 @@ cfg.rb:
 #-----|  -> ... > ...

 #   91| Next
-#-----| next -> for ... in ...
+#-----| next -> In

 #   92| call to puts
-#-----|  -> for ... in ...
+#-----|  -> In

 #   92| puts
 #-----|  -> x
@@ -1824,7 +1842,7 @@ cfg.rb:
 #-----|  -> call to puts

 #  171| ... unless ...
-#-----|  -> until ...
+#-----|  -> x

 #  171| call to puts
 #-----|  -> ... unless ...
@@ -1846,11 +1864,11 @@ cfg.rb:
 #-----|  -> ... == ...

 #  173| until ...
-#-----|  -> x
+#-----|  -> 0

 #  173| ... > ...
+#-----| true -> until ...
 #-----| false -> x
-#-----| true -> 0

 #  173| x
 #-----|  -> 10
@@ -1877,7 +1895,7 @@ cfg.rb:
 #-----|  -> call to puts

 #  175| ... = ...
-#-----|  -> ... until ...
+#-----|  -> i

 #  175| i
 #-----|  -> ... = ...
@@ -1886,7 +1904,7 @@ cfg.rb:
 #-----|  -> i

 #  176| ... until ...
-#-----|  -> i
+#-----|  -> 0

 #  176| (...; ...)
 #-----|  -> i
@@ -1910,8 +1928,8 @@ cfg.rb:
 #-----|  -> ... += ...

 #  176| ... == ...
+#-----| true -> ... until ...
 #-----| false -> puts
-#-----| true -> 0

 #  176| i
 #-----|  -> 10
@@ -1920,7 +1938,7 @@ cfg.rb:
 #-----|  -> ... == ...

 #  178| ... = ...
-#-----|  -> while ...
+#-----|  -> x

 #  178| x
 #-----|  -> ... = ...
@@ -1929,11 +1947,11 @@ cfg.rb:
 #-----|  -> x

 #  179| while ...
-#-----|  -> x
+#-----|  -> i

 #  179| ... < ...
+#-----| false -> while ...
 #-----| true -> x
-#-----| false -> ... while ...

 #  179| x
 #-----|  -> 10
@@ -1976,7 +1994,7 @@ cfg.rb:
 #-----|  -> call to puts

 #  185| ... while ...
-#-----|  -> i
+#-----|  -> run_block

 #  185| (...; ...)
 #-----|  -> i
@@ -2000,8 +2018,8 @@ cfg.rb:
 #-----|  -> ... -= ...

 #  185| ... != ...
+#-----| false -> ... while ...
 #-----| true -> puts
-#-----| false -> run_block

 #  185| i
 #-----|  -> 0
@@ -2523,14 +2541,14 @@ loops.rb:
 #-----|  -> m1

 #    1| x
-#-----|  -> while ...
-
-#    2| while ...
 #-----|  -> x

+#    2| while ...
+#-----|  -> exit m1 (normal)
+
 #    2| ... >= ...
+#-----| false -> while ...
 #-----| true -> puts
-#-----| false -> exit m1 (normal)

 #    2| x
 #-----|  -> 0
@@ -2563,14 +2581,14 @@ loops.rb:
 #-----|  -> m2

 #    8| x
-#-----|  -> while ...
-
-#    9| while ...
 #-----|  -> x

+#    9| while ...
+#-----|  -> puts
+
 #    9| ... >= ...
+#-----| false -> while ...
 #-----| true -> puts
-#-----| false -> puts

 #    9| x
 #-----|  -> 0
@@ -2610,7 +2628,7 @@ loops.rb:
 #-----|  -> ... > ...

 #   13| Break
-#-----| break -> puts
+#-----| break -> while ...

 #   14| elsif ...
 #-----|  -> if ...
@@ -3878,9 +3896,6 @@ break_ensure.rb:
 #   27| exit m3 (normal)
 #-----|  -> exit m3

-#   44| exit m4 (abnormal)
-#-----|  -> exit m4
-
 #   44| exit m4 (normal)
 #-----|  -> exit m4

--- a/ql/test/library-tests/variables/ssa.expected
+++ b/ql/test/library-tests/variables/ssa.expected
@@ -58,8 +58,8 @@ definition
 | ssa.rb:21:5:21:10 | ... -= ... | ssa.rb:18:8:18:8 | x |
 | ssa.rb:25:1:30:3 | <uninitialized> | ssa.rb:26:7:26:10 | elem |
 | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements |
-| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem |
 | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem |
+| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem |
 | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x |
 | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 |
 | ssa.rb:44:1:47:3 | <uninitialized> | ssa.rb:45:3:45:3 | x |
@@ -147,8 +147,8 @@ read
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:20:10:20:10 | x |
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:21:5:21:5 | x |
 | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | ssa.rb:26:15:26:22 | elements |
-| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem |
 | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | ssa.rb:27:10:27:13 | elem |
+| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem |
 | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | ssa.rb:34:10:34:10 | x |
 | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | ssa.rb:41:8:41:9 | m3 |
 | ssa.rb:44:8:44:8 | b | ssa.rb:44:8:44:8 | b | ssa.rb:45:12:45:12 | b |
@@ -217,8 +217,8 @@ firstRead
 | ssa.rb:10:5:10:9 | ... = ... | ssa.rb:2:3:2:3 | i | ssa.rb:11:10:11:10 | i |
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:19:9:19:9 | x |
 | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | ssa.rb:26:15:26:22 | elements |
-| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem |
 | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | ssa.rb:27:10:27:13 | elem |
+| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem |
 | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | ssa.rb:34:10:34:10 | x |
 | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | ssa.rb:41:8:41:9 | m3 |
 | ssa.rb:44:8:44:8 | b | ssa.rb:44:8:44:8 | b | ssa.rb:45:12:45:12 | b |
@@ -287,8 +287,8 @@ lastRead
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:19:9:19:9 | x |
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:21:5:21:5 | x |
 | ssa.rb:25:8:25:15 | elements | ssa.rb:25:8:25:15 | elements | ssa.rb:26:15:26:22 | elements |
-| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem |
 | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem | ssa.rb:27:10:27:13 | elem |
+| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:29:8:29:11 | elem |
 | ssa.rb:33:20:33:20 | x | ssa.rb:33:20:33:20 | x | ssa.rb:34:10:34:10 | x |
 | ssa.rb:40:3:40:9 | ... = ... | ssa.rb:40:3:40:4 | m3 | ssa.rb:41:8:41:9 | m3 |
 | ssa.rb:44:8:44:8 | b | ssa.rb:44:8:44:8 | b | ssa.rb:45:12:45:12 | b |
@@ -330,8 +330,8 @@ phi
 | ssa.rb:5:3:13:5 | phi | ssa.rb:2:3:2:3 | i | ssa.rb:10:5:10:9 | ... = ... |
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:18:8:18:8 | x |
 | ssa.rb:19:9:19:9 | phi | ssa.rb:18:8:18:8 | x | ssa.rb:21:5:21:10 | ... -= ... |
-| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:25:1:30:3 | <uninitialized> |
-| ssa.rb:26:3:28:5 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem |
+| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:25:1:30:3 | <uninitialized> |
+| ssa.rb:26:12:26:22 | phi | ssa.rb:26:7:26:10 | elem | ssa.rb:26:7:26:10 | elem |
 | ssa.rb:45:3:45:12 | phi | ssa.rb:45:3:45:3 | x | ssa.rb:44:1:47:3 | <uninitialized> |
 | ssa.rb:45:3:45:12 | phi | ssa.rb:45:3:45:3 | x | ssa.rb:45:3:45:7 | ... = ... |
 | ssa.rb:50:3:50:6 | phi | ssa.rb:49:14:49:14 | y | ssa.rb:49:1:51:3 | <uninitialized> |