diff --git a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll index 24a506f21ce..89e4ac38675 100644 --- a/java/ql/lib/semmle/code/java/ControlFlowGraph.qll +++ b/java/ql/lib/semmle/code/java/ControlFlowGraph.qll @@ -489,14 +489,14 @@ private module ControlFlowGraphImpl { private Stmt getSwitchStatement(SwitchBlock switch, int i) { result.isNthChildOf(switch, i) } /** - * Holds if `last` is the last node in a pattern case `pc`'s succeeding bind-and-test operation, + * Holds if `last` is the last node in any of pattern case `pc`'s succeeding bind-and-test operations, * immediately before either falling through to execute successor statements or execute a rule body * if present. `completion` is the completion kind of the last operation. */ private predicate lastPatternCaseMatchingOp( PatternCase pc, ControlFlowNode last, Completion completion ) { - last(pc.getPattern(), last, completion) and + last(pc.getAPattern(), last, completion) and completion = NormalCompletion() and not exists(pc.getGuard()) or @@ -776,6 +776,19 @@ private module ControlFlowGraphImpl { last(try.getFinally(), last, NormalCompletion()) } + private predicate isNextNormalSwitchStmt(SwitchBlock switch, Stmt pred, Stmt succ) { + exists(int i, Stmt immediateSucc | + getSwitchStatement(switch, i) = pred and + getSwitchStatement(switch, i + 1) = immediateSucc and + ( + succ = immediateSucc and + not immediateSucc instanceof PatternCase + or + isNextNormalSwitchStmt(switch, immediateSucc, succ) + ) + ) + } + /** * Bind `last` to a cfg node nested inside `n` (or, indeed, `n` itself) such * that `last` may be the last node during an execution of `n` and finish @@ -927,9 +940,15 @@ private module ControlFlowGraphImpl { completion != anonymousBreakCompletion() and not completion instanceof NormalOrBooleanCompletion or - // if the last case completes normally, then so does the switch - last(switch.getStmt(strictcount(switch.getAStmt()) - 1), last, NormalCompletion()) and - completion = NormalCompletion() + // if a statement without a non-pattern-case successor completes normally (or for a pattern case + // the guard succeeds) then the switch completes normally. + exists(Stmt lastNormalStmt, Completion stmtCompletion | + lastNormalStmt = getSwitchStatement(switch, _) and + not isNextNormalSwitchStmt(switch, lastNormalStmt, _) and + last(lastNormalStmt, last, stmtCompletion) and + (stmtCompletion = NormalCompletion() or stmtCompletion = BooleanCompletion(true, _)) and + completion = NormalCompletion() + ) or // if no default case exists, then normal completion of the expression may terminate the switch // Note this can't happen if there are pattern cases or a null literal, as @@ -973,9 +992,9 @@ private module ControlFlowGraphImpl { ) or // A pattern case statement can complete: - // * On failure of its type test (boolean false) + // * On failure of its final type test (boolean false) // * On failure of its guard test if any (boolean false) - // * On completion of its variable declarations, if it is not a rule and has no guard (normal completion) + // * On completion of one of its pattern variable declarations, if it is not a rule and has no guard (normal completion) // * On success of its guard test, if it is not a rule (boolean true) // (the latter two cases are accounted for by lastPatternCaseMatchingOp) exists(PatternCase pc | n = pc | @@ -1315,9 +1334,13 @@ private module ControlFlowGraphImpl { // Note this includes non-rule case statements and the successful pattern match successor // of a non-rule pattern case statement. Rule case statements do not complete normally // (they always break or yield). - exists(int i | - last(getSwitchStatement(switch, i), n, completion) and - result = first(getSwitchStatement(switch, i + 1)) and + // Exception: falling through into a pattern case statement (which necessarily does not + // declare any named variables) must skip one or more such statements, otherwise we would + // incorrectly apply their type test and/or guard. + exists(Stmt pred, Stmt succ | + isNextNormalSwitchStmt(switch, pred, succ) and + last(pred, n, completion) and + result = first(succ) and (completion = NormalCompletion() or completion = BooleanCompletion(true, _)) ) or @@ -1328,16 +1351,19 @@ private module ControlFlowGraphImpl { ) or // Pattern cases have internal edges: - // * Type test success -true-> variable declarations + // * Type test success -true-> one of the possible sets of variable declarations + // n.b. for unnamed patterns (e.g. case A _, B _) this means that *one* of the + // type tests has succeeded. There aren't enough nodes in the AST to describe + // a sequential test in detail, so CFG consumers have to watch out for this case. // * Variable declarations -normal-> guard evaluation // * Variable declarations -normal-> rule execution (when there is no guard) // * Guard success -true-> rule execution exists(PatternCase pc | n = pc and completion = basicBooleanCompletion(true) and - result = first(pc.getPattern()) + result = first(pc.getAPattern()) or - last(pc.getPattern(), n, completion) and + last(pc.getAPattern(), n, completion) and completion = NormalCompletion() and result = first(pc.getGuard()) or diff --git a/java/ql/lib/semmle/code/java/Dependency.qll b/java/ql/lib/semmle/code/java/Dependency.qll index 1bdf0140079..29dc81a1960 100644 --- a/java/ql/lib/semmle/code/java/Dependency.qll +++ b/java/ql/lib/semmle/code/java/Dependency.qll @@ -84,7 +84,7 @@ predicate depends(RefType t, RefType dep) { or // A type accessed in a pattern-switch case statement in `t`. exists(PatternCase pc | t = pc.getEnclosingCallable().getDeclaringType() | - usesType(pc.getPattern().getAChildExpr*().getType(), dep) + usesType(pc.getAPattern().getAChildExpr*().getType(), dep) ) ) } diff --git a/java/ql/lib/semmle/code/java/DependencyCounts.qll b/java/ql/lib/semmle/code/java/DependencyCounts.qll index ca0acdedcde..464f8847188 100644 --- a/java/ql/lib/semmle/code/java/DependencyCounts.qll +++ b/java/ql/lib/semmle/code/java/DependencyCounts.qll @@ -107,7 +107,7 @@ predicate numDepends(RefType t, RefType dep, int value) { or // the type accessed in a pattern-switch case statement in `t`. exists(PatternCase pc | elem = pc and t = pc.getEnclosingCallable().getDeclaringType() | - usesType(pc.getPattern().getAChildExpr*().getType(), dep) + usesType(pc.getAPattern().getAChildExpr*().getType(), dep) ) ) } diff --git a/java/ql/lib/semmle/code/java/Expr.qll b/java/ql/lib/semmle/code/java/Expr.qll index 315b4d19b9b..ce2612c9a80 100644 --- a/java/ql/lib/semmle/code/java/Expr.qll +++ b/java/ql/lib/semmle/code/java/Expr.qll @@ -1590,7 +1590,9 @@ class InstanceOfExpr extends Expr, @instanceofexpr { * Note that this won't get anything when record pattern matching is used-- for more general patterns, * use `getPattern`. */ - LocalVariableDeclExpr getLocalVariableDeclExpr() { result = this.getPattern().asBindingPattern() } + LocalVariableDeclExpr getLocalVariableDeclExpr() { + result = this.getPattern().asBindingOrUnnamedPattern() + } /** * Gets the access to the type on the right-hand side of the `instanceof` operator. @@ -1681,7 +1683,10 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr { or exists(InstanceOfExpr ioe | this.getParent() = ioe | result.isNthChildOf(ioe, 1)) or - exists(PatternCase pc | this.getParent() = pc | result.isNthChildOf(pc, -2)) + exists(PatternCase pc, int index, int typeAccessIdx | this.isNthChildOf(pc, index) | + (if index = 0 then typeAccessIdx = -4 else typeAccessIdx = (-3 - index)) and + result.isNthChildOf(pc, typeAccessIdx) + ) or exists(RecordPatternExpr rpe, int index | this.isNthChildOf(rpe, index) and result.isNthChildOf(rpe, -(index + 1)) @@ -1742,17 +1747,17 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr { or exists(SwitchStmt switch | result = switch.getExpr() and - this = switch.getAPatternCase().getPattern().asBindingPattern() + this = switch.getAPatternCase().getAPattern().asBindingOrUnnamedPattern() ) or exists(SwitchExpr switch | result = switch.getExpr() and - this = switch.getAPatternCase().getPattern().asBindingPattern() + this = switch.getAPatternCase().getAPattern().asBindingOrUnnamedPattern() ) or exists(InstanceOfExpr ioe | result = ioe.getExpr() and - this = ioe.getPattern().asBindingPattern() + this = ioe.getPattern().asBindingOrUnnamedPattern() ) } @@ -2676,9 +2681,9 @@ class NotNullExpr extends UnaryExpr, @notnullexpr { } /** - * A binding or record pattern. + * A binding, unnamed or record pattern. * - * Note binding patterns are represented as `LocalVariableDeclExpr`s. + * Note binding and unnamed patterns are represented as `LocalVariableDeclExpr`s. */ class PatternExpr extends Expr { PatternExpr() { @@ -2691,9 +2696,14 @@ class PatternExpr extends Expr { } /** - * Gets this pattern cast to a binding pattern. + * Gets this pattern cast to a binding or unnamed pattern. */ - LocalVariableDeclExpr asBindingPattern() { result = this } + LocalVariableDeclExpr asBindingOrUnnamedPattern() { result = this } + + /** + * DEPRECATED: alias for `asBindingOrUnnamedPattern`. + */ + deprecated LocalVariableDeclExpr asBindingPattern() { result = this.asBindingOrUnnamedPattern() } /** * Gets this pattern cast to a record pattern. diff --git a/java/ql/lib/semmle/code/java/PrettyPrintAst.qll b/java/ql/lib/semmle/code/java/PrettyPrintAst.qll index 6a5e5aa698b..2b0cd1a7899 100644 --- a/java/ql/lib/semmle/code/java/PrettyPrintAst.qll +++ b/java/ql/lib/semmle/code/java/PrettyPrintAst.qll @@ -386,7 +386,7 @@ private class PpInstanceOfExpr extends PpAst, InstanceOfExpr { i = 3 and result = " " and this.getPattern() instanceof LocalVariableDeclExpr or i = 4 and - result = this.getPattern().asBindingPattern().getName() + result = this.getPattern().asBindingOrUnnamedPattern().getName() } override PpAst getChild(int i) { @@ -782,28 +782,53 @@ private class PpSwitchCase extends PpAst, SwitchCase { } private class PpPatternCase extends PpAst, PatternCase { + private TypeAccess getPatternTypeAccess(int n) { + result = this.getPatternAtIndex(n).asBindingOrUnnamedPattern().getTypeAccess() + } + + private predicate isAnonymousPattern(int n) { + this.getPatternAtIndex(n).asBindingOrUnnamedPattern().isAnonymous() + } + override string getPart(int i) { - i = 0 and result = "case " + exists(int n, int base | exists(this.getPatternAtIndex(n)) and base = n * 4 | + i = base and + (if n = 0 then result = "case " else result = ", ") + or + i = base + 2 and + this.getPatternAtIndex(n) instanceof LocalVariableDeclExpr and + not this.isAnonymousPattern(n) and + result = " " + or + i = base + 3 and + ( + if this.isAnonymousPattern(n) + then result = "_" + else result = this.getPatternAtIndex(n).asBindingOrUnnamedPattern().getName() + ) + ) or - i = 2 and this.getPattern() instanceof LocalVariableDeclExpr and result = " " - or - i = 3 and result = this.getPattern().asBindingPattern().getName() - or - i = 4 and result = ":" and not this.isRule() - or - i = 4 and result = " -> " and this.isRule() - or - i = 6 and result = ";" and exists(this.getRuleExpression()) + exists(int base | base = (max(int n | exists(this.getPatternAtIndex(n))) + 1) * 4 | + i = base and result = ":" and not this.isRule() + or + i = base and result = " -> " and this.isRule() + or + i = base + 2 and result = ";" and exists(this.getRuleExpression()) + ) } override PpAst getChild(int i) { - i = 1 and result = this.getPattern().asBindingPattern().getTypeAccess() + exists(int n, int base | exists(this.getPatternAtIndex(n)) and base = n * 4 | + i = 1 and result = this.getPatternAtIndex(n).asBindingOrUnnamedPattern().getTypeAccess() + or + i = 1 and result = this.getPatternAtIndex(n).asRecordPattern() + ) or - i = 1 and result = this.getPattern().asRecordPattern() - or - i = 5 and result = this.getRuleExpression() - or - i = 5 and result = this.getRuleStatement() + exists(int base | base = (max(int n | exists(this.getPatternAtIndex(n))) + 1) * 4 | + i = base + 1 and result = this.getRuleExpression() + or + i = base + 1 and result = this.getRuleStatement() + ) } } diff --git a/java/ql/lib/semmle/code/java/PrintAst.qll b/java/ql/lib/semmle/code/java/PrintAst.qll index bc2b81a1a33..9444a4a66bf 100644 --- a/java/ql/lib/semmle/code/java/PrintAst.qll +++ b/java/ql/lib/semmle/code/java/PrintAst.qll @@ -117,7 +117,11 @@ private newtype TPrintAstNode = TElementNode(Element el) { shouldPrint(el, _) } or TForInitNode(ForStmt fs) { shouldPrint(fs, _) and exists(fs.getAnInit()) } or TLocalVarDeclNode(LocalVariableDeclExpr lvde) { - shouldPrint(lvde, _) and lvde.getParent() instanceof SingleLocalVarDeclParent + shouldPrint(lvde, _) and + ( + lvde.getParent() instanceof SingleLocalVarDeclParent or + lvde.getParent() instanceof PatternCase + ) } or TAnnotationsNode(Annotatable ann) { shouldPrint(ann, _) and @@ -415,6 +419,23 @@ final class ForStmtNode extends ExprStmtNode { } } +/** + * A node representing a `PatternCase`. + */ +final class PatternCaseNode extends ExprStmtNode { + PatternCase pc; + + PatternCaseNode() { pc = element } + + override PrintAstNode getChild(int childIndex) { + result = super.getChild(childIndex) and + not result.(ElementNode).getElement() instanceof LocalVariableDeclExpr and + not result.(ElementNode).getElement() instanceof TypeAccess + or + result = TLocalVarDeclNode(pc.getPatternAtIndex(childIndex)) + } +} + /** * An element that can be the parent of up to one `LocalVariableDeclExpr` for which we want * to use a synthetic node to hold the variable declaration and its `TypeAccess`. @@ -423,8 +444,7 @@ private class SingleLocalVarDeclParent extends ExprOrStmt { SingleLocalVarDeclParent() { this instanceof EnhancedForStmt or this instanceof CatchClause or - this.(InstanceOfExpr).isPattern() or - this instanceof PatternCase + this.(InstanceOfExpr).isPattern() } /** Gets the variable declaration that this element contains */ @@ -643,7 +663,11 @@ final class LocalVarDeclSynthNode extends PrintAstNode, TLocalVarDeclNode { LocalVarDeclSynthNode() { this = TLocalVarDeclNode(lvde) } - override string toString() { result = "(Single Local Variable Declaration)" } + override string toString() { + if lvde.getParent() instanceof PatternCase + then result = "(Pattern case declaration)" + else result = "(Single Local Variable Declaration)" + } override ElementNode getChild(int childIndex) { childIndex = 0 and diff --git a/java/ql/lib/semmle/code/java/Statement.qll b/java/ql/lib/semmle/code/java/Statement.qll index 05d105e4de3..c25ccef88ba 100644 --- a/java/ql/lib/semmle/code/java/Statement.qll +++ b/java/ql/lib/semmle/code/java/Statement.qll @@ -539,12 +539,19 @@ class ConstCase extends SwitchCase { /** A pattern case of a `switch` statement */ class PatternCase extends SwitchCase { - PatternExpr pattern; + PatternCase() { exists(PatternExpr pe | pe.isNthChildOf(this, _)) } - PatternCase() { pattern.isNthChildOf(this, 0) } + /** + * DEPRECATED: alias for getPatternAtIndex(0) + */ + deprecated PatternExpr getPattern() { result = this.getPatternAtIndex(0) } - /** Gets this case's pattern. */ - PatternExpr getPattern() { result = pattern } + /** + * Gets this case's `n`th pattern. + */ + PatternExpr getPatternAtIndex(int n) { result.isNthChildOf(this, n) } + + PatternExpr getAPattern() { result = this.getPatternAtIndex(_) } /** Gets the guard applicable to this pattern case, if any. */ Expr getGuard() { result.isNthChildOf(this, -3) } diff --git a/java/ql/lib/semmle/code/java/Variable.qll b/java/ql/lib/semmle/code/java/Variable.qll index f28e378c233..a4cf09df055 100644 --- a/java/ql/lib/semmle/code/java/Variable.qll +++ b/java/ql/lib/semmle/code/java/Variable.qll @@ -58,7 +58,13 @@ class LocalVariableDecl extends @localvar, LocalScopeVariable { /** Gets the callable in which this declaration occurs. */ Callable getEnclosingCallable() { result = this.getCallable() } - override string toString() { result = this.getType().getName() + " " + this.getName() } + override string toString() { + exists(string sourceName | + if this.getName() = "" then sourceName = "_" else sourceName = this.getName() + | + result = this.getType().getName() + " " + sourceName + ) + } /** Gets the initializer expression of this local variable declaration. */ override Expr getInitializer() { result = this.getDeclExpr().getInit() } diff --git a/java/ql/lib/semmle/code/java/controlflow/Guards.qll b/java/ql/lib/semmle/code/java/controlflow/Guards.qll index a97cf1f8f57..94052d289db 100644 --- a/java/ql/lib/semmle/code/java/controlflow/Guards.qll +++ b/java/ql/lib/semmle/code/java/controlflow/Guards.qll @@ -115,8 +115,20 @@ private predicate isNonFallThroughPredecessor(SwitchCase sc, ControlFlowNode pre ( pred.(Expr).getParent*() = sc.getSelectorExpr() or - pred.(Expr).getParent*() = getClosestPrecedingPatternCase(sc).getGuard() + // Ambiguous: in the case of `case String _ when x: case "SomeConstant":`, the guard `x` + // passing edge will fall through into the constant case, and the guard failing edge + // will test if the selector equals `"SomeConstant"` and if so branch to the same + // case statement. Therefore don't label this a non-fall-through predecessor. + exists(PatternCase previousPatternCase | + previousPatternCase = getClosestPrecedingPatternCase(sc) + | + pred.(Expr).getParent*() = previousPatternCase.getGuard() and + // Check there is any statement in between the previous pattern case and this one. + not previousPatternCase.getIndex() = sc.getIndex() - 1 + ) or + // Unambigious: on the test-passing edge there must be at least one intervening + // declaration node, including anonymous `_` declarations. pred = getClosestPrecedingPatternCase(sc) ) }