Skip to content

Ruby printAst: fix order for synth children of real parents #19448

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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

d10c
Copy link
Contributor

@d10c d10c commented May 1, 2025

Real parents can have synthesized children, so always assigning index 0 leads to nondeterminism in graph output.

@github-actions github-actions bot added the Ruby label May 1, 2025
@d10c d10c force-pushed the d10c/ruby-printast-order-fix branch from 8341f88 to ed8f10f Compare May 1, 2025 15:31
@d10c d10c added the Run: RTJO Language Tests Run all language tests with --dynamic-join-order-mode=all label May 1, 2025
@d10c d10c marked this pull request as ready for review May 1, 2025 15:52
@Copilot Copilot AI review requested due to automatic review settings May 1, 2025 15:52
@d10c d10c requested a review from a team as a code owner May 1, 2025 15:52
@d10c d10c requested a review from alexet May 1, 2025 15:52
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that synthesized AST children under real (non-synthesized) parents receive the correct index instead of defaulting to zero, making graph output deterministic.

  • Update expected AST print output to reflect the new child ordering
  • Revise getSynthAstNodeIndex logic to compute and assign the correct index when a real parent has synthesized children

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
ruby/ql/test/library-tests/ast/Ast.expected Adjusted expected lines to match reordered synth children
ruby/ql/lib/codeql/ruby/printAst.qll Changed getSynthAstNodeIndex to use if/then for proper index lookup
Comments suppressed due to low confidence (1)

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

  • Consider adding a test case where a real parent has multiple synthesized children to verify that each child gets a unique, deterministic index instead of defaulting to zero.
private int getSynthAstNodeIndex() {

@alexet
Copy link
Contributor

alexet commented May 1, 2025

I think this is quite subtle. Note that printAST results are shown to the user in the ide so this is about more than just the tests. It was explicitly written this way in #12612 . The other tests that change originally had the right order and are now incorrect.

@d10c
Copy link
Contributor Author

d10c commented May 1, 2025

The other tests that change

By that do you mean the changes in this PR? As an alternative, I could leave the synth order the same and use toString() as a final disambiguator as in that PR.

I don't see any test changes from this other than the Ast.ql output.

@alexet
Copy link
Contributor

alexet commented May 2, 2025

I mean the change where the LocalVariableAccess get moved. In that case it does have a useful location compared to the parent but gets moved to a less sensible place in the order. This is all very subtle which is why I'm not sure either of us can fix it as I think the problem goes quite deep.

@d10c d10c added Run: RTJO Language Tests Run all language tests with --dynamic-join-order-mode=all and removed Run: RTJO Language Tests Run all language tests with --dynamic-join-order-mode=all labels May 2, 2025
@d10c
Copy link
Contributor Author

d10c commented May 2, 2025

@alexet Ok, I think I have something more precise now, but I agree, this might have more implications than the tests capture.

d10c added 3 commits May 2, 2025 15:33
Real parents can have synthesized children, so always assigning index 0 leads to nondeterminism in graph output.
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.
…nth index in real parent

This prevents a bunch of unrelated movements in AstDesugar.ql
@d10c d10c force-pushed the d10c/ruby-printast-order-fix branch from 09faa23 to 83edc8e Compare May 2, 2025 19:33
@d10c d10c force-pushed the d10c/ruby-printast-order-fix branch from 83edc8e to e9d5515 Compare May 2, 2025 19:49
@d10c d10c added Run: RTJO Language Tests Run all language tests with --dynamic-join-order-mode=all and removed Run: RTJO Language Tests Run all language tests with --dynamic-join-order-mode=all labels May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Ruby Run: RTJO Language Tests Run all language tests with --dynamic-join-order-mode=all
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants