Merge pull request github#7141 from hvitved/ruby/synthesis-realnode-recursion

Ruby: Eliminate unnecessary recursion through `RealNode`

Author: hvitved

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

@@ -167,7 +167,7 @@ private module Cached {
     TLTExpr(Ruby::Binary g) { g instanceof @ruby_binary_langle } or
     TLambda(Ruby::Lambda g) or
     TLeftAssignmentList(Ruby::LeftAssignmentList g) or
-    TLocalVariableAccessReal(Ruby::Identifier g, AST::LocalVariable v) {
+    TLocalVariableAccessReal(Ruby::Identifier g, TLocalVariableReal v) {
       LocalVariableAccess::range(g, v)
     } or
     TLocalVariableAccessSynth(AST::AstNode parent, int i, AST::LocalVariable v) {
@@ -284,13 +284,55 @@ private module Cached {
     TWhileModifierExpr(Ruby::WhileModifier g) or
     TYieldCall(Ruby::Yield g)
 
+  class TAstNodeReal =
+    TAddExprReal or TAliasStmt or TArgumentList or TAssignAddExpr or TAssignBitwiseAndExpr or
+        TAssignBitwiseOrExpr or TAssignBitwiseXorExpr or TAssignDivExpr or TAssignExponentExpr or
+        TAssignExprReal or TAssignLShiftExpr or TAssignLogicalAndExpr or TAssignLogicalOrExpr or
+        TAssignModuloExpr or TAssignMulExpr or TAssignRShiftExpr or TAssignSubExpr or
+        TBareStringLiteral or TBareSymbolLiteral or TBeginBlock or TBeginExpr or
+        TBitwiseAndExprReal or TBitwiseOrExprReal or TBitwiseXorExprReal or TBlockArgument or
+        TBlockParameter or TBraceBlock or TBreakStmt or TCaseEqExpr or TCaseExpr or
+        TCharacterLiteral or TClassDeclaration or TClassVariableAccessReal or TComplementExpr or
+        TComplexLiteral or TDefinedExpr or TDelimitedSymbolLiteral or TDestructuredLeftAssignment or
+        TDivExprReal or TDo or TDoBlock or TElementReference or TElse or TElsif or TEmptyStmt or
+        TEndBlock or TEnsure or TEqExpr or TExponentExprReal or TFalseLiteral or TFloatLiteral or
+        TForExpr or TForIn or TForwardParameter or TForwardArgument or TGEExpr or TGTExpr or
+        TGlobalVariableAccessReal or THashKeySymbolLiteral or THashLiteral or THashSplatExpr or
+        THashSplatParameter or THereDoc or TIdentifierMethodCall or TIf or TIfModifierExpr or
+        TInstanceVariableAccessReal or TIntegerLiteralReal or TKeywordParameter or TLEExpr or
+        TLShiftExprReal or TLTExpr or TLambda or TLeftAssignmentList or TLocalVariableAccessReal or
+        TLogicalAndExprReal or TLogicalOrExprReal or TMethod or TModuleDeclaration or
+        TModuloExprReal or TMulExprReal or TNEExpr or TNextStmt or TNilLiteral or
+        TNoRegExpMatchExpr or TNotExpr or TOptionalParameter or TPair or TParenthesizedExpr or
+        TRShiftExprReal or TRangeLiteralReal or TRationalLiteral or TRedoStmt or TRegExpLiteral or
+        TRegExpMatchExpr or TRegularArrayLiteral or TRegularMethodCall or TRegularStringLiteral or
+        TRegularSuperCall or TRescueClause or TRescueModifierExpr or TRetryStmt or TReturnStmt or
+        TScopeResolutionConstantAccess or TScopeResolutionMethodCall or TSelfReal or
+        TSimpleParameter or TSimpleSymbolLiteral or TSingletonClass or TSingletonMethod or
+        TSpaceshipExpr or TSplatExprReal or TSplatParameter or TStringArrayLiteral or
+        TStringConcatenation or TStringEscapeSequenceComponent or TStringInterpolationComponent or
+        TStringTextComponent or TSubExprReal or TSubshellLiteral or TSymbolArrayLiteral or
+        TTernaryIfExpr or TThen or TTokenConstantAccess or TTokenMethodName or TTokenSuperCall or
+        TToplevel or TTrueLiteral or TTuplePatternParameter or TUnaryMinusExpr or TUnaryPlusExpr or
+        TUndefStmt or TUnlessExpr or TUnlessModifierExpr or TUntilExpr or TUntilModifierExpr or
+        TWhenExpr or TWhileExpr or TWhileModifierExpr or TYieldCall;
+
+  class TAstNodeSynth =
+    TAddExprSynth or TAssignExprSynth or TBitwiseAndExprSynth or TBitwiseOrExprSynth or
+        TBitwiseXorExprSynth or TClassVariableAccessSynth or TConstantReadAccessSynth or
+        TDivExprSynth or TExponentExprSynth or TGlobalVariableAccessSynth or
+        TInstanceVariableAccessSynth or TIntegerLiteralSynth or TLShiftExprSynth or
+        TLocalVariableAccessSynth or TLogicalAndExprSynth or TLogicalOrExprSynth or
+        TMethodCallSynth or TModuloExprSynth or TMulExprSynth or TRShiftExprSynth or
+        TRangeLiteralSynth or TSelfSynth or TSplatExprSynth or TStmtSequenceSynth or TSubExprSynth;
+
   /**
    * Gets the underlying TreeSitter entity for a given AST node. This does not
    * include synthesized AST nodes, because they are not the primary AST node
    * for any given generated node.
    */
   cached
-  Ruby::AstNode toGenerated(AST::AstNode n) {
+  Ruby::AstNode toGenerated(TAstNodeReal n) {
     n = TAddExprReal(result) or
     n = TAliasStmt(result) or
     n = TArgumentList(result) or
@@ -495,7 +537,9 @@ private module Cached {
   predicate synthChild(AST::AstNode parent, int i, AST::AstNode child) {
     child = getSynthChild(parent, i)
     or
-    any(Synthesis s).child(parent, i, RealChild(child))
+    any(Synthesis s).child(parent, i, RealChildRef(child))
+    or
+    any(Synthesis s).child(parent, i, SynthChildRef(child))
   }
 
   /**
@@ -527,7 +571,7 @@ private module Cached {
 
 import Cached
 
-TAstNode fromGenerated(Ruby::AstNode n) { n = toGenerated(result) }
+TAstNodeReal fromGenerated(Ruby::AstNode n) { n = toGenerated(result) }
 
 class TCall = TMethodCall or TYieldCall;
 

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

@@ -46,7 +46,25 @@ newtype SynthKind =
  */
 newtype Child =
   SynthChild(SynthKind k) or
-  RealChild(AstNode n)
+  RealChildRef(TAstNodeReal n) or
+  SynthChildRef(TAstNodeSynth n)
+
+/**
+ * The purpose of this inlined predicate is to split up child references into
+ * those that are from real AST nodes (for which there will be no recursion
+ * through `RealChildRef`), and those that are synthesized recursively
+ * (for which there will be recursion through `SynthChildRef`).
+ *
+ * This performs much better than having a combined `ChildRef` that includes
+ * both real and synthesized AST nodes, since the recursion happening in
+ * `Synthesis::child/3` is non-linear.
+ */
+pragma[inline]
+private Child childRef(TAstNode n) {
+  result = RealChildRef(n)
+  or
+  result = SynthChildRef(n)
+}
 
 private newtype TSynthesis = MkSynthesis()
 
@@ -125,7 +143,7 @@ private predicate assign(
     child = SynthChild(LocalVariableAccessSynthKind(v))
     or
     i = 1 and
-    child = RealChild(value)
+    child = childRef(value)
   )
 }
 
@@ -218,10 +236,10 @@ private module SetterDesugar {
         exists(AstNode call | call = TMethodCallSynth(seq, 0, _, _, _) |
           parent = call and
           i = 0 and
-          child = RealChild(sae.getReceiver())
+          child = childRef(sae.getReceiver())
           or
           parent = call and
-          child = RealChild(sae.getArgument(i - 1))
+          child = childRef(sae.getArgument(i - 1))
           or
           exists(int valueIndex | valueIndex = sae.getNumberOfArguments() + 1 |
             parent = call and
@@ -234,7 +252,7 @@ private module SetterDesugar {
               child = SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(sae, 0)))
               or
               i = 1 and
-              child = RealChild(sae.getRightOperand())
+              child = childRef(sae.getRightOperand())
             )
           )
         )
@@ -357,7 +375,7 @@ private module AssignOperationDesugar {
       exists(AstNode assign | assign = TAssignExprSynth(vao, -1) |
         parent = assign and
         i = 0 and
-        child = RealChild(vao.getLeftOperand())
+        child = childRef(vao.getLeftOperand())
         or
         parent = assign and
         i = 1 and
@@ -369,7 +387,7 @@ private module AssignOperationDesugar {
           child = SynthChild(vao.getVariableAccessKind())
           or
           i = 1 and
-          child = RealChild(vao.getRightOperand())
+          child = childRef(vao.getRightOperand())
         )
       )
     )
@@ -477,7 +495,7 @@ private module AssignOperationDesugar {
               or
               parent = op and
               i = 1 and
-              child = RealChild(sao.getRightOperand())
+              child = childRef(sao.getRightOperand())
             )
           )
           or
@@ -648,7 +666,7 @@ private module CompoundAssignDesugar {
           or
           parent = TSplatExprSynth(assign, 1) and
           i = 0 and
-          child = RealChild(tae.getRightOperand())
+          child = childRef(tae.getRightOperand())
         )
         or
         exists(Pattern p, int j, int restIndex |
@@ -662,7 +680,7 @@ private module CompoundAssignDesugar {
           exists(AstNode assign | assign = TAssignExprSynth(seq, j + 1) |
             parent = assign and
             i = 0 and
-            child = RealChild(p)
+            child = childRef(p)
             or
             parent = assign and
             i = 1 and
@@ -767,7 +785,7 @@ private module ArrayLiteralDesugar {
         child = SynthChild(ConstantReadAccessKind("::Array"))
         or
         parent = mc and
-        child = RealChild(al.getElement(i - 1))
+        child = childRef(al.getElement(i - 1))
       )
     )
   }

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

@@ -495,7 +495,7 @@ abstract class VariableAccessImpl extends Expr, TVariableAccess {
 }
 
 module LocalVariableAccess {
-  predicate range(Ruby::Identifier id, LocalVariable v) {
+  predicate range(Ruby::Identifier id, TLocalVariableReal v) {
     access(id, v) and
     (
       explicitWriteAccess(id, _)

ruby/ql/test/library-tests/controlflow/graph/Cfg.expected

@@ -41,8 +41,8 @@ break_ensure.rb:
 #-----|  -> do ...
 
 #    3| ... > ...
-#-----| raise -> for ... in ...
 #-----| true -> break
+#-----| raise -> for ... in ...
 #-----| false -> if ...
 
 #    3| element
@@ -580,12 +580,12 @@ cfg.html.erb:
 #   12| self
 #-----|  -> call to a
 
-#   12| Pair
-#-----|  -> call to link_to
-
 #   12| :id
 #-----|  -> "a"
 
+#   12| Pair
+#-----|  -> call to link_to
+
 #   12| "a"
 #-----|  -> Pair
 
@@ -813,12 +813,12 @@ cfg.rb:
 #   23| 1
 #-----|  -> ... + ...
 
-#   25| 2
-#-----|  -> { ... }
-
 #   25| call to times
 #-----|  -> self
 
+#   25| 2
+#-----|  -> { ... }
+
 #   25| enter { ... }
 #-----|  -> x
 
@@ -1493,12 +1493,12 @@ cfg.rb:
 #   97| "d"
 #-----|  -> Pair
 
-#   97| Pair
-#-----|  -> {...}
-
 #   97| :e
 #-----|  -> "f"
 
+#   97| Pair
+#-----|  -> {...}
+
 #   97| "f"
 #-----|  -> Pair
 
@@ -1619,12 +1619,12 @@ cfg.rb:
 #  110| type
 #-----|  -> #{...}
 
-#  113| ... if ...
-#-----|  -> C
-
 #  113| call to puts
 #-----|  -> ... if ...
 
+#  113| ... if ...
+#-----|  -> C
+
 #  113| self
 #-----|  -> "hi"
 
@@ -1826,16 +1826,16 @@ cfg.rb:
 #  134| EmptyModule
 #-----|  -> ... rescue ...
 
-#  136| ... rescue ...
-#-----|  -> 1
-
 #  136| ... / ...
 #-----| raise -> self
 #-----|  -> __synth__0
 
 #  136| 1
 #-----|  -> 0
 
+#  136| ... rescue ...
+#-----|  -> 1
+
 #  136| 0
 #-----|  -> ... / ...
 
@@ -2708,12 +2708,12 @@ desugar.rb:
 #   18| __synth__2
 #-----|  -> __synth__3
 
-#   18| ... + ...
-#-----|  -> ... = ...
-
 #   18| call to baz
 #-----|  -> 3
 
+#   18| ... + ...
+#-----|  -> ... = ...
+
 #   18| x
 #-----|  -> call to baz
 
@@ -5076,12 +5076,12 @@ raise.rb:
 #  155| elem
 #-----|  -> element
 
-#  155| ... if ...
-#-----|  -> exit { ... } (normal)
-
 #  155| call to raise
 #-----| raise -> exit { ... } (abnormal)
 
+#  155| ... if ...
+#-----|  -> exit { ... } (normal)
+
 #  155| self
 #-----|  -> ""