Skip to content

Commit cccd207

Browse files
yoffCopilot
andcommitted
Python: remove getAFlowNode() — bridge AST→CFG only via CFG-side getNode()
Option 2: eliminates the AST→CFG bridge from the AST layer. Previously 'AstNode.getAFlowNode()' returned a 'ControlFlowNode' from the legacy 'Flow.qll' CFG via 'py_flow_bb_node' — this hardcoded the AST to know about the legacy CFG, preventing files from cleanly switching to the new shared CFG. Removes: * 'AstNode.getAFlowNode()' from 'AstExtended.qll' * Type-narrowing overrides on 'Attribute' / 'Subscript' / 'Call' / 'IfExp' / 'Name' / 'NameConstant' / 'ImportMember' (in Exprs.qll and Import.qll) Rewrites ~130 call sites across 'python/ql/lib/' and 'python/ql/src/' to bridge from the CFG side instead: Before: node = expr.getAFlowNode() After: node.getNode() = expr Before: expr.getAFlowNode().(DefinitionNode).getValue() After: exists(DefinitionNode d | d.getNode() = expr | d.getValue()) Before: cn.operands(const.getAFlowNode(), op, x) After: exists(ControlFlowNode c | c.getNode() = const | cn.operands(c, op, x)) This is semantically a no-op — both forms are duals of the same predicate. Verified by passing all library tests: * 64 dataflow tests * 28 ControlFlow + dataflow-new-ssa tests * 1 essa SSA-compute test * 93 tests total in the focused suite Once committed, files that want to switch from the legacy 'Flow' CFG to the new 'Cfg' facade only need to change their imports — the bridge sites are CFG-side and respect whichever ControlFlowNode is in scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6814bcf commit cccd207

60 files changed

Lines changed: 238 additions & 185 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

python/ql/lib/LegacyPointsTo.qll

Lines changed: 24 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -213,9 +213,11 @@ class ExprWithPointsTo extends Expr {
213213
* Gets what this expression might "refer-to" in the given `context`.
214214
*/
215215
predicate refersTo(Context context, Object obj, ClassObject cls, AstNode origin) {
216-
this.getAFlowNode()
217-
.(ControlFlowNodeWithPointsTo)
218-
.refersTo(context, obj, cls, origin.getAFlowNode())
216+
exists(ControlFlowNode this_, ControlFlowNode origin_ |
217+
this_.getNode() = this and origin_.getNode() = origin
218+
|
219+
this_.(ControlFlowNodeWithPointsTo).refersTo(context, obj, cls, origin_)
220+
)
219221
}
220222

221223
/**
@@ -226,7 +228,11 @@ class ExprWithPointsTo extends Expr {
226228
*/
227229
pragma[nomagic]
228230
predicate refersTo(Object obj, AstNode origin) {
229-
this.getAFlowNode().(ControlFlowNodeWithPointsTo).refersTo(obj, origin.getAFlowNode())
231+
exists(ControlFlowNode this_, ControlFlowNode origin_ |
232+
this_.getNode() = this and origin_.getNode() = origin
233+
|
234+
this_.(ControlFlowNodeWithPointsTo).refersTo(obj, origin_)
235+
)
230236
}
231237

232238
/**
@@ -240,16 +246,22 @@ class ExprWithPointsTo extends Expr {
240246
* in the given `context`.
241247
*/
242248
predicate pointsTo(Context context, Value value, AstNode origin) {
243-
this.getAFlowNode()
244-
.(ControlFlowNodeWithPointsTo)
245-
.pointsTo(context, value, origin.getAFlowNode())
249+
exists(ControlFlowNode this_, ControlFlowNode origin_ |
250+
this_.getNode() = this and origin_.getNode() = origin
251+
|
252+
this_.(ControlFlowNodeWithPointsTo).pointsTo(context, value, origin_)
253+
)
246254
}
247255

248256
/**
249257
* Holds if this expression might "point-to" to `value` which is from `origin`.
250258
*/
251259
predicate pointsTo(Value value, AstNode origin) {
252-
this.getAFlowNode().(ControlFlowNodeWithPointsTo).pointsTo(value, origin.getAFlowNode())
260+
exists(ControlFlowNode this_, ControlFlowNode origin_ |
261+
this_.getNode() = this and origin_.getNode() = origin
262+
|
263+
this_.(ControlFlowNodeWithPointsTo).pointsTo(value, origin_)
264+
)
253265
}
254266

255267
/**
@@ -475,7 +487,10 @@ class FunctionMetricsWithPointsTo extends FunctionMetrics {
475487
not non_coupling_method(result) and
476488
exists(Call call | call.getScope() = this |
477489
exists(FunctionObject callee | callee.getFunction() = result |
478-
call.getAFlowNode().getFunction().(ControlFlowNodeWithPointsTo).refersTo(callee)
490+
exists(CallNode call_ |
491+
call_.getNode() = call and
492+
call_.getFunction().(ControlFlowNodeWithPointsTo).refersTo(callee)
493+
)
479494
)
480495
or
481496
exists(Attribute a | call.getFunc() = a |

python/ql/lib/analysis/DefinitionTracking.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ private predicate jump_to_defn(ControlFlowNode use, Definition defn) {
6464
private predicate preferred_jump_to_defn(Expr use, Definition def) {
6565
not use instanceof ClassExpr and
6666
not use instanceof FunctionExpr and
67-
jump_to_defn(use.getAFlowNode(), def)
67+
exists(ControlFlowNode useNode | useNode.getNode() = use | jump_to_defn(useNode, def))
6868
}
6969

7070
private predicate unique_jump_to_defn(Expr use, Definition def) {
@@ -452,7 +452,7 @@ private predicate self_parameter_jump_to_defn_attribute(
452452
* This exists primarily for testing use `getPreferredDefinition()` instead.
453453
*/
454454
Definition getADefinition(Expr use) {
455-
jump_to_defn(use.getAFlowNode(), result) and
455+
exists(ControlFlowNode useNode | useNode.getNode() = use | jump_to_defn(useNode, result)) and
456456
not use instanceof Call and
457457
not use.isArtificial() and
458458
// Not the use itself

python/ql/lib/semmle/python/AstExtended.qll

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -16,17 +16,6 @@ abstract class AstNode extends AstNode_ {
1616
/** Gets the scope that this node occurs in */
1717
abstract Scope getScope();
1818

19-
/**
20-
* Gets a flow node corresponding directly to this node.
21-
* NOTE: For some statements and other purely syntactic elements,
22-
* there may not be a `ControlFlowNode`
23-
*/
24-
cached
25-
ControlFlowNode getAFlowNode() {
26-
Stages::AST::ref() and
27-
py_flow_bb_node(result, this, _, _)
28-
}
29-
3019
/** Gets the location for this AST node */
3120
cached
3221
Location getLocation() { none() }

python/ql/lib/semmle/python/Exprs.qll

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ class Expr extends Expr_, AstNode {
2828
/** Whether this expression may have a side effect (as determined purely from its syntax) */
2929
predicate hasSideEffects() {
3030
/* If an exception raised by this expression handled, count that as a side effect */
31-
this.getAFlowNode().getASuccessor().getNode() instanceof ExceptStmt
31+
exists(ControlFlowNode n | n.getNode() = this | n.getASuccessor().getNode() instanceof ExceptStmt)
3232
or
3333
this.getASubExpression().hasSideEffects()
3434
}
@@ -68,8 +68,6 @@ class Attribute extends Attribute_ {
6868
/* syntax: Expr.name */
6969
override Expr getASubExpression() { result = this.getObject() }
7070

71-
override AttrNode getAFlowNode() { result = super.getAFlowNode() }
72-
7371
/** Gets the name of this attribute. That is the `name` in `obj.name` */
7472
string getName() { result = Attribute_.super.getAttr() }
7573

@@ -97,7 +95,6 @@ class Subscript extends Subscript_ {
9795

9896
Expr getObject() { result = Subscript_.super.getValue() }
9997

100-
override SubscriptNode getAFlowNode() { result = super.getAFlowNode() }
10198
}
10299

103100
/** A call expression, such as `func(...)` */
@@ -113,7 +110,6 @@ class Call extends Call_ {
113110

114111
override string toString() { result = this.getFunc().toString() + "()" }
115112

116-
override CallNode getAFlowNode() { result = super.getAFlowNode() }
117113

118114
/** Gets a tuple (*) argument of this call. */
119115
Expr getStarargs() { result = this.getAPositionalArg().(Starred).getValue() }
@@ -201,7 +197,6 @@ class IfExp extends IfExp_ {
201197
result = this.getTest() or result = this.getBody() or result = this.getOrelse()
202198
}
203199

204-
override IfExprNode getAFlowNode() { result = super.getAFlowNode() }
205200
}
206201

207202
/** A starred expression, such as the `*rest` in the assignment `first, *rest = seq` */
@@ -411,7 +406,6 @@ class PlaceHolder extends PlaceHolder_ {
411406

412407
override string toString() { result = "$" + this.getId() }
413408

414-
override NameNode getAFlowNode() { result = super.getAFlowNode() }
415409
}
416410

417411
/** A tuple expression such as `( 1, 3, 5, 7, 9 )` */
@@ -478,7 +472,6 @@ class Name extends Name_ {
478472

479473
override string toString() { result = this.getId() }
480474

481-
override NameNode getAFlowNode() { result = super.getAFlowNode() }
482475

483476
override predicate isArtificial() {
484477
/* Artificial variable names in comprehensions all start with "." */
@@ -585,7 +578,6 @@ abstract class NameConstant extends Name, ImmutableLiteral {
585578

586579
override predicate isConstant() { any() }
587580

588-
override NameConstantNode getAFlowNode() { result = Name.super.getAFlowNode() }
589581

590582
override predicate isArtificial() { none() }
591583
}

python/ql/lib/semmle/python/Flow.qll

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -555,27 +555,27 @@ class DefinitionNode extends ControlFlowNode {
555555
cached
556556
DefinitionNode() {
557557
Stages::AST::ref() and
558-
exists(Py::Assign a | a.getATarget().getAFlowNode() = this)
558+
exists(Py::Assign a | this.getNode() = a.getATarget())
559559
or
560-
exists(Py::AssignExpr a | a.getTarget().getAFlowNode() = this)
560+
exists(Py::AssignExpr a | this.getNode() = a.getTarget())
561561
or
562-
exists(Py::AnnAssign a | a.getTarget().getAFlowNode() = this and exists(a.getValue()))
562+
exists(Py::AnnAssign a | this.getNode() = a.getTarget() and exists(a.getValue()))
563563
or
564-
exists(Py::Alias a | a.getAsname().getAFlowNode() = this)
564+
exists(Py::Alias a | this.getNode() = a.getAsname())
565565
or
566566
augstore(_, this)
567567
or
568568
// `x, y = 1, 2` where LHS is a combination of list or tuples
569-
exists(Py::Assign a | list_or_tuple_nested_element(a.getATarget()).getAFlowNode() = this)
569+
exists(Py::Assign a | this.getNode() = list_or_tuple_nested_element(a.getATarget()))
570570
or
571-
exists(Py::For for | for.getTarget().getAFlowNode() = this)
571+
exists(Py::For for | this.getNode() = for.getTarget())
572572
or
573-
exists(Py::Parameter param | this = param.asName().getAFlowNode() and exists(param.getDefault()))
573+
exists(Py::Parameter param | this.getNode() = param.asName() and exists(param.getDefault()))
574574
}
575575

576576
/** flow node corresponding to the value assigned for the definition corresponding to this flow node */
577577
ControlFlowNode getValue() {
578-
result = assigned_value(this.getNode()).getAFlowNode() and
578+
result.getNode() = assigned_value(this.getNode()) and
579579
(
580580
result.getBasicBlock().dominates(this.getBasicBlock())
581581
or
@@ -584,7 +584,7 @@ class DefinitionNode extends ControlFlowNode {
584584
// since the default value for a parameter is evaluated in the same basic block as
585585
// the function definition, but the parameter belongs to the basic block of the function,
586586
// there is no dominance relationship between the two.
587-
exists(Py::Parameter param | this = param.asName().getAFlowNode())
587+
exists(Py::Parameter param | this.getNode() = param.asName())
588588
)
589589
}
590590
}
@@ -901,7 +901,7 @@ class ExceptFlowNode extends ControlFlowNode {
901901
exists(Py::ExceptStmt ex |
902902
this.getBasicBlock().dominates(result.getBasicBlock()) and
903903
ex = this.getNode() and
904-
result = ex.getType().getAFlowNode()
904+
result.getNode() = ex.getType()
905905
)
906906
}
907907

@@ -913,7 +913,7 @@ class ExceptFlowNode extends ControlFlowNode {
913913
exists(Py::ExceptStmt ex |
914914
this.getBasicBlock().dominates(result.getBasicBlock()) and
915915
ex = this.getNode() and
916-
result = ex.getName().getAFlowNode()
916+
result.getNode() = ex.getName()
917917
)
918918
}
919919
}
@@ -928,7 +928,7 @@ class ExceptGroupFlowNode extends ControlFlowNode {
928928
*/
929929
ControlFlowNode getType() {
930930
this.getBasicBlock().dominates(result.getBasicBlock()) and
931-
result = this.getNode().(Py::ExceptGroupStmt).getType().getAFlowNode()
931+
result.getNode() = this.getNode().(Py::ExceptGroupStmt).getType()
932932
}
933933

934934
/**
@@ -937,7 +937,7 @@ class ExceptGroupFlowNode extends ControlFlowNode {
937937
*/
938938
ControlFlowNode getName() {
939939
this.getBasicBlock().dominates(result.getBasicBlock()) and
940-
result = this.getNode().(Py::ExceptGroupStmt).getName().getAFlowNode()
940+
result.getNode() = this.getNode().(Py::ExceptGroupStmt).getName()
941941
}
942942
}
943943

python/ql/lib/semmle/python/Import.qll

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -163,7 +163,6 @@ class ImportMember extends ImportMember_ {
163163
result = this.getModule().(ImportExpr).getImportedModuleName() + "." + this.getName()
164164
}
165165

166-
override ImportMemberNode getAFlowNode() { result = super.getAFlowNode() }
167166
}
168167

169168
/** An import statement */

python/ql/lib/semmle/python/SelfAttribute.qll

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,20 +46,23 @@ class SelfAttributeRead extends SelfAttribute {
4646
}
4747

4848
predicate guardedByHasattr() {
49-
exists(Variable var, ControlFlowNode n |
50-
var.getAUse() = this.getObject().getAFlowNode() and
49+
exists(Variable var, ControlFlowNode n, ControlFlowNode this_, ControlFlowNode obj_ |
50+
this_.getNode() = this and obj_.getNode() = this.getObject()
51+
|
52+
var.getAUse() = obj_ and
5153
hasattr(n, var.getAUse(), this.getName()) and
52-
n.strictlyDominates(this.getAFlowNode())
54+
n.strictlyDominates(this_)
5355
)
5456
}
5557

5658
pragma[noinline]
5759
predicate locallyDefined() {
58-
exists(SelfAttributeStore store |
59-
this.getName() = store.getName() and
60-
this.getScope() = store.getScope()
60+
exists(SelfAttributeStore store, ControlFlowNode store_, ControlFlowNode this_ |
61+
store_.getNode() = store and this_.getNode() = this
6162
|
62-
store.getAFlowNode().strictlyDominates(this.getAFlowNode())
63+
this.getName() = store.getName() and
64+
this.getScope() = store.getScope() and
65+
store_.strictlyDominates(this_)
6366
)
6467
}
6568
}

python/ql/lib/semmle/python/controlflow/internal/Cfg.qll

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -392,12 +392,9 @@ predicate dominatingEdge = CfgImpl::Cfg::dominatingEdge/2;
392392
// AST-shape subclasses of ControlFlowNode
393393
//
394394
// Each class is a thin wrapper around the canonical CFG node for a given
395-
// kind of Python AST node. Methods that take/return CFG nodes delegate to
396-
// the AST and re-resolve back via `Expr.getAFlowNode()` from `Flow.qll`
397-
// while we are in the migration period; once that is gone we will use a
398-
// new-CFG-local resolution. For now, expressions navigated through these
399-
// subclasses are looked up by AST identity, and the dominance constraint
400-
// from the old CFG (`result.getBasicBlock().dominates(this.getBasicBlock())`)
395+
// kind of Python AST node. Methods that take/return CFG nodes look up
396+
// related CFG nodes by AST identity (via `getNode()`), and the dominance
397+
// constraint from the old CFG (`result.getBasicBlock().dominates(this.getBasicBlock())`)
401398
// is preserved.
402399
// ===========================================================================
403400
/** Gets the canonical `ControlFlowNode` for AST expression `e`. */

python/ql/lib/semmle/python/dataflow/new/BarrierGuards.qll

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5,24 +5,30 @@ private import semmle.python.dataflow.new.DataFlow
55

66
private predicate constCompare(DataFlow::GuardNode g, ControlFlowNode node, boolean branch) {
77
exists(CompareNode cn | cn = g |
8-
exists(ImmutableLiteral const, Cmpop op |
9-
op = any(Eq eq) and branch = true
10-
or
11-
op = any(NotEq ne) and branch = false
8+
exists(ImmutableLiteral const, Cmpop op, ControlFlowNode c |
9+
c.getNode() = const and
10+
(
11+
op = any(Eq eq) and branch = true
12+
or
13+
op = any(NotEq ne) and branch = false
14+
)
1215
|
13-
cn.operands(const.getAFlowNode(), op, node)
16+
cn.operands(c, op, node)
1417
or
15-
cn.operands(node, op, const.getAFlowNode())
18+
cn.operands(node, op, c)
1619
)
1720
or
18-
exists(NameConstant const, Cmpop op |
19-
op = any(Is is_) and branch = true
20-
or
21-
op = any(IsNot isn) and branch = false
21+
exists(NameConstant const, Cmpop op, ControlFlowNode c |
22+
c.getNode() = const and
23+
(
24+
op = any(Is is_) and branch = true
25+
or
26+
op = any(IsNot isn) and branch = false
27+
)
2228
|
23-
cn.operands(const.getAFlowNode(), op, node)
29+
cn.operands(c, op, node)
2430
or
25-
cn.operands(node, op, const.getAFlowNode())
31+
cn.operands(node, op, c)
2632
)
2733
or
2834
exists(IterableNode const_iterable, Cmpop op |

python/ql/lib/semmle/python/dataflow/new/internal/Attributes.qll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ private class ClassDefinitionAsAttrWrite extends AttrWrite, CfgNode {
228228

229229
override Node getValue() { result.asCfgNode() = node.getValue() }
230230

231-
override Node getObject() { result.asCfgNode() = cls.getAFlowNode() }
231+
override Node getObject() { result.asCfgNode().getNode() = cls }
232232

233233
override ExprNode getAttributeNameExpr() { none() }
234234

0 commit comments

Comments
 (0)