CFG: Address more review comments

ql/src/codeql_ruby/controlflow/internal/Completion.qll

@@ -222,8 +222,13 @@ class SimpleCompletion extends NonNestedNormalCompletion, TSimpleCompletion {
  * completion (`EmptinessCompletion`), or a matching completion (`MatchingCompletion`).
  */
 abstract class ConditionalCompletion extends NonNestedNormalCompletion {
+  boolean value;
+
+  bindingset[value]
+  ConditionalCompletion() { any() }
+
   /** Gets the Boolean value of this conditional completion. */
-  abstract boolean getValue();
+  final boolean getValue() { result = value }
 }
 
 /**
@@ -231,12 +236,8 @@ abstract class ConditionalCompletion extends NonNestedNormalCompletion {
  * with a Boolean value.
  */
 class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion {
-  private boolean value;
-
   BooleanCompletion() { this = TBooleanCompletion(value) }
 
-  override boolean getValue() { result = value }
-
   /** Gets the dual Boolean completion. */
   BooleanCompletion getDual() { result = TBooleanCompletion(value.booleanNot()) }
 
@@ -260,12 +261,8 @@ class FalseCompletion extends BooleanCompletion {
  * a test in a `for in` statement.
  */
 class EmptinessCompletion extends ConditionalCompletion, TEmptinessCompletion {
-  private boolean value;
-
   EmptinessCompletion() { this = TEmptinessCompletion(value) }
 
-  override boolean getValue() { result = value }
-
   override EmptinessSuccessor getAMatchingSuccessorType() { result.getValue() = value }
 
   override string toString() { if value = true then result = "empty" else result = "non-empty" }
@@ -276,12 +273,8 @@ class EmptinessCompletion extends ConditionalCompletion, TEmptinessCompletion {
  * a test in a `rescue` statement.
  */
 class MatchingCompletion extends ConditionalCompletion, TMatchingCompletion {
-  private boolean value;
-
   MatchingCompletion() { this = TMatchingCompletion(value) }
 
-  override boolean getValue() { result = value }
-
   override MatchingSuccessor getAMatchingSuccessorType() { result.getValue() = value }
 
   override string toString() { if value = true then result = "match" else result = "no-match" }

ql/src/codeql_ruby/controlflow/internal/ControlFlowGraphImpl.qll

@@ -476,41 +476,13 @@ module Trees {
   private class ExceptionsTree extends PreOrderTree, Exceptions {
     final override predicate propagatesAbnormal(AstNode child) { none() }
 
-    /** Holds if this exception filter belongs to a final `rescue` block. */
-    pragma[noinline]
-    private predicate inLastRescue() {
-      exists(RescueEnsureBlockTree block, Rescue rescue, int i |
-        rescue = block.getRescue(i) and
-        this = rescue.getExceptions() and
-        not exists(block.getRescue(i + 1))
-      )
-    }
-
     final override predicate last(AstNode last, Completion c) {
       last(this.getChild(_), last, c) and
       c.(MatchingCompletion).getValue() = true
       or
-      exists(int lst, Completion c0 |
-        last(this.getChild(lst), last, c0) and
+      exists(int lst |
+        last(this.getChild(lst), last, c) and
         not exists(this.getChild(lst + 1))
-      |
-        // The last exception filter in a last `rescue` block propagates
-        // the exception when it doesn't match
-        this.inLastRescue() and
-        c =
-          any(NestedEnsureCompletion nec |
-            nec.getOuterCompletion() instanceof RaiseCompletion and
-            nec.getInnerCompletion() = c0 and
-            c0.(MatchingCompletion).getValue() = false and
-            nec.getNestLevel() = 0
-          )
-        or
-        c = c0 and
-        (
-          not this.inLastRescue()
-          or
-          not c0.(MatchingCompletion).getValue() = false
-        )
       )
     }
 
@@ -889,23 +861,36 @@ module Trees {
   private class RescueTree extends PreOrderTree, Rescue {
     final override predicate propagatesAbnormal(AstNode child) { child = this.getExceptions() }
 
-    final override predicate last(AstNode last, Completion c) {
-      last(this.getExceptions(), last, c) and
-      c.(MatchingCompletion).getValue() = false
-      or
+    predicate lastMatch(AstNode last, Completion c) {
       last(this.getBody(), last, c)
       or
-      not exists(this.getExceptions()) and
       not exists(this.getBody()) and
       (
         last(this.getVariable(), last, c)
         or
         not exists(this.getVariable()) and
-        last = this and
-        c.isValidFor(this)
+        (
+          last(this.getExceptions(), last, c) and
+          c.(MatchingCompletion).getValue() = true
+          or
+          not exists(this.getExceptions()) and
+          last = this and
+          c.isValidFor(this)
+        )
       )
     }
 
+    predicate lastNoMatch(AstNode last, Completion c) {
+      last(this.getExceptions(), last, c) and
+      c.(MatchingCompletion).getValue() = false
+    }
+
+    final override predicate last(AstNode last, Completion c) {
+      this.lastNoMatch(last, c)
+      or
+      this.lastMatch(last, c)
+    }
+
     final override predicate succ(AstNode pred, AstNode succ, Completion c) {
       exists(AstNode next |
         pred = this and
@@ -983,7 +968,11 @@ module Trees {
 
     final private predicate hasEnsure() { exists(this.getEnsure()) }
 
-    final override predicate propagatesAbnormal(AstNode child) { child = this.getEnsure() }
+    final override predicate propagatesAbnormal(AstNode child) {
+      child = this.getEnsure()
+      or
+      child = this.getBodyChild(_, false)
+    }
 
     /**
      * Gets a descendant that belongs to the `ensure` block of this block, if any.
@@ -1045,37 +1034,39 @@ module Trees {
      */
     pragma[nomagic]
     private AstNode getAnEnsurePredecessor(Completion c, boolean ensurable) {
-      exists(boolean ensurable0 |
-        // Exit completions ignore the `ensure` block (TODO: CHECK THIS)
-        if c instanceof ExitCompletion then ensurable = false else ensurable = ensurable0
-      |
-        this.lastBody(result, c, ensurable0) and
-        (
-          // Any non-throw completion will always continue directly to the `ensure` block,
-          // unless there is an `else` block
-          not c instanceof RaiseCompletion and
-          not exists(this.getElse())
-          or
-          // Any completion will continue to the `ensure` block when there are no `rescue`
-          // blocks
-          not exists(this.getRescue(_))
-        )
+      this.lastBody(result, c, ensurable) and
+      (
+        // Any non-throw completion will always continue directly to the `ensure` block,
+        // unless there is an `else` block
+        not c instanceof RaiseCompletion and
+        not exists(this.getElse())
         or
-        // Last element from any of the `rescue` blocks continues to the `ensure` block
-        last(this.getRescue(_).getBody(), result, c) and
-        ensurable0 = true
-        or
-        // Last element of last `rescue` block continues to the `ensure` block
-        exists(int lst |
-          last(this.getRescue(lst), result, c) and
-          not exists(this.getRescue(lst + 1)) and
-          ensurable0 = true
-        )
-        or
-        // Last element of `else` block continues to the `ensure` block
-        last(this.getElse(), result, c) and
-        ensurable0 = true
+        // Any completion will continue to the `ensure` block when there are no `rescue`
+        // blocks
+        not exists(this.getRescue(_))
       )
+      or
+      // Last element from any matching `rescue` block continues to the `ensure` block
+      this.getRescue(_).(RescueTree).lastMatch(result, c) and
+      ensurable = true
+      or
+      // If the last `rescue` block does not match, continue to the `ensure` block
+      exists(int lst, MatchingCompletion mc |
+        this.getRescue(lst).(RescueTree).lastNoMatch(result, mc) and
+        mc.getValue() = false and
+        not exists(this.getRescue(lst + 1)) and
+        c =
+          any(NestedEnsureCompletion nec |
+            nec.getOuterCompletion() instanceof RaiseCompletion and
+            nec.getInnerCompletion() = mc and
+            nec.getNestLevel() = 0
+          ) and
+        ensurable = true
+      )
+      or
+      // Last element of `else` block continues to the `ensure` block
+      last(this.getElse(), result, c) and
+      ensurable = true
     }
 
     pragma[nomagic]
@@ -1129,19 +1120,10 @@ module Trees {
       first(this.getRescue(0), succ) and
       c instanceof RaiseCompletion
       or
-      exists(Rescue rescue, int i | rescue = this.getRescue(i) |
-        pred = rescue and
-        last(this.getRescue(i), rescue, c) and
-        (
-          // Flow from one `rescue` clause to the next when there is no match
-          first(this.getRescue(i + 1), succ) and
-          c.(MatchingCompletion).getValue() = false
-          or
-          // Flow from last `rescue` clause to `ensure` block
-          not exists(this.getRescue(i + 1)) and
-          first(this.getEnsure(), succ) and
-          c.getInnerCompletion().(MatchingCompletion).getValue() = false
-        )
+      // Flow from one `rescue` clause to the next when there is no match
+      exists(RescueTree rescue, int i | rescue = this.getRescue(i) |
+        rescue.lastNoMatch(pred, c) and
+        first(this.getRescue(i + 1), succ)
       )
       or
       // Flow from body to `else` block when no exception

ql/src/codeql_ruby/controlflow/internal/NonReturning.qll

@@ -18,5 +18,5 @@ private class RaiseCall extends NonReturningCall, MethodCall {
 private class ExitCall extends NonReturningCall, MethodCall {
   ExitCall() { this.getMethod().toString() in ["abort", "exit"] }
 
-  override ExitCompletion getACompletion() { any() }
+  override ExitCompletion getACompletion() { not result instanceof NestedCompletion }
 }

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

@@ -125,6 +125,9 @@ raise.rb:
 #  121| enter m10
 #-----|  -> OptionalParameter
 
+#  128| enter m11
+#-----|  -> b
+
 break_ensure.rb:
 #    1| elements
 #-----|  -> elements
@@ -3059,16 +3062,91 @@ raise.rb:
 #  121| String
 #-----|  -> raise
 
-#  122| Ensure
+#  124| Ensure
 #-----|  -> String
 
-#  123| MethodCall
+#  125| MethodCall
 #-----|  -> exit m10 (normal)
 
-#  123| puts
+#  125| puts
 #-----|  -> MethodCall
 
-#  123| String
+#  125| String
+#-----|  -> puts
+
+#  128| b
+#-----|  -> If
+
+#  130| If
+#-----|  -> b
+
+#  130| b
+#-----| true -> ExceptionA
+#-----| false -> Ensure
+
+#  131| MethodCall
+#-----| raise -> Rescue
+
+#  131| raise
+#-----|  -> MethodCall
+
+#  131| ExceptionA
+#-----|  -> raise
+
+#  133| Rescue
+#-----|  -> ExceptionA
+
+#  133| ExceptionA
+#-----| no-match -> Rescue
+#-----| match -> Ensure
+
+#  134| Rescue
+#-----|  -> ExceptionB
+
+#  134| ExceptionB
+#-----| match -> String
+#-----| raise -> [ensure: raise] Ensure
+
+#  135| MethodCall
+#-----|  -> Ensure
+
+#  135| puts
+#-----|  -> MethodCall
+
+#  135| String
+#-----|  -> puts
+
+#  136| Ensure
+#-----|  -> String
+
+#  136| [ensure: raise] Ensure
+#-----|  -> [ensure: raise] String
+
+#  137| MethodCall
+#-----|  -> String
+
+#  137| [ensure: raise] MethodCall
+#-----| raise -> exit m11 (abnormal)
+
+#  137| puts
+#-----|  -> MethodCall
+
+#  137| [ensure: raise] puts
+#-----|  -> [ensure: raise] MethodCall
+
+#  137| String
+#-----|  -> puts
+
+#  137| [ensure: raise] String
+#-----|  -> [ensure: raise] puts
+
+#  139| MethodCall
+#-----|  -> exit m11 (normal)
+
+#  139| puts
+#-----|  -> MethodCall
+
+#  139| String
 #-----|  -> puts
 
 break_ensure.rb:
@@ -3156,6 +3234,8 @@ raise.rb:
 
 #  121| exit m10
 
+#  128| exit m11
+
 break_ensure.rb:
 #    1| exit m1 (normal)
 #-----|  -> exit m1
@@ -3306,3 +3386,9 @@ raise.rb:
 
 #  121| exit m10 (normal)
 #-----|  -> exit m10
+
+#  128| exit m11 (abnormal)
+#-----|  -> exit m11
+
+#  128| exit m11 (normal)
+#-----|  -> exit m11

ql/test/library-tests/controlflow/graph/raise.rb

@@ -119,6 +119,22 @@ ensure
 end
 
 def m10(p = (raise "Exception"))
+rescue
+  puts "Will not get executed if p is not supplied"
 ensure
   puts "Will not get executed if p is not supplied"
-end
+end
+
+def m11 b
+  begin
+    if b
+      raise ExceptionA
+    end
+  rescue ExceptionA
+  rescue ExceptionB
+    puts "ExceptionB"
+  ensure
+    puts "Ensure"
+  end
+  puts "End m5"
+end