Skip to content

Commit 3994566

Browse files
hvitvedhvitved
committed
Rust: Handle chained let expressions
1 parent d7abfc8 commit 3994566

File tree

11 files changed

+186
-66
lines changed

11 files changed

+186
-66
lines changed

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

Lines changed: 7 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -200,8 +200,7 @@ class TypeReprTree extends LeafTree instanceof TypeRepr { }
200200
/**
201201
* Provides `ControlFlowTree`s for expressions.
202202
*
203-
* Since expressions construct values, they are modeled in post-order, except for
204-
* `LetExpr`s.
203+
* Since expressions construct values, they are modeled in post-order.
205204
*/
206205
module ExprTrees {
207206
class ArrayExprTree extends StandardPostOrderTree, ArrayExpr {
@@ -341,21 +340,15 @@ module ExprTrees {
341340
child = [super.getCondition(), super.getABranch()]
342341
}
343342

344-
private ConditionalCompletion conditionCompletion(Completion c) {
345-
if super.getCondition() instanceof LetExpr
346-
then result = c.(MatchCompletion)
347-
else result = c.(BooleanCompletion)
348-
}
349-
350343
override predicate succ(AstNode pred, AstNode succ, Completion c) {
351344
// Edges from the condition to the branches
352345
last(super.getCondition(), pred, c) and
353346
(
354-
first(super.getThen(), succ) and this.conditionCompletion(c).succeeded()
347+
first(super.getThen(), succ) and c.(ConditionalCompletion).succeeded()
355348
or
356-
first(super.getElse(), succ) and this.conditionCompletion(c).failed()
349+
first(super.getElse(), succ) and c.(ConditionalCompletion).failed()
357350
or
358-
not super.hasElse() and succ = this and this.conditionCompletion(c).failed()
351+
not super.hasElse() and succ = this and c.(ConditionalCompletion).failed()
359352
)
360353
or
361354
// An edge from the then branch to the last node
@@ -401,10 +394,7 @@ module ExprTrees {
401394
}
402395
}
403396

404-
// `LetExpr` is a pre-order tree such that the pattern itself ends up
405-
// dominating successors in the graph in the same way that patterns do in
406-
// `match` expressions.
407-
class LetExprTree extends StandardPreOrderTree, LetExpr {
397+
class LetExprTree extends StandardPostOrderTree, LetExpr {
408398
override AstNode getChildNode(int i) {
409399
i = 0 and
410400
result = this.getScrutinee()
@@ -456,21 +446,15 @@ module ExprTrees {
456446

457447
override predicate first(AstNode node) { first(super.getCondition(), node) }
458448

459-
private ConditionalCompletion conditionCompletion(Completion c) {
460-
if super.getCondition() instanceof LetExpr
461-
then result = c.(MatchCompletion)
462-
else result = c.(BooleanCompletion)
463-
}
464-
465449
override predicate succ(AstNode pred, AstNode succ, Completion c) {
466450
super.succ(pred, succ, c)
467451
or
468452
last(super.getCondition(), pred, c) and
469-
this.conditionCompletion(c).succeeded() and
453+
c.(ConditionalCompletion).succeeded() and
470454
first(this.getLoopBody(), succ)
471455
or
472456
last(super.getCondition(), pred, c) and
473-
this.conditionCompletion(c).failed() and
457+
c.(ConditionalCompletion).failed() and
474458
succ = this
475459
}
476460
}

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,7 @@ module ConditionalCompletionSplitting {
7171
child = parent.(LogicalNotExpr).getExpr() and
7272
childCompletion.getDual() = parentCompletion
7373
or
74-
(
75-
childCompletion = parentCompletion
76-
or
77-
// needed for `let` expressions
78-
childCompletion.(MatchCompletion).getValue() =
79-
parentCompletion.(BooleanCompletion).getValue()
80-
) and
74+
childCompletion = parentCompletion and
8175
(
8276
child = parent.(BinaryLogicalOperation).getAnOperand()
8377
or
@@ -92,6 +86,9 @@ module ConditionalCompletionSplitting {
9286
or
9387
child = parent.(PatternTrees::PostOrderPatTree).getPat(_)
9488
)
89+
or
90+
child = parent.(LetExpr).getPat() and
91+
childCompletion.(MatchCompletion).getValue() = parentCompletion.(BooleanCompletion).getValue()
9592
}
9693
}
9794

‎rust/ql/lib/codeql/rust/dataflow/internal/DataFlowImpl.qll

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,12 @@ module LocalFlow {
241241
nodeTo.getCfgNode() = s.getPat()
242242
)
243243
or
244+
// An edge from the right-hand side of a let expression to the left-hand side.
245+
exists(LetExprCfgNode e |
246+
nodeFrom.getCfgNode() = e.getScrutinee() and
247+
nodeTo.getCfgNode() = e.getPat()
248+
)
249+
or
244250
exists(IdentPatCfgNode p |
245251
not p.isRef() and
246252
nodeFrom.getCfgNode() = p and
@@ -379,6 +385,8 @@ module RustDataFlow implements InputSig<Location> {
379385
predicate neverSkipInPathGraph(Node node) {
380386
node.(Node::Node).getCfgNode() = any(LetStmtCfgNode s).getPat()
381387
or
388+
node.(Node::Node).getCfgNode() = any(LetExprCfgNode e).getPat()
389+
or
382390
node.(Node::Node).getCfgNode() = any(AssignmentExprCfgNode a).getLhs()
383391
or
384392
exists(MatchExprCfgNode match |
@@ -899,6 +907,12 @@ module VariableCapture {
899907
v.getPat() = ls.getPat().getPat() and
900908
ls.getInitializer() = source
901909
)
910+
or
911+
exists(LetExprCfgNode le |
912+
this = le and
913+
v.getPat() = le.getPat().getPat() and
914+
le.getScrutinee() = source
915+
)
902916
}
903917

904918
CapturedVariable getVariable() { result = v }

‎rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,12 @@ module Impl {
5555
not exists(getEnclosingOrPat(result))
5656
}
5757

58+
private predicate testvariableDecl(AstNode definingNode, Name name, string text) {
59+
variableDecl(definingNode, name, text) and
60+
definingNode.fromSource() and
61+
definingNode.getLocation().getStartLine() = 276
62+
}
63+
5864
/**
5965
* Holds if `name` declares a variable named `text` at `definingNode`.
6066
* Normally, `definingNode = name`, except in cases like
@@ -152,8 +158,14 @@ module Impl {
152158
/** Gets the `let` statement that introduces this variable, if any. */
153159
LetStmt getLetStmt() { this.getPat() = result.getPat() }
154160

161+
/** Gets the `let` expression that introduces this variable, if any. */
162+
LetExpr getLetExpr() { this.getPat() = result.getPat() }
163+
155164
/** Gets the initial value of this variable, if any. */
156-
Expr getInitializer() { result = this.getLetStmt().getInitializer() }
165+
Expr getInitializer() {
166+
result = this.getLetStmt().getInitializer() or
167+
result = this.getLetExpr().getScrutinee()
168+
}
157169

158170
/** Holds if this variable is captured. */
159171
predicate isCaptured() { this.getAnAccess().isCapture() }
@@ -198,6 +210,7 @@ module Impl {
198210
n instanceof Pat or
199211
n instanceof VariableAccessCand or
200212
n instanceof LetStmt or
213+
n instanceof LetExpr or
201214
n instanceof VariableScope
202215
) and
203216
exists(AstNode n0 |
@@ -243,6 +256,7 @@ module Impl {
243256
this instanceof VariableScope or
244257
this instanceof VariableAccessCand or
245258
this instanceof LetStmt or
259+
this instanceof LetExpr or
246260
getImmediateChild(this, _) instanceof RelevantElement
247261
}
248262

@@ -354,6 +368,14 @@ module Impl {
354368
ord = getLastPreOrderNumbering(scope, let) + 1
355369
)
356370
or
371+
exists(LetExpr let |
372+
let.getPat() = pat and
373+
scope = getEnclosingScope(let) and
374+
// for `let` expressions, variables are bound _after_ the statement, i.e.
375+
// not in the RHS
376+
ord = getLastPreOrderNumbering(scope, let) + 1
377+
)
378+
or
357379
exists(IfExpr ie, LetExpr let |
358380
let.getPat() = pat and
359381
ie.getCondition() = let and

‎rust/ql/test/library-tests/dataflow/local/DataFlowStep.expected

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,17 @@ localStep
3030
| main.rs:26:12:26:12 | [SSA] x | main.rs:27:14:27:14 | x |
3131
| main.rs:26:12:26:12 | x | main.rs:26:12:26:12 | [SSA] x |
3232
| main.rs:26:12:26:12 | x | main.rs:26:12:26:12 | x |
33+
| main.rs:26:16:26:16 | s | main.rs:26:12:26:12 | x |
3334
| main.rs:26:16:26:16 | s | main.rs:30:16:30:16 | s |
3435
| main.rs:26:18:28:5 | { ... } | main.rs:26:5:28:5 | if ... {...} |
36+
| main.rs:30:12:30:12 | [SSA] x | main.rs:32:18:32:18 | x |
37+
| main.rs:30:12:30:12 | x | main.rs:30:12:30:12 | [SSA] x |
3538
| main.rs:30:12:30:12 | x | main.rs:30:12:30:12 | x |
39+
| main.rs:30:16:30:16 | s | main.rs:30:12:30:12 | x |
40+
| main.rs:32:18:32:18 | [post] x | main.rs:36:14:36:14 | x |
41+
| main.rs:32:18:32:18 | x | main.rs:36:14:36:14 | x |
42+
| main.rs:33:13:33:16 | true | main.rs:31:12:34:9 | { ... } |
43+
| main.rs:35:5:37:5 | { ... } | main.rs:30:5:37:5 | if ... {...} |
3644
| main.rs:40:18:40:21 | [SSA] cond | main.rs:43:16:43:19 | cond |
3745
| main.rs:40:18:40:21 | cond | main.rs:40:18:40:21 | [SSA] cond |
3846
| main.rs:40:18:40:21 | cond | main.rs:40:18:40:21 | cond |
@@ -294,7 +302,14 @@ localStep
294302
| main.rs:254:9:254:10 | s1 | main.rs:254:9:254:10 | [SSA] s1 |
295303
| main.rs:254:9:254:10 | s1 | main.rs:254:9:254:10 | s1 |
296304
| main.rs:254:14:254:29 | Some(...) | main.rs:254:9:254:10 | s1 |
305+
| main.rs:255:5:262:5 | if ... {...} | main.rs:253:25:263:1 | { ... } |
306+
| main.rs:255:17:255:17 | [SSA] n | main.rs:257:18:257:18 | n |
307+
| main.rs:255:17:255:17 | n | main.rs:255:17:255:17 | [SSA] n |
297308
| main.rs:255:17:255:17 | n | main.rs:255:17:255:17 | n |
309+
| main.rs:257:18:257:18 | [post] n | main.rs:261:14:261:14 | n |
310+
| main.rs:257:18:257:18 | n | main.rs:261:14:261:14 | n |
311+
| main.rs:258:13:258:16 | true | main.rs:256:12:259:9 | { ... } |
312+
| main.rs:260:5:262:5 | { ... } | main.rs:255:5:262:5 | if ... {...} |
298313
| main.rs:266:9:266:10 | [SSA] s1 | main.rs:267:10:267:11 | s1 |
299314
| main.rs:266:9:266:10 | s1 | main.rs:266:9:266:10 | [SSA] s1 |
300315
| main.rs:266:9:266:10 | s1 | main.rs:266:9:266:10 | s1 |

‎rust/ql/test/library-tests/dataflow/local/inline-flow.expected

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,12 @@ models
1111
| 10 | Summary: <core::result::Result>::ok; Argument[self].Field[core::result::Result::Ok(0)]; ReturnValue.Field[core::option::Option::Some(0)]; value |
1212
edges
1313
| main.rs:23:9:23:9 | s | main.rs:24:10:24:10 | s | provenance | |
14+
| main.rs:23:9:23:9 | s | main.rs:26:12:26:12 | x | provenance | |
15+
| main.rs:23:9:23:9 | s | main.rs:30:12:30:12 | x | provenance | |
1416
| main.rs:23:13:23:21 | source(...) | main.rs:23:9:23:9 | s | provenance | |
17+
| main.rs:26:12:26:12 | x | main.rs:27:14:27:14 | x | provenance | |
18+
| main.rs:30:12:30:12 | x | main.rs:32:18:32:18 | x | provenance | |
19+
| main.rs:30:12:30:12 | x | main.rs:36:14:36:14 | x | provenance | |
1520
| main.rs:41:9:41:9 | a | main.rs:43:9:43:9 | c | provenance | |
1621
| main.rs:41:13:41:21 | source(...) | main.rs:41:9:41:9 | a | provenance | |
1722
| main.rs:43:9:43:9 | c | main.rs:44:10:44:10 | c | provenance | |
@@ -240,6 +245,11 @@ nodes
240245
| main.rs:23:9:23:9 | s | semmle.label | s |
241246
| main.rs:23:13:23:21 | source(...) | semmle.label | source(...) |
242247
| main.rs:24:10:24:10 | s | semmle.label | s |
248+
| main.rs:26:12:26:12 | x | semmle.label | x |
249+
| main.rs:27:14:27:14 | x | semmle.label | x |
250+
| main.rs:30:12:30:12 | x | semmle.label | x |
251+
| main.rs:32:18:32:18 | x | semmle.label | x |
252+
| main.rs:36:14:36:14 | x | semmle.label | x |
243253
| main.rs:41:9:41:9 | a | semmle.label | a |
244254
| main.rs:41:13:41:21 | source(...) | semmle.label | source(...) |
245255
| main.rs:43:9:43:9 | c | semmle.label | c |
@@ -512,6 +522,9 @@ testFailures
512522
#select
513523
| main.rs:19:10:19:18 | source(...) | main.rs:19:10:19:18 | source(...) | main.rs:19:10:19:18 | source(...) | $@ | main.rs:19:10:19:18 | source(...) | source(...) |
514524
| main.rs:24:10:24:10 | s | main.rs:23:13:23:21 | source(...) | main.rs:24:10:24:10 | s | $@ | main.rs:23:13:23:21 | source(...) | source(...) |
525+
| main.rs:27:14:27:14 | x | main.rs:23:13:23:21 | source(...) | main.rs:27:14:27:14 | x | $@ | main.rs:23:13:23:21 | source(...) | source(...) |
526+
| main.rs:32:18:32:18 | x | main.rs:23:13:23:21 | source(...) | main.rs:32:18:32:18 | x | $@ | main.rs:23:13:23:21 | source(...) | source(...) |
527+
| main.rs:36:14:36:14 | x | main.rs:23:13:23:21 | source(...) | main.rs:36:14:36:14 | x | $@ | main.rs:23:13:23:21 | source(...) | source(...) |
515528
| main.rs:44:10:44:10 | c | main.rs:41:13:41:21 | source(...) | main.rs:44:10:44:10 | c | $@ | main.rs:41:13:41:21 | source(...) | source(...) |
516529
| main.rs:53:10:53:10 | b | main.rs:48:13:48:21 | source(...) | main.rs:53:10:53:10 | b | $@ | main.rs:48:13:48:21 | source(...) | source(...) |
517530
| main.rs:64:10:64:10 | b | main.rs:62:15:62:23 | source(...) | main.rs:64:10:64:10 | b | $@ | main.rs:62:15:62:23 | source(...) | source(...) |

‎rust/ql/test/library-tests/dataflow/local/main.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,16 @@ fn variable_usage() {
2424
sink(s); // $ hasValueFlow=2
2525

2626
if let x = s {
27-
sink(x); // $ MISSING: hasValueFlow=2
27+
sink(x); // $ hasValueFlow=2
2828
};
2929

3030
if let x = s
3131
&& {
32-
sink(x); // $ MISSING: hasValueFlow=2
32+
sink(x); // $ hasValueFlow=2
3333
true
3434
}
3535
{
36-
sink(x); // $ MISSING: hasValueFlow=2
36+
sink(x); // $ hasValueFlow=2
3737
};
3838
}
3939

0 commit comments

Comments
 (0)