Skip to content

Commit 54a89d6

Browse files
smowtonsmowton
committed
Handle 'case null, default:'
1 parent 9a450b0 commit 54a89d6

File tree

12 files changed

+131
-51
lines changed

12 files changed

+131
-51
lines changed

‎java/ql/examples/snippets/switchcase.ql

+1-1
Original file line numberDiff line numberDiff line change
@@ -14,5 +14,5 @@ where
1414
switch.getExpr().getType() = enum and
1515
missing.getDeclaringType() = enum and
1616
not switch.getAConstCase().getValue() = missing.getAnAccess() and
17-
not exists(switch.getDefaultCase())
17+
not exists(switch.getDefaultOrNullDefaultCase())
1818
select switch

‎java/ql/lib/config/semmlecode.dbscheme

+4
Original file line numberDiff line numberDiff line change
@@ -992,6 +992,10 @@ providesWith(
992992
string serviceImpl: string ref
993993
);
994994

995+
isNullDefaultCase(
996+
int id: @case ref
997+
);
998+
995999
/*
9961000
* Javadoc
9971001
*/

‎java/ql/lib/semmle/code/java/Expr.qll

+9-13
Original file line numberDiff line numberDiff line change
@@ -1514,7 +1514,8 @@ class SwitchExpr extends Expr, StmtParent, @switchexpr {
15141514
* which may be either a normal `case` or a `default`.
15151515
*/
15161516
SwitchCase getCase(int i) {
1517-
result = rank[i + 1](SwitchCase case, int idx | case.isNthChildOf(this, idx) | case order by idx)
1517+
result =
1518+
rank[i + 1](SwitchCase case, int idx | case.isNthChildOf(this, idx) | case order by idx)
15181519
}
15191520

15201521
/**
@@ -1532,6 +1533,9 @@ class SwitchExpr extends Expr, StmtParent, @switchexpr {
15321533
/** Gets the `default` case of this switch expression, if any. */
15331534
DefaultCase getDefaultCase() { result = this.getACase() }
15341535

1536+
/** Gets the `default` or `case null, default` case of this switch statement, if any. */
1537+
SwitchCase getDefaultOrNullDefaultCase() { result = this.getACase() and result.hasDefaultLabel() }
1538+
15351539
/** Gets the expression of this `switch` expression. */
15361540
Expr getExpr() { result.getParent() = this }
15371541

@@ -1543,9 +1547,7 @@ class SwitchExpr extends Expr, StmtParent, @switchexpr {
15431547
}
15441548

15451549
/** Holds if this switch has a case handling a null literal. */
1546-
predicate hasNullCase() {
1547-
this.getAConstCase().getValue(_) instanceof NullLiteral
1548-
}
1550+
predicate hasNullCase() { this.getAConstCase().getValue(_) instanceof NullLiteral }
15491551

15501552
/** Gets a printable representation of this expression. */
15511553
override string toString() { result = "switch (...)" }
@@ -1638,19 +1640,13 @@ class LocalVariableDeclExpr extends Expr, @localvariabledeclexpr {
16381640
string getName() { result = this.getVariable().getName() }
16391641

16401642
/** Gets the switch statement or expression whose pattern declares this identifier, if any. */
1641-
StmtParent getAssociatedSwitch() {
1642-
result = this.getParent().(PatternCase).getParent()
1643-
}
1643+
StmtParent getAssociatedSwitch() { result = this.getParent().(PatternCase).getParent() }
16441644

16451645
/** Holds if this is a declaration stemming from a pattern switch case. */
1646-
predicate hasAssociatedSwitch() {
1647-
exists(this.getAssociatedSwitch())
1648-
}
1646+
predicate hasAssociatedSwitch() { exists(this.getAssociatedSwitch()) }
16491647

16501648
/** Gets the initializer expression of this local variable declaration expression, if any. */
1651-
Expr getInit() {
1652-
result.isNthChildOf(this, 0)
1653-
}
1649+
Expr getInit() { result.isNthChildOf(this, 0) }
16541650

16551651
/** Holds if this variable declaration implicitly initializes the variable. */
16561652
predicate hasImplicitInit() {

‎java/ql/lib/semmle/code/java/PrettyPrintAst.qll

+9-2
Original file line numberDiff line numberDiff line change
@@ -746,7 +746,9 @@ private class PpSwitchCase extends PpAst, SwitchCase {
746746
override string getPart(int i) {
747747
i = 0 and result = "default" and this instanceof DefaultCase
748748
or
749-
i = 0 and result = "case " and this instanceof ConstCase
749+
i = 0 and result = "case " and not this instanceof DefaultCase
750+
or
751+
i = this.lastConstCaseValueIndex() and result = "default" and this instanceof NullDefaultCase
750752
or
751753
exists(int j | i = 2 * j and j != 0 and result = ", " and exists(this.(ConstCase).getValue(j)))
752754
or
@@ -757,8 +759,13 @@ private class PpSwitchCase extends PpAst, SwitchCase {
757759
i = 3 + this.lastConstCaseValueIndex() and result = ";" and exists(this.getRuleExpression())
758760
}
759761

762+
private int getCaseDefaultOffset() {
763+
if this instanceof NullDefaultCase then result = 1 else result = 0
764+
}
765+
760766
private int lastConstCaseValueIndex() {
761-
result = 1 + 2 * max(int j | j = 0 or exists(this.(ConstCase).getValue(j)))
767+
result =
768+
1 + 2 * (max(int j | j = 0 or exists(this.(ConstCase).getValue(j))) + this.getCaseDefaultOffset())
762769
}
763770

764771
override PpAst getChild(int i) {

‎java/ql/lib/semmle/code/java/Statement.qll

+36-2
Original file line numberDiff line numberDiff line change
@@ -406,6 +406,9 @@ class SwitchStmt extends Stmt, @switchstmt {
406406
/** Gets the `default` case of this switch statement, if any. */
407407
DefaultCase getDefaultCase() { result = this.getACase() }
408408

409+
/** Gets the `default` or `case null, default` case of this switch statement, if any. */
410+
SwitchCase getDefaultOrNullDefaultCase() { result = this.getACase() and result.hasDefaultLabel() }
411+
409412
/** Gets the expression of this `switch` statement. */
410413
Expr getExpr() { result.getParent() = this }
411414

@@ -487,14 +490,28 @@ class SwitchCase extends Stmt, @case {
487490
Stmt getRuleStatementOrExpressionStatement() {
488491
result.getParent() = this and result.getIndex() = -1
489492
}
493+
494+
/**
495+
* Holds if this case statement includes the default label, i.e. it is either `default`
496+
* or `case null, default`.
497+
*/
498+
predicate hasDefaultLabel() { this instanceof DefaultCase or this instanceof NullDefaultCase }
490499
}
491500

492-
/** A constant `case` of a switch statement. */
501+
/**
502+
* A constant `case` of a switch statement.
503+
*
504+
* Note this excludes `case null, default` even though that includes a null constant. It
505+
* does however include plain `case null`.
506+
*/
493507
class ConstCase extends SwitchCase {
494508
ConstCase() {
495509
exists(Expr e |
496510
e.getParent() = this and e.getIndex() >= 0 and not e instanceof LocalVariableDeclExpr
497511
)
512+
// For backward compatibility, we don't include `case null, default:` here, on the assumption
513+
// this will come as a surprise to CodeQL that predates that statement's validity.
514+
and not isNullDefaultCase(this)
498515
}
499516

500517
/** Gets the `case` constant at index 0. */
@@ -535,7 +552,11 @@ class PatternCase extends SwitchCase {
535552
override string getAPrimaryQlClass() { result = "PatternCase" }
536553
}
537554

538-
/** A `default` case of a `switch` statement */
555+
/**
556+
* A `default` case of a `switch` statement.
557+
*
558+
* Note this does not include `case null, default` -- for that, see `NullDefaultCase`.
559+
*/
539560
class DefaultCase extends SwitchCase {
540561
DefaultCase() { not exists(Expr e | e.getParent() = this | e.getIndex() >= 0) }
541562

@@ -548,6 +569,19 @@ class DefaultCase extends SwitchCase {
548569
override string getAPrimaryQlClass() { result = "DefaultCase" }
549570
}
550571

572+
/** A `case null, default` statement of a `switch` statement or expression. */
573+
class NullDefaultCase extends SwitchCase {
574+
NullDefaultCase() { isNullDefaultCase(this) }
575+
576+
override string pp() { result = "case null, default" }
577+
578+
override string toString() { result = "case null, default" }
579+
580+
override string getHalsteadID() { result = "NullDefaultCase" }
581+
582+
override string getAPrimaryQlClass() { result = "NullDefaultCase" }
583+
}
584+
551585
/** A `synchronized` statement. */
552586
class SynchronizedStmt extends Stmt, @synchronizedstmt {
553587
/** Gets the expression on which this `synchronized` statement synchronizes. */

‎java/ql/lib/semmle/code/java/controlflow/UnreachableBlocks.qll

+2-1
Original file line numberDiff line numberDiff line change
@@ -170,10 +170,11 @@ class ConstSwitchStmt extends SwitchStmt {
170170
/** Gets the matching case, if it can be deduced. */
171171
SwitchCase getMatchingCase() {
172172
// Must be a value we can deduce
173+
// TODO: handle other known constants (enum constants, String constants)
173174
exists(this.getExpr().(ConstantExpr).getIntValue()) and
174175
if exists(this.getMatchingConstCase())
175176
then result = this.getMatchingConstCase()
176-
else result = this.getDefaultCase()
177+
else result = this.getDefaultOrNullDefaultCase()
177178
}
178179

179180
/**

‎java/ql/lib/semmle/code/java/controlflow/internal/GuardsLogic.qll

+6-2
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,13 @@ predicate implies_v1(Guard g1, boolean b1, Guard g2, boolean b2) {
5555
)
5656
)
5757
or
58-
g1.(DefaultCase).getSwitch().getAConstCase() = g2 and b1 = true and b2 = false
58+
exists(SwitchCase sc | g1 = sc and sc.hasDefaultLabel() |
59+
sc.getSwitch().getAConstCase() = g2 and b1 = true and b2 = false
60+
)
5961
or
60-
g1.(DefaultCase).getSwitchExpr().getAConstCase() = g2 and b1 = true and b2 = false
62+
exists(SwitchCase sc | g1 = sc and sc.hasDefaultLabel() |
63+
sc.getSwitchExpr().getAConstCase() = g2 and b1 = true and b2 = false
64+
)
6165
or
6266
exists(MethodCall check, int argIndex | check = g1 |
6367
conditionCheckArgument(check, argIndex, _) and

‎java/ql/src/Advisory/Statements/MissingDefaultInSwitch.ql

+1-1
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,5 @@ import java
1515
from SwitchStmt switch
1616
where
1717
not switch.getExpr().getType() instanceof EnumType and
18-
not exists(switch.getDefaultCase())
18+
not exists(switch.getDefaultOrNullDefaultCase())
1919
select switch, "Switch statement does not have a default case."

‎java/ql/src/Likely Bugs/Statements/MissingEnumInSwitch.ql

+1-1
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,6 @@ from SwitchStmt switch, EnumType enum, EnumConstant missing
1717
where
1818
switch.getExpr().getType() = enum and
1919
missing.getDeclaringType() = enum and
20-
not exists(switch.getDefaultCase()) and
20+
not exists(switch.getDefaultOrNullDefaultCase()) and
2121
not switch.getAConstCase().getValue() = missing.getAnAccess()
2222
select switch, "Switch statement does not have a case for $@.", missing, missing.getName()

‎java/ql/test/library-tests/pattern-switch/cfg/Test.java

+18
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,24 @@ public static void test(Object thing) {
5252
default -> { }
5353
}
5454

55+
switch((String)thing) {
56+
case "Const1":
57+
System.out.println("It's Const1!");
58+
case "Const2":
59+
System.out.println("It's Const1 or Const2!");
60+
break;
61+
case String s when s.length() <= 6:
62+
System.out.println("It's <= 6 chars long, and neither Const1 nor Const2");
63+
case "Const3":
64+
System.out.println("It's (<= 6 chars long, and neither Const1 nor Const2), or Const3");
65+
break;
66+
case "Const30":
67+
System.out.println("It's Const30");
68+
break;
69+
case null, default:
70+
System.out.println("It's null, or something else");
71+
}
72+
5573
}
5674

5775
}

‎java/ql/test/library-tests/printAst/A.java

+4
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,10 @@ void varDecls(Object[] things) {
8888
case String s when s.length() == 5 -> "it's 5 letters long";
8989
default -> "It's something else";
9090
};
91+
var nullDefaultTest = switch(thing) {
92+
case String s -> "It's a string";
93+
case null, default -> "It's something else";
94+
};
9195
}
9296
}
9397
catch (RuntimeException rte) {

‎java/ql/test/library-tests/printAst/PrintAst.expected

+40-28
Original file line numberDiff line numberDiff line change
@@ -246,36 +246,48 @@ A.java:
246246
# 88| 1: [LocalVariableDeclExpr] s
247247
# 89| 3: [DefaultCase] default
248248
# 89| -1: [StringLiteral] "It's something else"
249-
# 93| 0: [CatchClause] catch (...)
249+
# 91| 8: [LocalVariableDeclStmt] var ...;
250+
# 91| 1: [LocalVariableDeclExpr] nullDefaultTest
251+
# 91| 0: [SwitchExpr] switch (...)
252+
# 91| -1: [VarAccess] thing
253+
# 92| 0: [PatternCase] case T t ...
254+
# 92| -1: [StringLiteral] "It's a string"
255+
#-----| 0: (Single Local Variable Declaration)
256+
# 92| 0: [TypeAccess] String
257+
# 92| 1: [LocalVariableDeclExpr] s
258+
# 93| 1: [NullDefaultCase] case null, default
259+
# 93| -1: [StringLiteral] "It's something else"
260+
# 93| 0: [NullLiteral] null
261+
# 97| 0: [CatchClause] catch (...)
250262
#-----| 0: (Single Local Variable Declaration)
251-
# 93| 0: [TypeAccess] RuntimeException
252-
# 93| 1: [LocalVariableDeclExpr] rte
253-
# 93| 1: [BlockStmt] { ... }
254-
# 94| 0: [ReturnStmt] return ...
255-
# 98| 10: [Class] E
256-
# 102| 3: [FieldDeclaration] E A;
263+
# 97| 0: [TypeAccess] RuntimeException
264+
# 97| 1: [LocalVariableDeclExpr] rte
265+
# 97| 1: [BlockStmt] { ... }
266+
# 98| 0: [ReturnStmt] return ...
267+
# 102| 10: [Class] E
268+
# 106| 3: [FieldDeclaration] E A;
257269
#-----| -3: (Javadoc)
258-
# 99| 1: [Javadoc] /** Javadoc for enum constant */
259-
# 100| 0: [JavadocText] Javadoc for enum constant
260-
# 102| -1: [TypeAccess] E
261-
# 102| 0: [ClassInstanceExpr] new E(...)
262-
# 102| -3: [TypeAccess] E
263-
# 103| 4: [FieldDeclaration] E B;
270+
# 103| 1: [Javadoc] /** Javadoc for enum constant */
271+
# 104| 0: [JavadocText] Javadoc for enum constant
272+
# 106| -1: [TypeAccess] E
273+
# 106| 0: [ClassInstanceExpr] new E(...)
274+
# 106| -3: [TypeAccess] E
275+
# 107| 4: [FieldDeclaration] E B;
264276
#-----| -3: (Javadoc)
265-
# 99| 1: [Javadoc] /** Javadoc for enum constant */
266-
# 100| 0: [JavadocText] Javadoc for enum constant
267-
# 103| -1: [TypeAccess] E
268-
# 103| 0: [ClassInstanceExpr] new E(...)
269-
# 103| -3: [TypeAccess] E
270-
# 104| 5: [FieldDeclaration] E C;
277+
# 103| 1: [Javadoc] /** Javadoc for enum constant */
278+
# 104| 0: [JavadocText] Javadoc for enum constant
279+
# 107| -1: [TypeAccess] E
280+
# 107| 0: [ClassInstanceExpr] new E(...)
281+
# 107| -3: [TypeAccess] E
282+
# 108| 5: [FieldDeclaration] E C;
271283
#-----| -3: (Javadoc)
272-
# 99| 1: [Javadoc] /** Javadoc for enum constant */
273-
# 100| 0: [JavadocText] Javadoc for enum constant
274-
# 104| -1: [TypeAccess] E
275-
# 104| 0: [ClassInstanceExpr] new E(...)
276-
# 104| -3: [TypeAccess] E
277-
# 110| 11: [FieldDeclaration] int i, ...;
284+
# 103| 1: [Javadoc] /** Javadoc for enum constant */
285+
# 104| 0: [JavadocText] Javadoc for enum constant
286+
# 108| -1: [TypeAccess] E
287+
# 108| 0: [ClassInstanceExpr] new E(...)
288+
# 108| -3: [TypeAccess] E
289+
# 114| 11: [FieldDeclaration] int i, ...;
278290
#-----| -3: (Javadoc)
279-
# 107| 1: [Javadoc] /** Javadoc for fields */
280-
# 108| 0: [JavadocText] Javadoc for fields
281-
# 110| -1: [TypeAccess] int
291+
# 111| 1: [Javadoc] /** Javadoc for fields */
292+
# 112| 0: [JavadocText] Javadoc for fields
293+
# 114| -1: [TypeAccess] int

0 commit comments

Comments
 (0)