Skip to content

Commit e25e192

Browse files
hmachmac
committed
Ruby: Change the CFG for while clauses
The `when` node now acts as a join point for patterns in the when clause, with match/no-match completions. This is similar to how `or` expressions work. The result of this is that the `when` clause "controls" the body of the `when`, which allows us to model barrier guards for multi-pattern when clauses. For this code case x when 1, 2 y end The old CFG was x --> when --> 1 --no-match--> 2 ---no-match---> case \ \ ^ \ \ | \ --match----+ | \ | | \ | | ------match---------> y --+ The new CFG is x --> 1 --no-match--> 2 --no-match--> [no-match] when --no-match--> case \ \ ^ \ \ | \ --match--> [match] when --match--> y -----+ \ / \ / -------match----- i.e. all patterns flow to the `when` node, which is split based on whether the pattern matched or not. The body of the when clause then has a single predecessor `[match] when`, which acts as condition block that controls `y`.
1 parent a8b0d29 commit e25e192

File tree

7 files changed

+79
-41
lines changed

7 files changed

+79
-41
lines changed

‎ruby/ql/lib/codeql/ruby/controlflow/CfgNodes.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -446,7 +446,7 @@ module ExprNodes {
446446
final ExprCfgNode getBody() { e.hasCfgChild(e.getBody(), this, result) }
447447

448448
/** Gets the `i`th pattern this `when`-clause. */
449-
final ExprCfgNode getPattern(int i) { e.hasCfgChild(e.getPattern(i), this, result) }
449+
final ExprCfgNode getPattern(int i) { result.getExpr() = e.getPattern(i) }
450450
}
451451

452452
/** A control-flow node that wraps a `CasePattern`. */

‎ruby/ql/lib/codeql/ruby/controlflow/internal/Completion.qll

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,12 @@ private predicate inMatchingContext(AstNode n) {
233233
or
234234
exists(CaseExpr c, WhenClause w |
235235
exists(c.getValue()) and
236-
c.getABranch() = w and
237-
w.getPattern(_) = n
236+
(
237+
c.getABranch() = w and
238+
w.getPattern(_) = n
239+
or
240+
w = n
241+
)
238242
)
239243
or
240244
n instanceof CasePattern

‎ruby/ql/lib/codeql/ruby/controlflow/internal/ControlFlowGraphImpl.qll

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -400,7 +400,7 @@ module Trees {
400400
c instanceof SimpleCompletion
401401
or
402402
exists(int i, WhenTree branch | branch = this.getBranch(i) |
403-
last(branch.getLastPattern(), pred, c) and
403+
pred = branch and
404404
first(this.getBranch(i + 1), succ) and
405405
c.(ConditionalCompletion).getValue() = false
406406
)
@@ -1397,7 +1397,7 @@ module Trees {
13971397
final override ControlFlowTree getChildElement(int i) { result = this.getMethodName(i) }
13981398
}
13991399

1400-
private class WhenTree extends PreOrderTree, WhenClause {
1400+
private class WhenTree extends ControlFlowTree, WhenClause {
14011401
final override predicate propagatesAbnormal(AstNode child) { child = this.getAPattern() }
14021402

14031403
final Expr getLastPattern() {
@@ -1407,28 +1407,35 @@ module Trees {
14071407
)
14081408
}
14091409

1410+
final override predicate first(AstNode first) { first(this.getPattern(0), first) }
1411+
14101412
final override predicate last(AstNode last, Completion c) {
1411-
last(this.getLastPattern(), last, c) and
1413+
last = this and
14121414
c.(ConditionalCompletion).getValue() = false
14131415
or
1414-
last(this.getBody(), last, c)
1416+
last(this.getBody(), last, c) and
1417+
c instanceof NormalCompletion
14151418
}
14161419

14171420
final override predicate succ(AstNode pred, AstNode succ, Completion c) {
14181421
pred = this and
1419-
first(this.getPattern(0), succ) and
1420-
c instanceof SimpleCompletion
1422+
c.isValidFor(this) and
1423+
c.(ConditionalCompletion).getValue() = true and
1424+
first(this.getBody(), succ)
14211425
or
14221426
exists(int i, Expr p, boolean b |
14231427
p = this.getPattern(i) and
14241428
last(p, pred, c) and
14251429
b = c.(ConditionalCompletion).getValue()
14261430
|
14271431
b = true and
1428-
first(this.getBody(), succ)
1432+
succ = this
14291433
or
14301434
b = false and
14311435
first(this.getPattern(i + 1), succ)
1436+
or
1437+
not exists(this.getPattern(i + 1)) and
1438+
succ = this
14321439
)
14331440
}
14341441
}

‎ruby/ql/lib/codeql/ruby/controlflow/internal/Splitting.qll

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,14 @@ private module ConditionalCompletionSplitting {
8585
or
8686
last(succ.(ConditionalExpr).getBranch(_), pred, c) and
8787
completion = c
88+
or
89+
last(succ.(WhenClause).getAPattern(), pred, c) and completion = c
8890
)
91+
or
92+
succ(pred, succ, c) and
93+
succ(succ, _, completion) and
94+
succ instanceof WhenClause and
95+
completion = c
8996
}
9097

9198
override predicate hasEntryScope(CfgScope scope, AstNode succ) { none() }

‎ruby/ql/lib/codeql/ruby/dataflow/BarrierGuards.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -186,8 +186,8 @@ private predicate stringConstCaseCompare(
186186
case.getValue() = testedNode and
187187
(
188188
exists(CfgNodes::ExprNodes::WhenClauseCfgNode branchNode |
189+
guard = branchNode and
189190
branchNode = case.getBranch(_) and
190-
guard = branchNode.getPattern(_) and
191191
// For simplicity, consider patterns that contain only string literals or arrays of string literals
192192
forall(ExprCfgNode pattern | pattern = branchNode.getPattern(_) |
193193
// when "foo"

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

Lines changed: 48 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -526,17 +526,20 @@ case.rb:
526526
#-----| -> exit if_in_case (normal)
527527

528528
# 2| call to x1
529-
#-----| -> when ...
529+
#-----| -> 1
530530

531531
# 2| self
532532
#-----| -> call to x1
533533

534-
# 3| when ...
535-
#-----| -> 1
534+
# 3| [match] when ...
535+
#-----| match -> self
536+
537+
# 3| [no-match] when ...
538+
#-----| no-match -> 2
536539

537540
# 3| 1
538-
#-----| match -> self
539-
#-----| no-match -> when ...
541+
#-----| match -> [match] when ...
542+
#-----| no-match -> [no-match] when ...
540543

541544
# 3| then ...
542545
#-----| -> case ...
@@ -569,12 +572,15 @@ case.rb:
569572
# 3| x2
570573
#-----| -> "x2"
571574

572-
# 4| when ...
573-
#-----| -> 2
575+
# 4| [match] when ...
576+
#-----| match -> self
574577

575-
# 4| 2
578+
# 4| [no-match] when ...
576579
#-----| no-match -> case ...
577-
#-----| match -> self
580+
581+
# 4| 2
582+
#-----| match -> [match] when ...
583+
#-----| no-match -> [no-match] when ...
578584

579585
# 4| then ...
580586
#-----| -> case ...
@@ -1826,17 +1832,20 @@ cfg.rb:
18261832
#-----| -> call to puts
18271833

18281834
# 41| case ...
1829-
#-----| -> when ...
1835+
#-----| -> b
18301836

18311837
# 41| 10
1832-
#-----| -> when ...
1833-
1834-
# 42| when ...
18351838
#-----| -> 1
18361839

1837-
# 42| 1
1840+
# 42| [match] when ...
18381841
#-----| match -> self
1839-
#-----| no-match -> when ...
1842+
1843+
# 42| [no-match] when ...
1844+
#-----| no-match -> 2
1845+
1846+
# 42| 1
1847+
#-----| match -> [match] when ...
1848+
#-----| no-match -> [no-match] when ...
18401849

18411850
# 42| then ...
18421851
#-----| -> case ...
@@ -1853,20 +1862,23 @@ cfg.rb:
18531862
# 42| one
18541863
#-----| -> "one"
18551864

1856-
# 43| when ...
1857-
#-----| -> 2
1865+
# 43| [match] when ...
1866+
#-----| match -> self
1867+
1868+
# 43| [no-match] when ...
1869+
#-----| no-match -> self
18581870

18591871
# 43| 2
1872+
#-----| match -> [match] when ...
18601873
#-----| no-match -> 3
1861-
#-----| match -> self
18621874

18631875
# 43| 3
1876+
#-----| match -> [match] when ...
18641877
#-----| no-match -> 4
1865-
#-----| match -> self
18661878

18671879
# 43| 4
1868-
#-----| match -> self
1869-
#-----| no-match -> self
1880+
#-----| match -> [match] when ...
1881+
#-----| no-match -> [no-match] when ...
18701882

18711883
# 43| then ...
18721884
#-----| -> case ...
@@ -1901,15 +1913,19 @@ cfg.rb:
19011913
# 47| case ...
19021914
#-----| -> chained
19031915

1916+
# 48| [false] when ...
1917+
#-----| false -> b
1918+
19041919
# 48| when ...
1905-
#-----| -> b
1920+
#-----| match -> self
1921+
#-----| no-match -> b
19061922

19071923
# 48| b
19081924
#-----| -> 1
19091925

19101926
# 48| ... == ...
1911-
#-----| true -> self
1912-
#-----| false -> when ...
1927+
#-----| false -> [false] when ...
1928+
#-----| true -> when ...
19131929

19141930
# 48| 1
19151931
#-----| -> ... == ...
@@ -1929,15 +1945,19 @@ cfg.rb:
19291945
# 48| one
19301946
#-----| -> "one"
19311947

1948+
# 49| [false] when ...
1949+
#-----| false -> case ...
1950+
19321951
# 49| when ...
1933-
#-----| -> b
1952+
#-----| no-match -> case ...
1953+
#-----| match -> self
19341954

19351955
# 49| b
19361956
#-----| -> 0
19371957

19381958
# 49| ... == ...
1959+
#-----| true -> when ...
19391960
#-----| false -> b
1940-
#-----| true -> self
19411961

19421962
# 49| 0
19431963
#-----| -> ... == ...
@@ -1946,8 +1966,8 @@ cfg.rb:
19461966
#-----| -> 1
19471967

19481968
# 49| ... > ...
1949-
#-----| false -> case ...
1950-
#-----| true -> self
1969+
#-----| false -> [false] when ...
1970+
#-----| true -> when ...
19511971

19521972
# 49| 1
19531973
#-----| -> ... > ...

‎ruby/ql/test/library-tests/dataflow/barrier-guards/barrier-guards.rb

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -137,9 +137,9 @@
137137

138138
case foo
139139
when "foo", "bar"
140-
foo # $ MISSING: guarded
140+
foo # $ guarded
141141
when "baz", "quux"
142-
foo # $ MISSING: guarded
142+
foo # $ guarded
143143
else
144144
foo
145145
end

0 commit comments

Comments
 (0)