Ruby: Code review suggestions

Author: hvitved

ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll

@@ -28,6 +28,10 @@ private newtype TCompletion =
     outer instanceof NonNestedNormalCompletion and
     nestLevel = 0
     or
+    inner instanceof TBooleanCompletion and
+    outer instanceof TMatchingCompletion and
+    nestLevel = 0
+    or
     inner instanceof NormalCompletion and
     nestedEnsureCompletion(outer, nestLevel)
   }
@@ -82,9 +86,8 @@ private predicate mayRaise(Call c) {
 
 /** A completion of a statement or an expression. */
 abstract class Completion extends TCompletion {
-  /** Holds if this completion is valid for node `n`. */
-  predicate isValidFor(AstNode n) {
-    exists(AstNode other | n = other.getDesugared() and this.isValidFor(other))
+  private predicate isValidForSpecific(AstNode n) {
+    exists(AstNode other | n = other.getDesugared() and this.isValidForSpecific(other))
     or
     this = n.(NonReturningCall).getACompletion()
     or
@@ -101,19 +104,26 @@ abstract class Completion extends TCompletion {
     mustHaveMatchingCompletion(n) and
     this = TMatchingCompletion(_)
     or
-    n = any(RescueModifierExpr parent).getBody() and this = TRaiseCompletion()
+    n = any(RescueModifierExpr parent).getBody() and
+    this = [TSimpleCompletion().(TCompletion), TRaiseCompletion()]
     or
     (
       mayRaise(n)
       or
       n instanceof CaseMatch and not exists(n.(CaseExpr).getElseBranch())
     ) and
-    this = TRaiseCompletion()
+    (
+      this = TRaiseCompletion()
+      or
+      this = TSimpleCompletion() and not n instanceof NonReturningCall
+    )
+  }
+
+  /** Holds if this completion is valid for node `n`. */
+  predicate isValidFor(AstNode n) {
+    this.isValidForSpecific(n)
     or
-    not n instanceof NonReturningCall and
-    not completionIsValidForStmt(n, _) and
-    not mustHaveBooleanCompletion(n) and
-    not mustHaveMatchingCompletion(n) and
+    not any(Completion c).isValidForSpecific(n) and
     this = TSimpleCompletion()
   }
 
@@ -254,7 +264,7 @@ class SimpleCompletion extends NonNestedNormalCompletion, TSimpleCompletion {
  * the successor. Either a Boolean completion (`BooleanCompletion`), or a matching
  * completion (`MatchingCompletion`).
  */
-abstract class ConditionalCompletion extends NonNestedNormalCompletion {
+abstract class ConditionalCompletion extends NormalCompletion {
   boolean value;
 
   bindingset[value]
@@ -268,7 +278,7 @@ abstract class ConditionalCompletion extends NonNestedNormalCompletion {
  * A completion that represents evaluation of an expression
  * with a Boolean value.
  */
-class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion {
+class BooleanCompletion extends ConditionalCompletion, NonNestedNormalCompletion, TBooleanCompletion {
   BooleanCompletion() { this = TBooleanCompletion(value) }
 
   /** Gets the dual Boolean completion. */
@@ -293,10 +303,16 @@ class FalseCompletion extends BooleanCompletion {
  * A completion that represents evaluation of a matching test, for example
  * a test in a `rescue` statement.
  */
-class MatchingCompletion extends ConditionalCompletion, TMatchingCompletion {
-  MatchingCompletion() { this = TMatchingCompletion(value) }
+class MatchingCompletion extends ConditionalCompletion {
+  MatchingCompletion() {
+    this = TMatchingCompletion(value)
+    or
+    this = TNestedCompletion(_, TMatchingCompletion(value), _)
+  }
 
-  override MatchingSuccessor getAMatchingSuccessorType() { result.getValue() = value }
+  override SuccessorType getAMatchingSuccessorType() {
+    this = TMatchingCompletion(result.(MatchingSuccessor).getValue())
+  }
 
   override string toString() { if value = true then result = "match" else result = "no-match" }
 }
@@ -502,3 +518,41 @@ class NestedEnsureCompletion extends NestedCompletion {
 
   override SuccessorType getAMatchingSuccessorType() { none() }
 }
+
+/**
+ * A completion used for conditions in pattern matching:
+ *
+ * ```rb
+ * in x if x == 5 then puts "five"
+ * in x unless x == 4 then puts "not four"
+ * ```
+ *
+ * The outer (Matching) completion indicates whether there is a match, and
+ * the inner (Boolean) completion indicates what the condition evaluated
+ * to.
+ *
+ * For the condition `x == 5` above, `TNestedCompletion(true, true, 0)` and
+ * `TNestedCompletion(false, false, 0)` are both valid completions, while
+ * `TNestedCompletion(true, false, 0)` and `TNestedCompletion(false, true, 0)`
+ * are valid completions for `x == 4`.
+ */
+class NestedMatchingCompletion extends NestedCompletion, MatchingCompletion {
+  NestedMatchingCompletion() {
+    inner instanceof TBooleanCompletion and
+    outer instanceof TMatchingCompletion
+  }
+
+  override BooleanCompletion getInnerCompletion() { result = inner }
+
+  override MatchingCompletion getOuterCompletion() { result = outer }
+
+  override Completion getAnInnerCompatibleCompletion() {
+    result.getOuterCompletion() = this.getInnerCompletion()
+  }
+
+  override BooleanSuccessor getAMatchingSuccessorType() {
+    result.getValue() = this.getInnerCompletion().getValue()
+  }
+
+  override string toString() { result = NestedCompletion.super.toString() }
+}

ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -429,12 +429,12 @@ module Trees {
       not c.(MatchingCompletion).getValue() = false
       or
       not exists(this.getElseBranch()) and
-      exists(Completion lc, Expr lastBranch |
+      exists(MatchingCompletion lc, Expr lastBranch |
         lastBranch = max(int i | | this.getBranch(i) order by i) and
-        lc.(MatchingCompletion).getValue() = false and
+        lc.getValue() = false and
         last(lastBranch, last, lc) and
-        c.getInnerCompletion() = lc and
-        c instanceof RaiseCompletion
+        c instanceof RaiseCompletion and
+        not c instanceof NestedCompletion
       )
     }
 
@@ -480,7 +480,8 @@ module Trees {
     final override predicate last(AstNode last, Completion c) {
       c.(MatchingCompletion).getValue() = false and
       (
-        last = this
+        last = this and
+        c.isValidFor(this)
         or
         exists(AstNode node |
           node = this.getClass() or
@@ -493,6 +494,7 @@ module Trees {
       or
       c.(MatchingCompletion).getValue() = true and
       last = this and
+      c.isValidFor(this) and
       not exists(this.getPrefixElement(_)) and
       not exists(this.getRestVariableAccess())
       or
@@ -512,14 +514,16 @@ module Trees {
       succ = this and
       c.(MatchingCompletion).getValue() = true
       or
-      pred = this and
-      first(this.getPrefixElement(0), succ) and
-      c.(MatchingCompletion).getValue() = true
-      or
-      not exists(this.getPrefixElement(_)) and
-      pred = this and
-      first(this.getRestVariableAccess(), succ) and
-      c.(MatchingCompletion).getValue() = true
+      exists(AstNode next |
+        pred = this and
+        c.(MatchingCompletion).getValue() = true and
+        first(next, succ)
+      |
+        next = this.getPrefixElement(0)
+        or
+        not exists(this.getPrefixElement(_)) and
+        next = this.getRestVariableAccess()
+      )
       or
       last(max(int i | | this.getPrefixElement(i) order by i), pred, c) and
       first(this.getRestVariableAccess(), succ) and
@@ -565,7 +569,8 @@ module Trees {
       or
       c.(MatchingCompletion).getValue() = false and
       (
-        last = this
+        last = this and
+        c.isValidFor(this)
         or
         exists(AstNode node | node = this.getClass() or node = this.getElement(_) |
           last(node, last, c)
@@ -578,14 +583,16 @@ module Trees {
       succ = this and
       c.(MatchingCompletion).getValue() = true
       or
-      pred = this and
-      first(this.getPrefixVariableAccess(), succ) and
-      c.(MatchingCompletion).getValue() = true
-      or
-      pred = this and
-      first(this.getElement(0), succ) and
-      not exists(this.getPrefixVariableAccess()) and
-      c.(MatchingCompletion).getValue() = true
+      exists(AstNode next |
+        pred = this and
+        c.(MatchingCompletion).getValue() = true and
+        first(next, succ)
+      |
+        next = this.getPrefixVariableAccess()
+        or
+        not exists(this.getPrefixVariableAccess()) and
+        next = this.getElement(0)
+      )
       or
       last(this.getPrefixVariableAccess(), pred, c) and
       first(this.getElement(0), succ) and
@@ -619,7 +626,8 @@ module Trees {
     final override predicate last(AstNode last, Completion c) {
       c.(MatchingCompletion).getValue() = false and
       (
-        last = this
+        last = this and
+        c.isValidFor(this)
         or
         exists(AstNode node |
           node = this.getClass() or
@@ -646,14 +654,16 @@ module Trees {
       succ = this and
       c.(MatchingCompletion).getValue() = true
       or
-      pred = this and
-      first(this.getValue(0), succ) and
-      c.(MatchingCompletion).getValue() = true
-      or
-      not exists(this.getValue(_)) and
-      pred = this and
-      first(this.getRestVariableAccess(), succ) and
-      c.(MatchingCompletion).getValue() = true
+      exists(AstNode next |
+        pred = this and
+        c.(MatchingCompletion).getValue() = true and
+        first(next, succ)
+      |
+        next = this.getValue(0)
+        or
+        not exists(this.getValue(_)) and
+        next = this.getRestVariableAccess()
+      )
       or
       last(max(int i | | this.getValue(i) order by i), pred, c) and
       first(this.getRestVariableAccess(), succ) and
@@ -732,20 +742,36 @@ module Trees {
       child = this.getCondition()
     }
 
+    private predicate lastCondition(AstNode last, BooleanCompletion c, boolean flag) {
+      last(this.getCondition(), last, c) and
+      (
+        flag = true and this.hasIfCondition()
+        or
+        flag = false and this.hasUnlessCondition()
+      )
+    }
+
     final override predicate last(AstNode last, Completion c) {
       last(this.getPattern(), last, c) and
       c.(MatchingCompletion).getValue() = false
       or
-      exists(Completion pc |
-        last(this.getCondition(), last, pc) and
-        pc.(ConditionalCompletion).getValue() = false and
-        c.(MatchingCompletion).getValue() = false
+      exists(BooleanCompletion bc, boolean flag, MatchingCompletion mc |
+        lastCondition(last, bc, flag) and
+        c =
+          any(NestedMatchingCompletion nmc |
+            nmc.getInnerCompletion() = bc and nmc.getOuterCompletion() = mc
+          )
+      |
+        mc.getValue() = false and
+        bc.getValue() = flag.booleanNot()
+        or
+        not exists(this.getBody()) and
+        mc.getValue() = true and
+        bc.getValue() = flag
       )
       or
       last(this.getBody(), last, c)
       or
-      not exists(this.getBody()) and last(this.getCondition(), last, c)
-      or
       not exists(this.getBody()) and
       not exists(this.getCondition()) and
       last(this.getPattern(), last, c)
@@ -766,15 +792,10 @@ module Trees {
         not exists(this.getCondition()) and next = this.getBody()
       )
       or
-      exists(Completion pc, boolean flag |
-        flag = true and this.hasIfCondition()
-        or
-        flag = false and this.hasUnlessCondition()
-      |
-        last(this.getCondition(), pred, pc) and
-        pc.(ConditionalCompletion).getValue() = flag and
-        first(this.getBody(), succ) and
-        c.(MatchingCompletion).getValue() = true
+      exists(boolean flag |
+        lastCondition(pred, c, flag) and
+        c.(BooleanCompletion).getValue() = flag and
+        first(this.getBody(), succ)
       )
     }
   }

ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

@@ -627,8 +627,8 @@ case.rb:
 #-----| match -> x
 
 #   14| ... == ...
-#-----| no-match -> in ... then ...
-#-----| match -> 6
+#-----| false -> in ... then ...
+#-----| true -> 6
 
 #   14| x
 #-----|  -> 5
@@ -649,8 +649,8 @@ case.rb:
 #-----| match -> x
 
 #   15| ... < ...
-#-----| match -> 7
-#-----| no-match -> 8
+#-----| false -> 7
+#-----| true -> 8
 
 #   15| x
 #-----|  -> 0

ruby/ql/test/library-tests/controlflow/graph/Nodes.expected

@@ -83,7 +83,7 @@ positionalArguments
 | case.rb:3:29:3:37 | call to puts | case.rb:3:34:3:37 | "x2" |
 | case.rb:4:17:4:24 | call to puts | case.rb:4:22:4:24 | "2" |
 | case.rb:14:13:14:18 | ... == ... | case.rb:14:18:14:18 | 5 |
-| case.rb:15:13:15:17 | ... < ... | case.rb:15:17:15:17 | 0 |
+| case.rb:15:17:15:21 | ... < ... | case.rb:15:21:15:21 | 0 |
 | case.rb:28:14:28:25 | call to raise | case.rb:28:20:28:25 | "oops" |
 | case.rb:76:8:76:18 | call to [] | case.rb:76:11:76:13 | "foo" |
 | case.rb:76:8:76:18 | call to [] | case.rb:76:15:76:17 | "bar" |

ruby/ql/test/library-tests/controlflow/graph/case.rb

@@ -12,7 +12,7 @@ def case_match value
     in 2
       4
     in x if x == 5 then 6
-    in x if x < 0 then 7
+    in x unless x < 0 then 7
     else 8
   end
 end