Skip to content

Commit 3654205

Browse files
authored
Merge pull request #21991 from github/copilot/change-ast-for-else-branches
Ruby: Add CaseElseBranch AST node to distinguish else-branch from its body
2 parents 72f34c2 + 027f302 commit 3654205

12 files changed

Lines changed: 154 additions & 27 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: breaking
3+
---
4+
* The `else` branch of a `case` expression is no longer represented as a `StmtSequence` directly. Instead, a new `CaseElseBranch` AST node wraps the body (a `StmtSequence`). `CaseExpr.getElseBranch()` now returns a `CaseElseBranch`, and the body of the else branch can be accessed via `CaseElseBranch.getBody()`.

ruby/ql/lib/codeql/ruby/ast/Control.qll

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -377,18 +377,18 @@ class CaseExpr extends ControlExpr instanceof CaseExprImpl {
377377

378378
/**
379379
* Gets the `n`th branch of this case expression, either a `WhenClause`, an
380-
* `InClause`, or a `StmtSequence`.
380+
* `InClause`, or a `CaseElseBranch`.
381381
*/
382382
final AstNode getBranch(int n) { result = super.getBranch(n) }
383383

384384
/**
385385
* Gets a branch of this case expression, either a `WhenClause`, an
386-
* `InClause`, or a `StmtSequence`.
386+
* `InClause`, or a `CaseElseBranch`.
387387
*/
388388
final AstNode getABranch() { result = this.getBranch(_) }
389389

390390
/** Gets the `else` branch of this case expression, if any. */
391-
final StmtSequence getElseBranch() { result = this.getABranch() }
391+
final CaseElseBranch getElseBranch() { result = this.getABranch() }
392392

393393
/**
394394
* Gets the number of branches of this case expression.
@@ -533,6 +533,30 @@ class InClause extends AstNode instanceof InClauseImpl {
533533
}
534534
}
535535

536+
/**
537+
* An `else` branch of a `case` expression.
538+
* ```rb
539+
* case foo
540+
* when 1 then puts "one"
541+
* else puts "other"
542+
* end
543+
* ```
544+
*/
545+
class CaseElseBranch extends AstNode instanceof CaseElseBranchImpl {
546+
final override string getAPrimaryQlClass() { result = "CaseElseBranch" }
547+
548+
/** Gets the body of this else branch. */
549+
final StmtSequence getBody() { result = super.getBody() }
550+
551+
final override string toString() { result = "else ..." }
552+
553+
final override AstNode getAChild(string pred) {
554+
result = AstNode.super.getAChild(pred)
555+
or
556+
pred = "getBody" and result = this.getBody()
557+
}
558+
}
559+
536560
/**
537561
* A one-line pattern match using the `=>` operator. For example:
538562
* ```rb

ruby/ql/lib/codeql/ruby/ast/internal/AST.qll

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ private module Cached {
113113
TBraceBlockSynth(Ast::AstNode parent, int i) { mkSynthChild(BraceBlockKind(), parent, i) } or
114114
TBraceBlockReal(Ruby::Block g) { not g.getParent() instanceof Ruby::Lambda } or
115115
TBreakStmt(Ruby::Break g) or
116+
TCaseElseBranchSynth(Ast::AstNode parent, int i) {
117+
mkSynthChild(CaseElseBranchKind(), parent, i)
118+
} or
116119
TCaseEqExpr(Ruby::Binary g) { g instanceof @ruby_binary_equalequalequal } or
117120
TCaseExpr(Ruby::Case g) or
118121
TCaseMatchReal(Ruby::CaseMatch g) or
@@ -400,14 +403,15 @@ private module Cached {
400403
class TAstNodeSynth =
401404
TAddExprSynth or TAssignExprSynth or TBitwiseAndExprSynth or TBitwiseOrExprSynth or
402405
TBitwiseXorExprSynth or TBraceBlockSynth or TBodyStmtSynth or TBooleanLiteralSynth or
403-
TCaseMatchSynth or TClassVariableAccessSynth or TConstantReadAccessSynth or
404-
TConstantWriteAccessSynth or TDivExprSynth or TElseSynth or TExponentExprSynth or
405-
TGlobalVariableAccessSynth or TIfSynth or TInClauseSynth or TInstanceVariableAccessSynth or
406-
TIntegerLiteralSynth or TLShiftExprSynth or TLocalVariableAccessSynth or
407-
TLogicalAndExprSynth or TLogicalOrExprSynth or TMethodCallSynth or TModuloExprSynth or
408-
TMulExprSynth or TNilLiteralSynth or TRShiftExprSynth or TRangeLiteralSynth or TSelfSynth or
409-
TSimpleParameterSynth or TSplatExprSynth or THashSplatExprSynth or TStmtSequenceSynth or
410-
TSubExprSynth or TPairSynth or TSimpleSymbolLiteralSynth;
406+
TCaseElseBranchSynth or TCaseMatchSynth or TClassVariableAccessSynth or
407+
TConstantReadAccessSynth or TConstantWriteAccessSynth or TDivExprSynth or TElseSynth or
408+
TExponentExprSynth or TGlobalVariableAccessSynth or TIfSynth or TInClauseSynth or
409+
TInstanceVariableAccessSynth or TIntegerLiteralSynth or TLShiftExprSynth or
410+
TLocalVariableAccessSynth or TLogicalAndExprSynth or TLogicalOrExprSynth or
411+
TMethodCallSynth or TModuloExprSynth or TMulExprSynth or TNilLiteralSynth or
412+
TRShiftExprSynth or TRangeLiteralSynth or TSelfSynth or TSimpleParameterSynth or
413+
TSplatExprSynth or THashSplatExprSynth or TStmtSequenceSynth or TSubExprSynth or
414+
TPairSynth or TSimpleSymbolLiteralSynth;
411415

412416
/**
413417
* Gets the underlying TreeSitter entity for a given AST node. This does not
@@ -598,6 +602,8 @@ private module Cached {
598602
or
599603
result = TBraceBlockSynth(parent, i)
600604
or
605+
result = TCaseElseBranchSynth(parent, i)
606+
or
601607
result = TCaseMatchSynth(parent, i)
602608
or
603609
result = TClassVariableAccessSynth(parent, i, _)
@@ -718,6 +724,8 @@ TAstNodeReal fromGenerated(Ruby::AstNode n) { n = toGenerated(result) }
718724

719725
class TCall = TMethodCall or TYieldCall;
720726

727+
class TCaseElseBranch = TCaseElseBranchSynth;
728+
721729
class TCaseMatch = TCaseMatchReal or TCaseMatchSynth;
722730

723731
class TCase = TCaseExpr or TCaseMatch;

ruby/ql/lib/codeql/ruby/ast/internal/Control.qll

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,8 +19,11 @@ class CaseWhenClause extends CaseExprImpl, TCaseExpr {
1919
final override Expr getValue() { toGenerated(result) = g.getValue() }
2020

2121
final override AstNode getBranch(int n) {
22-
toGenerated(result) = g.getChild(n) or
23-
toGenerated(result) = g.getChild(n)
22+
// When branches map directly to WhenClause nodes
23+
toGenerated(result) = g.getChild(n) and not g.getChild(n) instanceof Ruby::Else
24+
or
25+
// The else branch is wrapped in a synthesized CaseElseBranch node
26+
g.getChild(n) instanceof Ruby::Else and result = getSynthChild(this, n)
2427
}
2528
}
2629

@@ -34,7 +37,8 @@ class CaseMatch extends CaseExprImpl, TCaseMatchReal {
3437
final override AstNode getBranch(int n) {
3538
toGenerated(result) = g.getClauses(n)
3639
or
37-
n = count(g.getClauses(_)) and toGenerated(result) = g.getElse()
40+
// The else branch is wrapped in a synthesized CaseElseBranch node
41+
n = count(g.getClauses(_)) and exists(g.getElse()) and result = getSynthChild(this, n)
3842
}
3943
}
4044

@@ -87,3 +91,9 @@ class InClauseSynth extends InClauseImpl, TInClauseSynth {
8791

8892
final override predicate hasUnlessCondition() { none() }
8993
}
94+
95+
class CaseElseBranchImpl extends AstNode, TCaseElseBranch {
96+
CaseElseBranchImpl() { this = TCaseElseBranchSynth(_, _) }
97+
98+
final StmtSequence getBody() { synthChild(this, 0, result) }
99+
}

ruby/ql/lib/codeql/ruby/ast/internal/Synthesis.qll

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ newtype TSynthKind =
2222
BodyStmtKind() or
2323
BooleanLiteralKind(boolean value) { value = true or value = false } or
2424
BraceBlockKind() or
25+
CaseElseBranchKind() or
2526
CaseMatchKind() or
2627
ClassVariableAccessKind(ClassVariable v) or
2728
DefinedExprKind() or
@@ -80,6 +81,8 @@ class SynthKind extends TSynthKind {
8081
or
8182
this = BraceBlockKind() and result = "BraceBlockKind"
8283
or
84+
this = CaseElseBranchKind() and result = "CaseElseBranchKind"
85+
or
8386
this = CaseMatchKind() and result = "CaseMatchKind"
8487
or
8588
this = ClassVariableAccessKind(_) and result = "ClassVariableAccessKind"
@@ -1840,7 +1843,7 @@ private module TestPatternDesugar {
18401843
or
18411844
child = SynthChild(InClauseKind()) and i = 1
18421845
or
1843-
child = SynthChild(ElseKind()) and i = 2
1846+
child = SynthChild(CaseElseBranchKind()) and i = 2
18441847
)
18451848
or
18461849
parent = TInClauseSynth(case, 1) and
@@ -1851,7 +1854,11 @@ private module TestPatternDesugar {
18511854
child = SynthChild(BooleanLiteralKind(true)) and i = 1
18521855
)
18531856
or
1854-
parent = TElseSynth(case, 2) and
1857+
parent = TCaseElseBranchSynth(case, 2) and
1858+
child = SynthChild(ElseKind()) and
1859+
i = 0
1860+
or
1861+
parent = TElseSynth(TCaseElseBranchSynth(case, 2), 0) and
18551862
child = SynthChild(BooleanLiteralKind(false)) and
18561863
i = 0
18571864
)
@@ -1994,3 +2001,61 @@ private module CallableBodySynthesis {
19942001
}
19952002
}
19962003
}
2004+
2005+
private module CaseElseBranchSynthesis {
2006+
pragma[nomagic]
2007+
private predicate caseElseBranchSynthesis(AstNode parent, int i, Child child) {
2008+
// Wrap the else branch of a real `case`/`when` expression
2009+
exists(Ruby::Case g, Ruby::Else elseNode, int elseIndex |
2010+
elseNode = g.getChild(elseIndex) and
2011+
(
2012+
// Create the CaseElseBranch wrapper node at the else index
2013+
parent = TCaseExpr(g) and
2014+
child = SynthChild(CaseElseBranchKind()) and
2015+
i = elseIndex
2016+
or
2017+
// The body of the CaseElseBranch is the Else node
2018+
parent = TCaseElseBranchSynth(TCaseExpr(g), elseIndex) and
2019+
child = RealChildRef(TElseReal(elseNode)) and
2020+
i = 0
2021+
)
2022+
)
2023+
or
2024+
// Wrap the else branch of a real `case`/`in` expression
2025+
exists(Ruby::CaseMatch g, Ruby::Else elseNode, int elseIndex |
2026+
elseNode = g.getElse() and
2027+
elseIndex = count(g.getClauses(_)) and
2028+
(
2029+
// Create the CaseElseBranch wrapper node at the else index
2030+
parent = TCaseMatchReal(g) and
2031+
child = SynthChild(CaseElseBranchKind()) and
2032+
i = elseIndex
2033+
or
2034+
// The body of the CaseElseBranch is the Else node
2035+
parent = TCaseElseBranchSynth(TCaseMatchReal(g), elseIndex) and
2036+
child = RealChildRef(TElseReal(elseNode)) and
2037+
i = 0
2038+
)
2039+
)
2040+
}
2041+
2042+
private class CaseElseBranchSynthesisImpl extends Synthesis {
2043+
final override predicate child(AstNode parent, int i, Child child) {
2044+
caseElseBranchSynthesis(parent, i, child)
2045+
}
2046+
2047+
final override predicate location(AstNode n, Location l) {
2048+
// Give the CaseElseBranch the location of the underlying Else node
2049+
exists(Ruby::Case g, int elseIndex |
2050+
n = TCaseElseBranchSynth(TCaseExpr(g), elseIndex) and
2051+
l = g.getChild(elseIndex).getLocation()
2052+
)
2053+
or
2054+
exists(Ruby::CaseMatch g, int elseIndex |
2055+
elseIndex = count(g.getClauses(_)) and
2056+
n = TCaseElseBranchSynth(TCaseMatchReal(g), elseIndex) and
2057+
l = g.getElse().getLocation()
2058+
)
2059+
}
2060+
}
2061+
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -498,6 +498,16 @@ module Trees {
498498
}
499499
}
500500

501+
private class CaseElseBranchTree extends ControlFlowTree instanceof CaseElseBranch {
502+
final override predicate propagatesAbnormal(AstNode child) { child = super.getBody() }
503+
504+
final override predicate first(AstNode first) { first(super.getBody(), first) }
505+
506+
final override predicate last(AstNode last, Completion c) { last(super.getBody(), last, c) }
507+
508+
final override predicate succ(AstNode pred, AstNode succ, Completion c) { none() }
509+
}
510+
501511
private class PatternVariableAccessTree extends LocalVariableAccessTree instanceof LocalVariableWriteAccess,
502512
CasePattern
503513
{

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowPrivate.qll

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -68,9 +68,9 @@ private CfgNodes::ExprCfgNode getALastEvalNode(CfgNodes::ExprCfgNode n) {
6868
result = branch.(CfgNodes::ExprNodes::InClauseCfgNode).getBody()
6969
or
7070
result = branch.(CfgNodes::ExprNodes::WhenClauseCfgNode).getBody()
71-
or
72-
result = branch
7371
)
72+
or
73+
result.getAstNode() = n.(CfgNodes::ExprNodes::CaseExprCfgNode).getExpr().getElseBranch().getBody()
7474
}
7575

7676
/**

ruby/ql/test/library-tests/ast/Ast.expected

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -815,8 +815,9 @@ control/cases.rb:
815815
# 11| getPattern: [LocalVariableAccess] d
816816
# 11| getBody: [StmtSequence] then ...
817817
# 12| getStmt: [IntegerLiteral] 200
818-
# 13| getBranch/getElseBranch: [StmtSequence] else ...
819-
# 14| getStmt: [IntegerLiteral] 300
818+
# 13| getBranch/getElseBranch: [CaseElseBranch] else ...
819+
# 13| getBody: [StmtSequence] else ...
820+
# 14| getStmt: [IntegerLiteral] 300
820821
# 18| getStmt: [CaseExpr] case ...
821822
# 19| getBranch: [WhenClause] when ...
822823
# 19| getPattern: [GTExpr] ... > ...
@@ -843,8 +844,9 @@ control/cases.rb:
843844
# 27| getPattern: [IntegerLiteral] 5
844845
# 27| getBody: [StmtSequence] then ...
845846
# 27| getStmt: [BooleanLiteral] true
846-
# 28| getBranch/getElseBranch: [StmtSequence] else ...
847-
# 28| getStmt: [BooleanLiteral] false
847+
# 28| getBranch/getElseBranch: [CaseElseBranch] else ...
848+
# 28| getBody: [StmtSequence] else ...
849+
# 28| getStmt: [BooleanLiteral] false
848850
# 31| getStmt: [CaseExpr] case ...
849851
# 31| getValue: [MethodCall] call to expr
850852
# 31| getReceiver: [SelfVariableAccess] self
@@ -862,8 +864,9 @@ control/cases.rb:
862864
# 34| getAnOperand/getArgument/getGreaterOperand/getRightOperand: [IntegerLiteral] 0
863865
# 34| getBody: [StmtSequence] then ...
864866
# 35| getStmt: [BooleanLiteral] true
865-
# 36| getBranch/getElseBranch: [StmtSequence] else ...
866-
# 36| getStmt: [BooleanLiteral] false
867+
# 36| getBranch/getElseBranch: [CaseElseBranch] else ...
868+
# 36| getBody: [StmtSequence] else ...
869+
# 36| getStmt: [BooleanLiteral] false
867870
# 39| getStmt: [CaseExpr] case ...
868871
# 39| getValue: [MethodCall] call to expr
869872
# 39| getReceiver: [SelfVariableAccess] self

ruby/ql/test/library-tests/ast/AstDesugar.expected

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -389,8 +389,9 @@ control/cases.rb:
389389
# 160| getPrefixElement: [IntegerLiteral] 1
390390
# 160| getPrefixElement: [IntegerLiteral] 2
391391
# 160| getBody: [BooleanLiteral] true
392-
# 160| getBranch/getElseBranch: [StmtSequence] else ...
393-
# 160| getStmt: [BooleanLiteral] false
392+
# 160| getBranch/getElseBranch: [CaseElseBranch] else ...
393+
# 160| getBody: [StmtSequence] else ...
394+
# 160| getStmt: [BooleanLiteral] false
394395
# 162| [MatchPattern] ... => ...
395396
# 162| getDesugared: [CaseExpr] case ...
396397
# 162| getValue: [MethodCall] call to expr

ruby/ql/test/library-tests/ast/control/CaseExpr.ql

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ query predicate caseValues(CaseExpr c, Expr value) { value = c.getValue() }
44

55
query predicate caseNoValues(CaseExpr c) { not exists(c.getValue()) }
66

7-
query predicate caseElseBranches(CaseExpr c, StmtSequence elseBranch) {
7+
query predicate caseElseBranches(CaseExpr c, CaseElseBranch elseBranch) {
88
elseBranch = c.getElseBranch()
99
}
1010

0 commit comments

Comments
 (0)