From 2511f521616ce428a9e5ddcab55467caf50a0bc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Thu, 1 May 2025 11:26:42 -0400 Subject: [PATCH 1/5] Ruby printAst: fix order for synth children of real parents Real parents can have synthesized children, so always assigning index 0 leads to nondeterminism in graph output. --- ruby/ql/lib/codeql/ruby/printAst.qll | 20 +++++++++++--------- ruby/ql/test/library-tests/ast/Ast.expected | 16 ++++++++-------- 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/printAst.qll b/ruby/ql/lib/codeql/ruby/printAst.qll index c15b717610ab..e87b9bc43ece 100644 --- a/ruby/ql/lib/codeql/ruby/printAst.qll +++ b/ruby/ql/lib/codeql/ruby/printAst.qll @@ -121,15 +121,17 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode { } private int getSynthAstNodeIndex() { - this.parentIsSynthesized() and - exists(AstNode parent | - shouldPrintAstEdge(parent, _, astNode) and - parent.isSynthesized() and - synthChild(parent, result, astNode) - ) - or - not this.parentIsSynthesized() and - result = 0 + if + exists(AstNode parent | + shouldPrintAstEdge(parent, _, astNode) and + synthChild(parent, _, astNode) + ) + then + exists(AstNode parent | + shouldPrintAstEdge(parent, _, astNode) and + synthChild(parent, result, astNode) + ) + else result = 0 } override int getOrder() { diff --git a/ruby/ql/test/library-tests/ast/Ast.expected b/ruby/ql/test/library-tests/ast/Ast.expected index 294edfdab1e6..5d8cefb01ab2 100644 --- a/ruby/ql/test/library-tests/ast/Ast.expected +++ b/ruby/ql/test/library-tests/ast/Ast.expected @@ -915,8 +915,8 @@ control/cases.rb: # 56| getValue: [IntegerLiteral] 5 # 56| getKey: [SymbolLiteral] :b # 56| getComponent: [StringTextComponent] b -# 56| getValue: [LocalVariableAccess] b # 56| getRestVariableAccess: [LocalVariableAccess] map +# 56| getValue: [LocalVariableAccess] b # 57| getBranch: [InClause] in ... then ... # 57| getPattern: [HashPattern] { ..., ** } # 57| getKey: [SymbolLiteral] :a @@ -1006,8 +1006,8 @@ control/cases.rb: # 75| getValue: [IntegerLiteral] 5 # 75| getKey: [SymbolLiteral] :b # 75| getComponent: [StringTextComponent] b -# 75| getValue: [LocalVariableAccess] b # 75| getRestVariableAccess: [LocalVariableAccess] map +# 75| getValue: [LocalVariableAccess] b # 76| getBranch: [InClause] in ... then ... # 76| getPattern: [HashPattern] { ..., ** } # 76| getKey: [SymbolLiteral] :a @@ -1209,8 +1209,8 @@ control/cases.rb: # 141| getValue: [IntegerLiteral] 1 # 141| getKey: [SymbolLiteral] :a # 141| getComponent: [StringTextComponent] a -# 141| getValue: [LocalVariableAccess] a # 141| getRestVariableAccess: [LocalVariableAccess] rest +# 141| getValue: [LocalVariableAccess] a # 142| getBranch: [InClause] in ... then ... # 142| getPattern: [HashPattern] { ..., ** } # 142| getClass: [ConstantReadAccess] Foo @@ -3185,14 +3185,14 @@ params/params.rb: # 106| getParameter: [HashSplatParameter] **kwargs # 106| getDefiningAccess: [LocalVariableAccess] kwargs # 107| getStmt: [SuperCall] super call to m -# 107| getArgument: [HashSplatExpr] ** ... -# 107| getAnOperand/getOperand/getReceiver: [LocalVariableAccess] kwargs +# 107| getArgument: [LocalVariableAccess] y +# 107| getArgument: [SplatExpr] * ... +# 107| getAnOperand/getOperand/getReceiver: [LocalVariableAccess] rest # 107| getArgument: [Pair] Pair # 107| getKey: [SymbolLiteral] k # 107| getValue: [LocalVariableAccess] k -# 107| getArgument: [SplatExpr] * ... -# 107| getAnOperand/getOperand/getReceiver: [LocalVariableAccess] rest -# 107| getArgument: [LocalVariableAccess] y +# 107| getArgument: [HashSplatExpr] ** ... +# 107| getAnOperand/getOperand/getReceiver: [LocalVariableAccess] kwargs # 111| getStmt: [MethodCall] call to m # 111| getReceiver: [MethodCall] call to new # 111| getReceiver: [ConstantReadAccess] Sub From b95092ef1c8179386947202b6920e33de7796a55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Fri, 2 May 2025 12:33:21 -0400 Subject: [PATCH 2/5] Ruby printAst: order by start line and column before synth index This counteracts the movement of synth children away from the node from which they take their location, following the decision to take the index of synth children of real parents into account. --- ruby/ql/lib/codeql/ruby/printAst.qll | 4 ++-- ruby/ql/test/library-tests/ast/Ast.expected | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/printAst.qll b/ruby/ql/lib/codeql/ruby/printAst.qll index e87b9bc43ece..707fa20effde 100644 --- a/ruby/ql/lib/codeql/ruby/printAst.qll +++ b/ruby/ql/lib/codeql/ruby/printAst.qll @@ -142,8 +142,8 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode { | p order by - f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), p.getSynthAstNodeIndex(), - l.getStartColumn(), l.getEndLine(), l.getEndColumn() + f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), l.getStartColumn(), + p.getSynthAstNodeIndex(), l.getEndLine(), l.getEndColumn() ) } diff --git a/ruby/ql/test/library-tests/ast/Ast.expected b/ruby/ql/test/library-tests/ast/Ast.expected index 5d8cefb01ab2..6263cb8919b5 100644 --- a/ruby/ql/test/library-tests/ast/Ast.expected +++ b/ruby/ql/test/library-tests/ast/Ast.expected @@ -915,8 +915,8 @@ control/cases.rb: # 56| getValue: [IntegerLiteral] 5 # 56| getKey: [SymbolLiteral] :b # 56| getComponent: [StringTextComponent] b -# 56| getRestVariableAccess: [LocalVariableAccess] map # 56| getValue: [LocalVariableAccess] b +# 56| getRestVariableAccess: [LocalVariableAccess] map # 57| getBranch: [InClause] in ... then ... # 57| getPattern: [HashPattern] { ..., ** } # 57| getKey: [SymbolLiteral] :a @@ -1006,8 +1006,8 @@ control/cases.rb: # 75| getValue: [IntegerLiteral] 5 # 75| getKey: [SymbolLiteral] :b # 75| getComponent: [StringTextComponent] b -# 75| getRestVariableAccess: [LocalVariableAccess] map # 75| getValue: [LocalVariableAccess] b +# 75| getRestVariableAccess: [LocalVariableAccess] map # 76| getBranch: [InClause] in ... then ... # 76| getPattern: [HashPattern] { ..., ** } # 76| getKey: [SymbolLiteral] :a @@ -1209,8 +1209,8 @@ control/cases.rb: # 141| getValue: [IntegerLiteral] 1 # 141| getKey: [SymbolLiteral] :a # 141| getComponent: [StringTextComponent] a -# 141| getRestVariableAccess: [LocalVariableAccess] rest # 141| getValue: [LocalVariableAccess] a +# 141| getRestVariableAccess: [LocalVariableAccess] rest # 142| getBranch: [InClause] in ... then ... # 142| getPattern: [HashPattern] { ..., ** } # 142| getClass: [ConstantReadAccess] Foo From 83a619a53287f9fb474515da7007d50db9acdec9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Fri, 2 May 2025 12:59:17 -0400 Subject: [PATCH 3/5] Ruby printAst: order by line, synth index in synth parent, column, synth index in real parent This prevents a bunch of unrelated movements in AstDesugar.ql --- ruby/ql/lib/codeql/ruby/printAst.qll | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/printAst.qll b/ruby/ql/lib/codeql/ruby/printAst.qll index 707fa20effde..947f8de06ef2 100644 --- a/ruby/ql/lib/codeql/ruby/printAst.qll +++ b/ruby/ql/lib/codeql/ruby/printAst.qll @@ -134,6 +134,10 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode { else result = 0 } + private int getSynthAstNodeIndexForSynthParent() { + if this.parentIsSynthesized() then result = this.getSynthAstNodeIndex() else result = 0 + } + override int getOrder() { this = rank[result](PrintRegularAstNode p, Location l, File f | @@ -142,8 +146,9 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode { | p order by - f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), l.getStartColumn(), - p.getSynthAstNodeIndex(), l.getEndLine(), l.getEndColumn() + f.getBaseName(), f.getAbsolutePath(), l.getStartLine(), + p.getSynthAstNodeIndexForSynthParent(), l.getStartColumn(), p.getSynthAstNodeIndex(), + l.getEndLine(), l.getEndColumn() ) } From e9d5515c3baeb8e7ac1d9178a219adca7c9a80d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Fri, 2 May 2025 15:32:31 -0400 Subject: [PATCH 4/5] Add change note --- .../lib/change-notes/2025-05-02-ruby-printast-order-fix.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 ruby/ql/lib/change-notes/2025-05-02-ruby-printast-order-fix.md diff --git a/ruby/ql/lib/change-notes/2025-05-02-ruby-printast-order-fix.md b/ruby/ql/lib/change-notes/2025-05-02-ruby-printast-order-fix.md new file mode 100644 index 000000000000..b71b60c22b3d --- /dev/null +++ b/ruby/ql/lib/change-notes/2025-05-02-ruby-printast-order-fix.md @@ -0,0 +1,6 @@ +--- +category: fix +--- +### Bug Fixes + +* The Ruby printAst.qll library now orders AST nodes slightly differently: child nodes that do not literally appear in the source code, but whose parent nodes do, are assigned a deterministic order based on a combination of source location and logical order within the parent. This fixes the non-deterministic ordering that sometimes occurred depending on evaluation order. The effect may also be visible in downstream uses of the printAst library, such as the AST view in the VSCode extension. From 96bdfbf76b7b6485839a864cb564bee8b6272b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nora=20Dimitrijevi=C4=87?= Date: Wed, 14 May 2025 15:36:45 +0200 Subject: [PATCH 5/5] Fix inefficient pattern: if-exists -> exists-or-not-exists --- ruby/ql/lib/codeql/ruby/printAst.qll | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/ruby/ql/lib/codeql/ruby/printAst.qll b/ruby/ql/lib/codeql/ruby/printAst.qll index 947f8de06ef2..97c7119eae29 100644 --- a/ruby/ql/lib/codeql/ruby/printAst.qll +++ b/ruby/ql/lib/codeql/ruby/printAst.qll @@ -121,17 +121,16 @@ class PrintRegularAstNode extends PrintAstNode, TPrintRegularAstNode { } private int getSynthAstNodeIndex() { - if - exists(AstNode parent | - shouldPrintAstEdge(parent, _, astNode) and - synthChild(parent, _, astNode) - ) - then - exists(AstNode parent | - shouldPrintAstEdge(parent, _, astNode) and - synthChild(parent, result, astNode) - ) - else result = 0 + exists(AstNode parent | + shouldPrintAstEdge(parent, _, astNode) and + synthChild(parent, result, astNode) + ) + or + not exists(AstNode parent | + shouldPrintAstEdge(parent, _, astNode) and + synthChild(parent, _, astNode) + ) and + result = 0 } private int getSynthAstNodeIndexForSynthParent() {