Merge pull request github#12612 from hvitved/ruby/print-ast-desugar-reorder

Ruby: Order synthetic children in PrintAST based on their index instead of location

Author: hvitved

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

@@ -43,11 +43,16 @@ class MethodCallSynth extends MethodCallImpl, TMethodCallSynth {
 
   final override AstNode getReceiverImpl() { synthChild(this, 0, result) }
 
-  final override AstNode getArgumentImpl(int n) { synthChild(this, n + 1, result) and n >= 0 }
+  final override AstNode getArgumentImpl(int n) {
+    synthChild(this, n + 1, result) and
+    n in [0 .. this.getNumberOfArgumentsImpl() - 1]
+  }
 
   final override int getNumberOfArgumentsImpl() { this = TMethodCallSynth(_, _, _, _, result) }
 
-  final override Block getBlockImpl() { synthChild(this, -2, result) }
+  final override Block getBlockImpl() {
+    synthChild(this, this.getNumberOfArgumentsImpl() + 1, result)
+  }
 }
 
 class IdentifierMethodCall extends MethodCallImpl, TIdentifierMethodCall {

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

@@ -818,7 +818,7 @@ private module AssignOperationDesugar {
             i in [0 .. sao.getNumberOfArguments()]
             or
             parent = setter and
-            i = opAssignIndex + 1 and
+            i = opAssignIndex and
             child =
               SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(sao, opAssignIndex)))
           )
@@ -975,7 +975,7 @@ private module DestructuredAssignDesugar {
 
   pragma[nomagic]
   private predicate destructuredAssignSynthesis(AstNode parent, int i, Child child) {
-    exists(DestructuredAssignExpr tae |
+    exists(DestructuredAssignExpr tae, int total | total = tae.getNumberOfElements() |
       parent = tae and
       i = -1 and
       child = SynthChild(StmtSequenceKind())
@@ -998,15 +998,13 @@ private module DestructuredAssignDesugar {
         )
         or
         parent = seq and
-        i = tae.getNumberOfElements() and
+        i = total and
         child = SynthChild(AssignExprKind())
         or
-        exists(AstNode assign | assign = TAssignExprSynth(seq, tae.getNumberOfElements()) |
+        exists(AstNode assign | assign = TAssignExprSynth(seq, total) |
           parent = assign and
           i = 0 and
-          child =
-            SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(tae,
-                  tae.getNumberOfElements())))
+          child = SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(tae, total)))
           or
           parent = assign and
           i = 1 and
@@ -1022,12 +1020,10 @@ private module DestructuredAssignDesugar {
           restIndex = tae.getRestIndexOrNumberOfElements()
         |
           parent = seq and
-          i = j + 1 + tae.getNumberOfElements() and
+          i = j + 1 + total and
           child = SynthChild(AssignExprKind())
           or
-          exists(AstNode assign |
-            assign = TAssignExprSynth(seq, j + 1 + tae.getNumberOfElements())
-          |
+          exists(AstNode assign | assign = TAssignExprSynth(seq, j + 1 + total) |
             exists(LhsWithReceiver mc | mc = elem |
               parent = assign and
               i = 0 and
@@ -1063,9 +1059,7 @@ private module DestructuredAssignDesugar {
             or
             parent = TMethodCallSynth(assign, 1, _, _, _) and
             i = 0 and
-            child =
-              SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(tae,
-                    tae.getNumberOfElements())))
+            child = SynthChild(LocalVariableAccessSynthKind(TLocalVariableSynth(tae, total)))
             or
             j < restIndex and
             parent = TMethodCallSynth(assign, 1, _, _, _) and
@@ -1086,14 +1080,14 @@ private module DestructuredAssignDesugar {
                 child = SynthChild(IntegerLiteralKind(j))
                 or
                 i = 1 and
-                child = SynthChild(IntegerLiteralKind(restIndex - tae.getNumberOfElements()))
+                child = SynthChild(IntegerLiteralKind(restIndex - total))
               )
             )
             or
             j > restIndex and
             parent = TMethodCallSynth(assign, 1, _, _, _) and
             i = 1 and
-            child = SynthChild(IntegerLiteralKind(j - tae.getNumberOfElements()))
+            child = SynthChild(IntegerLiteralKind(j - total))
           )
         )
       )
@@ -1284,10 +1278,10 @@ private module ForLoopDesugar {
         child = childRef(for.getValue()) // value is the Enumerable
         or
         parent = eachCall and
-        i = -2 and
+        i = 1 and
         child = SynthChild(BraceBlockKind())
         or
-        exists(Block block | block = TBraceBlockSynth(eachCall, -2) |
+        exists(Block block | block = TBraceBlockSynth(eachCall, 1) |
           // block params
           parent = block and
           i = 0 and
@@ -1534,14 +1528,13 @@ private module SafeNavigationCallDesugar {
             i = 1
           )
           or
-          parent = TMethodCallSynth(ifExpr, 2, _, _, _) and
-          (
+          exists(int arity | parent = TMethodCallSynth(ifExpr, 2, _, _, arity) |
             i = 0 and
             child = SynthChild(local)
             or
             child = childRef(call.getArgumentImpl(i - 1))
             or
-            child = childRef(call.getBlockImpl()) and i = -2
+            child = childRef(call.getBlockImpl()) and i = arity + 1
           )
         )
       )

ruby/ql/lib/codeql/ruby/printAst.qll

@@ -36,8 +36,6 @@ private predicate shouldPrintAstEdge(AstNode parent, string edgeName, AstNode ch
   any(PrintAstConfiguration config).shouldPrintAstEdge(parent, edgeName, child)
 }
 
-private int nonSynthIndex() { result = min([-1, any(int i | exists(getSynthChild(_, i)))]) - 1 }
-
 newtype TPrintNode =
   TPrintRegularAstNode(AstNode n) { shouldPrintNode(n) } or
   TPrintRegExpNode(RE::RegExpTerm term) {
@@ -115,10 +113,23 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode {
     )
   }
 
+  private predicate parentIsSynthesized() {
+    exists(AstNode parent |
+      shouldPrintAstEdge(parent, _, astNode) and
+      parent.isSynthesized()
+    )
+  }
+
   private int getSynthAstNodeIndex() {
-    not astNode.isSynthesized() and result = nonSynthIndex()
+    this.parentIsSynthesized() and
+    exists(AstNode parent |
+      shouldPrintAstEdge(parent, _, astNode) and
+      parent.isSynthesized() and
+      synthChild(parent, result, astNode)
+    )
     or
-    astNode = getSynthChild(astNode.getParent(), result)
+    not this.parentIsSynthesized() and
+    result = 0
   }
 
   override int getOrder() {
@@ -129,8 +140,8 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode {
       |
         p
         order by
-          f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), l.getStartColumn(),
-          l.getEndLine(), l.getEndColumn(), p.getSynthAstNodeIndex()
+          f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), p.getSynthAstNodeIndex(),
+          l.getStartColumn(), l.getEndLine(), l.getEndColumn()
       )
   }