Skip to content

Unified: Fix translation of tuples and grouping exprs - #22551

Draft
asgerf wants to merge 2 commits into
github:mainfrom
asgerf:unified/tuples
Draft

Unified: Fix translation of tuples and grouping exprs#22551
asgerf wants to merge 2 commits into
github:mainfrom
asgerf:unified/tuples

Conversation

@asgerf

@asgerf asgerf commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Tuples were accidentally translated into leaf nodes. They are now properly translated into TupleExpr.

The input AST represents parenthesized expressions as unary tuples, so we unfold them. Note that unlike Python, it is not possible to construct a unary tuple in Swift (or at least there is no syntax for it).

Speaking of parenthesized expressions, we'll want to eventually model this similarly to Java, where parentheses are stripped from the AST, and a side table records the number of enclosing parentheses for a given expr (and perhaps the location of the outermost parenthesis pair). The side table can come later; right now we just strip parentheses.

@asgerf asgerf added the no-change-note-required This PR does not need a change note label Sep 11, 2026
@asgerf
asgerf requested a balanced review from Copilot September 11, 2026 12:06
| test.swift:74:10:74:17 | isSorted | isSorted |
| test.swift:74:24:74:27 | Bool | Bool |
| test.swift:75:13:75:13 | i | i |
| test.swift:75:23:75:27 | count | count |

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change is because count appeared inside a parenthesis on line 75:

for i in 0..<(count - 1) {

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Unresolved critical and moderate findings remain in Swift tuple handling and corpus coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity

New issues introduced by this change (1)
Severity Finding
High severity unified/​extractor/​src/​languages/​swift/​swift.rs — Regenerate the stale tuple expression corpus fixture
What changed in this PR

This pull request updates Swift AST translation to structure tuple literals and unwrap ordinary parenthesized expressions.

Changes:

  • Translates tuple elements into argument children.
  • Removes grouping-expression wrappers.
  • Updates QL tests and Swift extractor fixtures.
File Summary
unified/​ql/​test/​library-tests/​BasicTest/​test.swift Adds tuple and grouping-expression cases.
unified/​ql/​test/​library-tests/​BasicTest/​test.ql Verifies grouping expressions are unwrapped.
unified/​ql/​test/​library-tests/​BasicTest/​test.expected Updates expected query results.
unified/​extractor/​tests/​corpus/​swift/​operators/​parenthesised-expression.output Updates parenthesized-expression output.
unified/​extractor/​tests/​corpus/​swift/​collections/​tuple-literal.output Updates tuple-literal output.
unified/​extractor/​src/​languages/​swift/​swift.rs Implements tuple/grouping translation; has critical (1 vote) and moderate (1 vote) unresolved findings.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread unified/extractor/src/languages/swift/swift.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The changes are fully reviewed and have no unresolved blocking issues.

Review tier: Lite
Findings: None

Issues resolved since last review (1)
Severity Finding
High severity unified/​extractor/​src/​languages/​swift/​swift.rs — Regenerate the stale tuple expression corpus fixture View resolved comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants