Skip to content

Commit 84ed5d6

Browse files
owen-mcowen-mc
committed
Address review comments
1 parent 755e78b commit 84ed5d6

9 files changed

Lines changed: 13 additions & 260 deletions

File tree

‎actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll‎

Lines changed: 0 additions & 112 deletions
This file was deleted.

‎actions/ql/lib/codeql/actions/controlflow/internal/Cfg.qll‎

Lines changed: 7 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -77,16 +77,6 @@ module CfgImpl {
7777
)
7878
}
7979

80-
private AstNode getLastCfgAstNode(AstNode node) {
81-
not exists(getCfgChild(node, _)) and result = node
82-
or
83-
exists(AstNode child, int index |
84-
child = getCfgChild(node, index) and
85-
not exists(int later | later > index and exists(getCfgChild(node, later))) and
86-
result = getLastCfgAstNode(child)
87-
)
88-
}
89-
9080
private module CfgAst implements CfgShared::AstSig<Location> {
9181
class AstNode = ActionsAstNode;
9282

@@ -397,54 +387,23 @@ module CfgImpl {
397387
/** A CFG scope for a composite action. */
398388
class CompositeActionScope extends CfgScope instanceof CompositeAction { }
399389

400-
/**
401-
* A control flow node.
402-
*
403-
* Only nodes that can be reached from an entry point are included in the CFG.
404-
*/
405-
class Node extends ControlFlowNode {
406-
/** Gets the CFG scope containing this node. */
407-
CfgScope getScope() { result = this.getEnclosingCallable() }
408-
409-
Node getASuccessor(SuccessorType type) { result = super.getASuccessor(type) }
410-
411-
Node getASuccessor() { result = super.getASuccessor() }
412-
413-
/** Gets an immediate predecessor connected by an edge of type `type`, if any. */
414-
Node getAPredecessor(SuccessorType type) { result.getASuccessor(type) = this }
415-
416-
Node getAPredecessor() { result = super.getAPredecessor() }
417-
418-
/** Holds if this node has a conditional successor. */
419-
predicate isCondition() { exists(this.getASuccessor(any(ConditionalSuccessor successor))) }
420-
421-
/** Holds if this node has more than one predecessor. */
422-
predicate isJoin() { strictcount(this.getAPredecessor()) > 1 }
423-
424-
/** Holds if this node has more than one successor. */
425-
predicate isBranch() { strictcount(this.getASuccessor()) > 1 }
426-
}
390+
/** A control flow node. */
391+
class Node = ControlFlowNode;
427392

428393
/** The control flow node at the entry point of a scope. */
429-
class EntryNode extends Node, ControlFlow::EntryNode { }
394+
class EntryNode = ControlFlow::EntryNode;
430395

431396
/** A control flow node indicating normal or exceptional termination of a scope. */
432-
class AnnotatedExitNode extends Node, ControlFlow::AnnotatedExitNode {
433-
/** Holds if this node represents a normal exit. */
434-
predicate isNormal() { this instanceof NormalExitNode }
435-
}
397+
class AnnotatedExitNode = ControlFlow::AnnotatedExitNode;
436398

437399
/** A control flow node indicating normal termination of a scope. */
438-
class NormalExitNode extends AnnotatedExitNode, ControlFlow::NormalExitNode { }
400+
class NormalExitNode = ControlFlow::NormalExitNode;
439401

440402
/** A control flow node indicating exceptional termination of a scope. */
441-
class ExceptionalExitNode extends AnnotatedExitNode, ControlFlow::ExceptionalExitNode { }
403+
class ExceptionalExitNode = ControlFlow::ExceptionalExitNode;
442404

443405
/** A control flow node indicating the termination of a scope. */
444-
class ExitNode extends Node, ControlFlow::ExitNode { }
445-
446-
/** The empty split type retained for compatibility with the legacy Actions CFG. */
447-
class Split = Void;
406+
class ExitNode = ControlFlow::ExitNode;
448407

449408
/**
450409
* A node that uniquely represents an AST node.
@@ -455,35 +414,5 @@ module CfgImpl {
455414
AstCfgNode() { this.injects(_) }
456415

457416
AstNode getAstNode() { this.injects(result) }
458-
459-
/** Gets a comma-separated list of splits in this node, if any. */
460-
string getSplitsString() { none() }
461-
462-
/** Gets a split for this control flow node, if any. */
463-
Split getASplit() { none() }
464417
}
465-
466-
/**
467-
* If needed, call this predicate to force a stage dependency on the cached CFG stage.
468-
*/
469-
cached
470-
predicate forceCachingInSameStage() { CfgCachedStage::ref() }
471-
472-
/** Gets the first AST node executed within `node`. */
473-
cached
474-
AstNode getAControlFlowEntryNode(AstNode node) {
475-
result = node and
476-
exists(Node cfgNode | cfgNode.injects(node))
477-
}
478-
479-
/** Gets a potential last AST node executed within `node`. */
480-
cached
481-
AstNode getAControlFlowExitNode(AstNode node) {
482-
exists(Node cfgNode | cfgNode.injects(node)) and
483-
result = getLastCfgAstNode(node)
484-
}
485-
486-
/** Gets the CFG scope of `node`. */
487-
cached
488-
CfgScope getNodeCfgScope(Node node) { result = node.getScope() }
489418
}

‎actions/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ private import codeql.dataflow.DataFlow
33
private import codeql.actions.Ast
44
private import codeql.actions.Cfg as Cfg
55
private import codeql.Locations
6-
private import codeql.actions.controlflow.BasicBlocks
76
private import DataFlowPublic
87
private import codeql.actions.dataflow.ExternalFlow
98
private import codeql.actions.dataflow.FlowSteps
@@ -50,7 +49,7 @@ predicate isArgumentNode(ArgumentNode arg, DataFlowCall call, ArgumentPosition p
5049
}
5150

5251
DataFlowCallable nodeGetEnclosingCallable(Node node) {
53-
node = TExprNode(any(DataFlowExpr e | result = e.getScope()))
52+
node = TExprNode(any(DataFlowExpr e | result = e.getEnclosingCallable()))
5453
}
5554

5655
DataFlowType getNodeType(Node node) { any() }
@@ -86,7 +85,7 @@ class DataFlowCall instanceof Cfg::Node {
8685

8786
string getName() { result = super.getAstNode().(Uses).getCallee() }
8887

89-
DataFlowCallable getEnclosingCallable() { result = super.getScope() }
88+
DataFlowCallable getEnclosingCallable() { result = super.getEnclosingCallable() }
9089

9190
/** Gets a best-effort total ordering. */
9291
int totalorder() { none() }

‎actions/ql/test/library-tests/basic/test.expected‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1293,7 +1293,6 @@ cfgNodes
12931293
| .github/workflows/test.yml:40:14:40:52 | echo ${{needs.job1.outputs.job_output}} |
12941294
| .github/workflows/test.yml:40:20:40:53 | needs.job1.outputs.job_output |
12951295
cfgCycles
1296-
cfgDeadEnds
12971296
dfNodes
12981297
| .github/workflows/commands.yml:9:5:31:2 | Job: local_commands |
12991298
| .github/workflows/commands.yml:15:9:18:6 | Run Step |
@@ -1747,16 +1746,6 @@ workflowScopes
17471746
| .github/workflows/shell.yml:1:1:22:32 | on: push |
17481747
| .github/workflows/test.yml:1:1:40:53 | on: push |
17491748
compositeActionScopes
1750-
workflowCfgBounds
1751-
| .github/workflows/commands.yml:1:1:39:30 | on: push | 1 | 38 |
1752-
| .github/workflows/controlcheck.yml:1:1:16:25 | on: | 1 | 16 |
1753-
| .github/workflows/expression_nodes.yml:1:1:21:47 | on: issue_comment | 1 | 20 |
1754-
| .github/workflows/many_strings.yml:1:1:18:1211 | on: | 1 | 11 |
1755-
| .github/workflows/multiline2.yml:1:1:89:35 | on: | 1 | 86 |
1756-
| .github/workflows/multiline.yml:1:1:89:29 | on: | 1 | 86 |
1757-
| .github/workflows/poisonable_steps.yml:1:1:46:111 | on: push | 1 | 44 |
1758-
| .github/workflows/shell.yml:1:1:22:32 | on: push | 1 | 22 |
1759-
| .github/workflows/test.yml:1:1:40:53 | on: push | 1 | 40 |
17601749
workflowCfgNodes
17611750
| .github/workflows/commands.yml:1:1:39:30 | on: push |
17621751
| .github/workflows/controlcheck.yml:1:1:16:25 | on: |
@@ -1787,8 +1776,6 @@ normalExitNodes
17871776
| .github/workflows/poisonable_steps.yml:1:1:46:111 | Normal Exit |
17881777
| .github/workflows/shell.yml:1:1:22:32 | Normal Exit |
17891778
| .github/workflows/test.yml:1:1:40:53 | Normal Exit |
1790-
legacyNodeProperties
1791-
legacyCfgSplits
17921779
sources
17931780
| AvraamMavridis/files-changed-action | * | output.CHANGED_FILES | filename | manual |
17941781
| AvraamMavridis/files-changed-action | * | output.CHANGED_FILES_EXTENSIONS | filename | manual |

‎actions/ql/test/library-tests/basic/test.ql‎

Lines changed: 4 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -39,11 +39,6 @@ query predicate cfgNodes(Cfg::Node n) { any() }
3939

4040
query predicate cfgCycles(Cfg::Node n) { n.getASuccessor+() = n }
4141

42-
query predicate cfgDeadEnds(Cfg::Node n) {
43-
not n instanceof Cfg::ExitNode and
44-
not exists(n.getASuccessor())
45-
}
46-
4742
query predicate dfNodes(DataFlow::Node e) { any() }
4843

4944
query predicate argumentNodes(DataFlow::ArgumentNode e) { any() }
@@ -58,43 +53,16 @@ query predicate workflowScopes(Cfg::WorkflowScope c) { any() }
5853

5954
query predicate compositeActionScopes(Cfg::CompositeActionScope c) { any() }
6055

61-
query predicate workflowCfgBounds(Workflow workflow, int entryLine, int exitLine) {
62-
exists(AstNode entry, AstNode exit |
63-
entry = Cfg::getAControlFlowEntryNode(workflow) and
64-
exit = Cfg::getAControlFlowExitNode(workflow) and
65-
entryLine = entry.getLocation().getStartLine() and
66-
exitLine = exit.getLocation().getStartLine()
67-
)
68-
}
69-
7056
query predicate workflowCfgNodes(Cfg::AstCfgNode node) {
71-
Cfg::forceCachingInSameStage() and
7257
node.getAstNode() instanceof Workflow and
73-
Cfg::getNodeCfgScope(node) = node.getAstNode()
58+
node.getEnclosingCallable() = node.getAstNode()
7459
}
7560

76-
query predicate entryScopes(Cfg::EntryNode entry, Cfg::CfgScope scope) { scope = entry.getScope() }
77-
78-
query predicate normalExitNodes(Cfg::AnnotatedExitNode exit) { exit.isNormal() }
79-
80-
query predicate legacyNodeProperties(
81-
Cfg::Node node, Cfg::SuccessorType successorType, string property
82-
) {
83-
node = node.getASuccessor(successorType) and property = "successor"
84-
or
85-
node = node.getAPredecessor(successorType) and property = "predecessor"
86-
or
87-
node.isCondition() and property = "condition"
88-
or
89-
node.isJoin() and property = "join"
90-
or
91-
node.isBranch() and property = "branch"
61+
query predicate entryScopes(Cfg::EntryNode entry, Cfg::CfgScope scope) {
62+
scope = entry.getEnclosingCallable()
9263
}
9364

94-
query predicate legacyCfgSplits(Cfg::AstCfgNode node) {
95-
exists(node.getSplitsString()) or
96-
exists(node.getASplit())
97-
}
65+
query predicate normalExitNodes(Cfg::AnnotatedExitNode exit) { exit instanceof Cfg::NormalExitNode }
9866

9967
query predicate sources(string action, string version, string output, string kind, string provenance) {
10068
actionsSourceModel(action, version, output, kind, provenance)

‎actions/ql/test/library-tests/cfg-callable/test.expected‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,3 @@ roots
22
| 1 | 1 |
33
cfgScopes
44
| action.yml:1:1:12:61 | name: Ambiguous root | composite action |
5-
cfgConsistency

‎actions/ql/test/library-tests/cfg-callable/test.ql‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,3 @@ query predicate cfgScopes(Cfg::CfgScope scope, string kind) {
1616
scope instanceof Cfg::CompositeActionScope and kind = "composite action"
1717
)
1818
}
19-
20-
query predicate cfgConsistency(string query, int results) {
21-
Cfg::Consistency::consistencyOverview(query, results) and
22-
results != 0
23-
}

‎actions/ql/test/library-tests/cfg-environment/test.expected‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,5 +3,3 @@ envCfgNodes
33
| .github/workflows/test.yml:10:19:10:48 | github.event.issue.body |
44
| .github/workflows/test.yml:16:24:16:42 | github.actor |
55
cfgCycles
6-
cfgDeadEnds
7-
cfgConsistency

‎actions/ql/test/library-tests/cfg-environment/test.ql‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,3 @@ query predicate envCfgNodes(Expression expression) {
77
}
88

99
query predicate cfgCycles(Cfg::Node node) { node.getASuccessor+() = node }
10-
11-
query predicate cfgDeadEnds(Cfg::Node node) {
12-
not node instanceof Cfg::ExitNode and
13-
not exists(node.getASuccessor())
14-
}
15-
16-
query predicate cfgConsistency(string query, int results) {
17-
Cfg::Consistency::consistencyOverview(query, results) and
18-
results != 0
19-
}

0 commit comments

Comments
 (0)