Skip to content

SSA: Replace the Guards interface in the SSA data flow integration. #18934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aschackmull merged 3 commits into from
Mar 6, 2025
Merged

SSA: Replace the Guards interface in the SSA data flow integration. #18934

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a027353
Ruby: Remove some DefinitionExt references and deprecate the rest.
aschackmull Feb 27, 2025
c6761db
SSA: Replace the Guards interface in the SSA data flow integration.
aschackmull Mar 5, 2025
5e722ee
Ruby: Push in casts to Definition to delete the then unused Definitio…
aschackmull Mar 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,10 +74,10 @@ CfgNodes::ExprCfgNode getAPostUpdateNodeForArg(Argument arg) {

/** Gets the SSA definition node corresponding to parameter `p`. */
pragma[nomagic]
SsaImpl::DefinitionExt getParameterDef(NamedParameter p) {
Ssa::Definition getParameterDef(NamedParameter p) {
exists(BasicBlock bb, int i |
bb.getNode(i).getAstNode() = p.getDefiningAccess() and
result.definesAt(_, bb, i, _)
result.definesAt(_, bb, i)
)
}

Expand Down Expand Up @@ -388,15 +388,15 @@ module VariableCapture {
Flow::clearsContent(asClosureNode(node), c.getVariable())
}

class CapturedSsaDefinitionExt extends SsaImpl::DefinitionExt {
CapturedSsaDefinitionExt() { this.getSourceVariable() instanceof CapturedVariable }
class CapturedSsaDefinition extends SsaImpl::Definition {
CapturedSsaDefinition() { this.getSourceVariable() instanceof CapturedVariable }
}

// From an assignment or implicit initialization of a captured variable to its flow-insensitive node
private predicate flowInsensitiveWriteStep(
SsaDefinitionNodeImpl node1, CapturedVariableNode node2, CapturedVariable v
) {
exists(CapturedSsaDefinitionExt def |
exists(CapturedSsaDefinition def |
def = node1.getDefinition() and
def.getSourceVariable() = v and
(
Expand All @@ -412,7 +412,7 @@ module VariableCapture {
private predicate flowInsensitiveReadStep(
CapturedVariableNode node1, SsaDefinitionNodeImpl node2, CapturedVariable v
) {
exists(CapturedSsaDefinitionExt def |
exists(CapturedSsaDefinition def |
node1.getVariable() = v and
def = node2.getDefinition() and
def.getSourceVariable() = v and
Expand Down Expand Up @@ -772,7 +772,7 @@ class SsaNode extends NodeImpl, TSsaNode {
class SsaDefinitionNodeImpl extends SsaNode {
override SsaImpl::DataFlowIntegration::SsaDefinitionNode node;

SsaImpl::Definition getDefinition() { result = node.getDefinition() }
Ssa::Definition getDefinition() { result = node.getDefinition() }

override predicate isHidden() {
exists(SsaImpl::Definition def | def = this.getDefinition() |
Expand Down Expand Up @@ -2478,7 +2478,7 @@ module TypeInference {
n = def or
n.asExpr() =
any(CfgNodes::ExprCfgNode read |
read = def.getDefinition().(SsaImpl::DefinitionExt).getARead() and
read = def.getDefinition().getARead() and
not isTypeCheckedRead(read, _) // could in principle be checked against a new type
)
)
Expand Down
8 changes: 4 additions & 4 deletions ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ import Cached
*
* Only intended for internal use.
*/
class DefinitionExt extends Impl::DefinitionExt {
deprecated class DefinitionExt extends Impl::DefinitionExt {
Copy link
Contributor

@hvitved hvitved Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are inside an internal folder, and has the Only intended for internal use comment, it should be fine to simply delete.

Copy link
Contributor Author

@aschackmull aschackmull Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They're actually used in a couple of deprecated predicates that ultimately end up in the public getALastRead member predicate on Definition in the outwards-facing SSA.qll, so it's less clear cut. That's why I left them in for now.

Copy link
Contributor Author

@aschackmull aschackmull Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make it private, though, and possibly push in enough casts from DefinitionExt to Definition that we might be able to eliminate it. Let me check.

Copy link
Contributor Author

@aschackmull aschackmull Mar 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. All the uses of DefinitionExt here were actually in contexts for which we could push a context with a cast to Definition. So I'll be able to delete this. Commit incoming.

VariableReadAccessCfgNode getARead() { result = getARead(this) }

override string toString() { result = this.(Ssa::Definition).toString() }
Expand All @@ -434,7 +434,7 @@ class DefinitionExt extends Impl::DefinitionExt {
*
* Only intended for internal use.
*/
class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
deprecated class PhiReadNode extends DefinitionExt, Impl::PhiReadNode {
override string toString() { result = "SSA phi read(" + this.getSourceVariable() + ")" }

override Location getLocation() { result = Impl::PhiReadNode.super.getLocation() }
Expand All @@ -453,10 +453,10 @@ class NormalParameter extends Parameter {

/** Gets the SSA definition node corresponding to parameter `p`. */
pragma[nomagic]
DefinitionExt getParameterDef(NamedParameter p) {
Definition getParameterDef(NamedParameter p) {
exists(Cfg::BasicBlock bb, int i |
bb.getNode(i).getAstNode() = p.getDefiningAccess() and
result.definesAt(_, bb, i, _)
result.definesAt(_, bb, i)
)
}

Expand Down