Rust: Remove nonsensical no-match CFG edges

Author: paldepind

rust/ql/.generated.list

@@ -117,51 +117,63 @@ class BooleanCompletion extends ConditionalCompletion, TBooleanCompletion {
   override string toString() { result = "boolean(" + value + ")" }
 }
 
-/** Holds if `pat` is guaranteed to match at the point in the AST where it occurs. */
-pragma[nomagic]
-private predicate isExhaustiveMatch(Pat pat) {
-  (
-    pat instanceof WildcardPat
-    or
-    pat = any(IdentPat ip | not ip.hasPat() and ip = any(Variable v).getPat())
-    or
-    pat instanceof RestPat
-    or
-    // `let` statements without an `else` branch must be exhaustive
-    pat = any(LetStmt let | not let.hasLetElse()).getPat()
-    or
-    // `match` expressions must be exhaustive, so last arm cannot fail
-    pat = any(MatchExpr me).getLastArm().getPat()
-    or
-    // macro invocations are exhaustive if their expansion is
-    pat = any(MacroPat mp | isExhaustiveMatch(mp.getMacroCall().getExpanded()))
-    or
-    // parameter patterns must be exhaustive
-    pat = any(Param p).getPat()
-  ) and
-  not pat = any(ForExpr for).getPat() // workaround until `for` loops are desugared
+/**
+ * Holds if `pat` can not _itself_ be the cause of a pattern match failure. This
+ * does not mean that `pat` is irrefutable, as its children might be the cause
+ * of a failure.
+ */
+private predicate canCauseMatchFailure(Pat pat) {
+  pat instanceof LiteralPat
   or
-  exists(Pat parent | isExhaustiveMatch(parent) |
-    pat = parent.(BoxPat).getPat()
-    or
-    pat = parent.(IdentPat).getPat()
-    or
-    pat = parent.(MacroPat).getMacroCall().getExpanded()
-    or
-    pat = parent.(ParenPat).getPat()
-    or
-    pat = parent.(RecordPat).getRecordPatFieldList().getField(_).getPat()
-    or
-    pat = parent.(RefPat).getPat()
-    or
-    pat = parent.(TuplePat).getAField()
+  // NOTE: a `TupleStructPat` can cause a failure if it resolves to a an enum
+  // variant but not when it resolves to a tuple struct.
+  pat instanceof TupleStructPat
+  or
+  pat instanceof SlicePat
+  or
+  pat instanceof PathPat
+  or
+  pat instanceof OrPat
+  or
+  // Identifier patterns that are in fact path patterns can cause failures. For
+  // instance `None`. Only if a `@ ...` part is present can we be sure that it's
+  // an actual identifier pattern.
+  pat = any(IdentPat p | not p.hasPat())
+}
+
+/**
+ * Holds if `pat` is guaranteed to match at the point in the AST where it occurs
+ * due to Rust's exhaustiveness checks.
+ */
+private predicate guaranteedMatchPosition(Pat pat) {
+  // `let` statements without an `else` branch must match
+  pat = any(LetStmt let | not let.hasLetElse()).getPat()
+  or
+  // `match` expressions must be exhaustive, so last arm must match
+  pat = any(MatchExpr me).getLastArm().getPat()
+  or
+  // parameter patterns must match
+  pat = any(Param p).getPat()
+  or
+  exists(Pat parent | guaranteedMatchPosition(parent) |
+    // propagate to all children except for or patterns
+    parent = pat.getParentPat() and not parent instanceof OrPat
     or
-    pat = parent.(TupleStructPat).getAField()
+    // for or patterns only the last child inherits the property
+    parent.(OrPat).getLastPat() = pat
     or
-    pat = parent.(OrPat).getLastPat()
+    // for macro patterns we propagate to the expanded pattern
+    parent.(MacroPat).getMacroCall().getExpanded() = pat
   )
 }
 
+private predicate guaranteedMatch(Pat pat) {
+  (not canCauseMatchFailure(pat) or guaranteedMatchPosition(pat)) and
+  // In `for` loops we use a no-match edge from the pattern to terminate the
+  // loop, hence we special case and always allow the no-match edge.
+  not pat = any(ForExpr for).getPat()
+}
+
 /**
  * A completion that represents the result of a pattern match.
  */
@@ -170,7 +182,7 @@ class MatchCompletion extends TMatchCompletion, ConditionalCompletion {
 
   override predicate isValidForSpecific(AstNode e) {
     e instanceof Pat and
-    if isExhaustiveMatch(e) then value = true else any()
+    if guaranteedMatch(e) then value = true else any()
     or
     e instanceof TryExpr and value = true
   }

rust/ql/.gitattributes

@@ -641,7 +641,7 @@ module PatternTrees {
       super.last(node, c)
       or
       c.(MatchCompletion).failed() and
-      completionIsValidFor(c, this) and
+      completionIsValidFor(c, node) and
       (node = this or last(this.getPatRanked(_), node, c))
     }
   }

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

@@ -1,19 +1,30 @@
-// generated by codegen, remove this comment if you wish to edit this file
 /**
  * This module provides a hand-modifiable wrapper around the generated class `Pat`.
  *
  * INTERNAL: Do not use.
  */
 
 private import codeql.rust.elements.internal.generated.Pat
+private import codeql.rust.elements.internal.generated.ParentChild
 
 /**
  * INTERNAL: This module contains the customizable definition of `Pat` and should not
  * be referenced directly.
  */
 module Impl {
+  // the following QLdoc is generated: if you need to edit it, do it in the schema file
   /**
    * The base class for patterns.
    */
-  class Pat extends Generated::Pat { }
+  class Pat extends Generated::Pat {
+    /**
+     * If this pattern is immediately nested within another pattern, then get the
+     * parent pattern.
+     */
+    Pat getParentPat() {
+      result = getImmediateParent(this)
+      or
+      result.(RecordPat).getRecordPatFieldList().getAField().getPat() = this
+    }
+  }
 }

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

@@ -36,15 +36,9 @@ module Impl {
     ClosureBodyScope() { this = any(ClosureExpr ce).getBody() }
   }
 
-  private Pat getImmediatePatParent(AstNode n) {
-    result = getImmediateParent(n)
-    or
-    result.(RecordPat).getRecordPatFieldList().getAField().getPat() = n
-  }
-
   private Pat getAPatAncestor(Pat p) {
     (p instanceof IdentPat or p instanceof OrPat) and
-    exists(Pat p0 | result = getImmediatePatParent(p0) |
+    exists(Pat p0 | result = p0.getParentPat() |
       p0 = p
       or
       p0 = getAPatAncestor(p) and
@@ -222,7 +216,7 @@ module Impl {
     or
     exists(Pat mid |
       mid = getAVariablePatAncestor(v) and
-      result = getImmediatePatParent(mid)
+      result = mid.getParentPat()
     )
   }
 

rust/ql/lib/codeql/rust/elements/internal/PatImpl.qll

@@ -19,6 +19,7 @@ edges
 | test.rs:6:17:6:31 | let ... = b | test.rs:6:31:6:31 | b |  |
 | test.rs:6:21:6:27 | Some(...) | test.rs:6:12:6:31 | [boolean(false)] ... && ... | no-match |
 | test.rs:6:21:6:27 | Some(...) | test.rs:6:26:6:26 | d | match |
+| test.rs:6:26:6:26 | d | test.rs:6:12:6:31 | [boolean(false)] ... && ... | no-match |
 | test.rs:6:26:6:26 | d | test.rs:6:12:6:31 | [boolean(true)] ... && ... | match |
 | test.rs:6:26:6:26 | d | test.rs:6:26:6:26 | d |  |
 | test.rs:6:31:6:31 | b | test.rs:6:21:6:27 | Some(...) |  |
@@ -46,6 +47,7 @@ edges
 | test.rs:14:12:15:16 | [boolean(false)] ... && ... | test.rs:19:13:19:17 | false | false |
 | test.rs:14:12:15:16 | [boolean(true)] ... && ... | test.rs:17:13:17:13 | d | true |
 | test.rs:14:17:14:25 | let ... = b | test.rs:14:25:14:25 | b |  |
+| test.rs:14:21:14:21 | d | test.rs:14:12:14:25 | [boolean(false)] ... && ... | no-match |
 | test.rs:14:21:14:21 | d | test.rs:14:12:14:25 | [boolean(true)] ... && ... | match |
 | test.rs:14:21:14:21 | d | test.rs:14:21:14:21 | d |  |
 | test.rs:14:25:14:25 | b | test.rs:14:21:14:21 | d |  |