From 48785a0a767f69265b82b350c0cbfde51c1ae260 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Thu, 7 May 2026 13:07:07 +0200 Subject: [PATCH] Cfg: Rework CFG for switch case patterns. --- .../controlflow/internal/ControlFlowGraph.qll | 2 +- .../lib/semmle/code/java/ControlFlowGraph.qll | 8 +- .../codeql/controlflow/ControlFlowGraph.qll | 89 ++++++++++--------- 3 files changed, 50 insertions(+), 49 deletions(-) diff --git a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll index c44a3ef4a55..ca71e213e32 100644 --- a/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll +++ b/csharp/ql/lib/semmle/code/csharp/controlflow/internal/ControlFlowGraph.qll @@ -229,7 +229,7 @@ module Ast implements AstSig { final private class FinalCase = CS::Case; class Case extends FinalCase { - AstNode getAPattern() { result = this.getPattern() } + AstNode getPattern(int index) { result = this.getPattern() and index = 0 } Expr getGuard() { result = this.getCondition() } diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index a1e071df10c..27f0102b3cf 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -153,10 +153,10 @@ private module Ast implements AstSig { } class Case extends AstNode instanceof J::SwitchCase { - /** Gets a pattern being matched by this case. */ - AstNode getAPattern() { - result = this.(PatternCase).getAPattern() or - result = this.(ConstCase).getValue(_) + /** Gets the pattern being matched by this case at the specified (zero-based) `index`. */ + AstNode getPattern(int index) { + result = this.(PatternCase).getPattern(index) or + result = this.(ConstCase).getValue(index) } /** Gets the guard expression of this case, if any. */ diff --git a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll index 514a68cba47..17a01b7e371 100644 --- a/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll +++ b/shared/controlflow/codeql/controlflow/ControlFlowGraph.qll @@ -255,8 +255,8 @@ signature module AstSig { /** A case in a switch. */ class Case extends AstNode { - /** Gets a pattern being matched by this case. */ - AstNode getAPattern(); + /** Gets the pattern being matched by this case at the specified (zero-based) `index`. */ + AstNode getPattern(int index); /** Gets the guard expression of this case, if any. */ Expr getGuard(); @@ -489,6 +489,18 @@ module Make0 Ast> { * need to be consecutive nor start from a specific index. */ default Parameter callableGetParameter(Callable c, CallableContext ctx, int index) { none() } + + /** Holds if this catch clause catches all exceptions. */ + default predicate catchAll(CatchClause catch) { none() } + + /** + * Holds if this case matches all possible values, for example, if it is a + * `default` case or a match-all pattern like `Object o` or if it is the + * final case in a switch that is known to be exhaustive. + * + * A match-all case can still ultimately fail to match if it has a guard. + */ + default predicate matchAll(Case c) { c instanceof DefaultCase } } /** @@ -612,6 +624,8 @@ module Make0 Ast> { or n instanceof Case or + n = any(Case case).getPattern(_) + or exists(n.(Parameter).getDefaultValue()) ) } @@ -772,12 +786,25 @@ module Make0 Ast> { result = DenseRank2::denseRank(c, ctx.asSome(), rnk) } + private predicate constantCondition(AstNode n, ConditionalSuccessor t) { + n.(BooleanLiteral).getValue() = t.(BooleanSuccessor).getValue() + or + exists(Case c, int i | + Input1::matchAll(c) and + c.getPattern(i) = n and + not exists(c.getPattern(i + 1)) and + t.(MatchingSuccessor).getValue() = true + ) + } + cached private newtype TNode = TBeforeNode(AstNode n) { Input1::cfgCachedStageRef() and exists(getEnclosingCallable(n)) } or TAstNode(AstNode n) { postOrInOrder(n) and exists(getEnclosingCallable(n)) } or TAfterValueNode(AstNode n, ConditionalSuccessor t) { - inConditionalContext(n, t.getKind()) and exists(getEnclosingCallable(n)) + inConditionalContext(n, t.getKind()) and + exists(getEnclosingCallable(n)) and + not constantCondition(n, t.getDual()) } or TAfterNode(AstNode n) { exists(getEnclosingCallable(n)) and @@ -1105,18 +1132,6 @@ module Make0 Ast> { } signature module InputSig2 { - /** Holds if this catch clause catches all exceptions. */ - default predicate catchAll(CatchClause catch) { none() } - - /** - * Holds if this case matches all possible values, for example, if it is a - * `default` case or a match-all pattern like `Object o` or if it is the - * final case in a switch that is known to be exhaustive. - * - * A match-all case can still ultimately fail to match if it has a guard. - */ - default predicate matchAll(Case c) { c instanceof DefaultCase } - /** * Holds if `ast` may result in an abrupt completion `c` originating at * `n`. The boolean `always` indicates whether the abrupt completion @@ -1471,12 +1486,6 @@ module Make0 Ast> { n2.isBefore(condexpr.getElse()) ) or - exists(BooleanLiteral boollit | - inConditionalContext(boollit, _) and - n1.isBefore(boollit) and - n2.isAfterValue(boollit, any(BooleanSuccessor t | t.getValue() = boollit.getValue())) - ) - or exists(PatternMatchExpr pme | n1.isBefore(pme) and n2.isBefore(pme.getExpr()) @@ -1662,7 +1671,7 @@ module Make0 Ast> { exists(MatchingSuccessor t | n1.isBefore(catchclause) and n2.isAfterValue(catchclause, t) and - if Input2::catchAll(catchclause) then t.getValue() = true else any() + if Input1::catchAll(catchclause) then t.getValue() = true else any() ) or exists(PreControlFlowNode beforeVar, PreControlFlowNode beforeCond | @@ -1720,21 +1729,22 @@ module Make0 Ast> { ) or exists(Case case | - exists(MatchingSuccessor t | - n1.isBefore(case) and - n2.isAfterValue(case, t) and - if Input2::matchAll(case) then t.getValue() = true else any() + n1.isBefore(case) and + ( + if exists(case.getPattern(_)) + then n2.isBefore(case.getPattern(0)) + else n2.isAfterValue(case, any(MatchingSuccessor t | t.getValue() = true)) ) or - exists( - PreControlFlowNode beforePattern, PreControlFlowNode beforeGuard, - PreControlFlowNode beforeBody - | - ( - beforePattern.isBefore(case.getAPattern()) - or - not exists(case.getAPattern()) and beforePattern = beforeGuard - ) and + exists(int i, MatchingSuccessor ms | n1.isAfterValue(case.getPattern(i), ms) | + ms.getValue() = false and + n2.isBefore(case.getPattern(i + 1)) + or + (ms.getValue() = true or not exists(case.getPattern(i + 1))) and + n2.isAfterValue(case, ms) + ) + or + exists(PreControlFlowNode beforeGuard, PreControlFlowNode beforeBody | ( beforeGuard.isBefore(case.getGuard()) or @@ -1748,9 +1758,6 @@ module Make0 Ast> { ) | n1.isAfterValue(case, any(MatchingSuccessor t | t.getValue() = true)) and - n2 = beforePattern - or - n1.isAfter(case.getAPattern()) and n2 = beforeGuard or n1.isAfterTrue(case.getGuard()) and @@ -2225,12 +2232,6 @@ module Make0 Ast> { t instanceof DirectSuccessor and node.isAdditional(any(ForeachStmt foreach), loopHeaderTag()) ) and - // allow for disjunctive patterns (e.g. `case "foo", "bar":`) - not ( - t instanceof DirectSuccessor and - node.isAfterValue(any(Case c | 2 <= strictcount(c.getAPattern())), - any(MatchingSuccessor m | m.getValue() = true)) - ) and // allow for functions with multiple bodies not exists(Callable c | successor.getAstNode() = getBodyEntry(c, _) and