Include elided tokens in yeast AST node locations

[Copilot]

Yeast rewrites can elide matched tokens when producing the target AST, causing synthesized node locations to shrink to only their retained children. For cases like return x, this made the rewritten return_expr span only x instead of the full return statement.

• Location synthesis

  • Union synthesized node child ranges with the matched source node range.
  • Preserve existing child-derived behavior while allowing elided matched tokens to contribute to the replacement range.

• Regression coverage

  • Added a yeast regression test where a rule drops part of a matched node but the replacement node still spans the original match.

Example:

rule!(
    (call
        method: (identifier) @name
        receiver: (identifier) @recv
    )
    =>
    (call
        method: {name}
    )
)

The synthesized call now keeps the source range of the full matched foo.bar() expression, not just the retained bar child.

[Copilot]

🟢 Approval recommended

The implementation matches the stated behavior and includes focused regression coverage.

Pull request overview

Extends Yeast synthesized-node locations to include elided portions of matched source nodes.

Changes:

• Unions child-derived and inherited match ranges.
• Adds regression coverage for elided tokens.

File summaries

File	Description
shared/yeast/src/lib.rs	Implements source-range unioning.
shared/yeast/tests/test.rs	Verifies the full matched call range is retained.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Balanced

---

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

[tausbn]

Looks good to me! 👍

While I'm not sure if this is the solution we want to use long term, I think this a perfectly good solution right now, especially if it unblocks other work.

[tausbn]

On second thought, looking at the test failure:

--- expected
+++ actual
@@ -1,6 +1,6 @@
 nameExpr
 | name_expr.swift:1:9:1:9 | NameExpr | y |
-| test.swift:1:8:1:17 | NameExpr | Foundation |
+| test.swift:1:1:1:17 | NameExpr | Foundation |
 | test.swift:8:9:8:13 | NameExpr | items |
 | test.swift:8:22:8:25 | NameExpr | item |
 | test.swift:12:16:12:20 | NameExpr | items |
Error: [7/10 comp 2.6s eval 44ms] FAILED(RESULT) /home/runner/work/semmle-code/semmle-code/ql/unified/ql/test/library-tests/BasicTest/test.ql

The code in question is import Foundation, and I'm not sure it's sensible to have the location of the NameExpr be the entire import statement.

[aschackmull]

The code in question is import Foundation, and I'm not sure it's sensible to have the location of the NameExpr be the entire import statement.

That's certainly true - I've no idea why that happens. I'll dig a bit.

[tausbn]

I think we need something that treats the matched "top-level" node of a rule specially.

So, something like:

• The top-level node of the output of a rule gets the location of the outermost node that was matched.
• A child node containing a single capture gets the location of that capture.
• If there are multiple captures, we union their ranges.

I'm not quite sure what to do about childless synthesised nodes (if we have any). Perhaps these should just inherit the range surrounding them.

[aschackmull]

Copilot claims to have fixed it now. I admit I don't have a good enough overview of the code to have complete confidence, but testing locally it appears to work.

[aschackmull]

Rerun has been triggered: 18 restarted 🚀

• codeql-coding-standards Language Tests Linux
• Java Language Tests Linux
• Go Language Tests MacOS
• Java Integration tests MacOS
• Rust Language Tests Linux
• C# Language Tests Linux
• CPP Language Tests Linux
• CPP RTJO Language Tests Linux
• Ruby RTJO Language Tests Linux
• Swift Language Tests MacOS
• Go Language Tests Linux
• Swift RTJO Language Tests MacOS
• Unified Unit Tests
• CPP Integration tests Linux
• Unified Language Tests Linux
• C# Integration tests Linux
• Unified RTJO Language Tests Linux
• Actions Integration tests MacOS

[tausbn]

I had a look at the fix, and it looks okay, if a bit hacky. Unfortunately, there are other issues introduced by the changes in this PR that make some locations worse than before.

For example, a generic type expression like

C<Foo>

becomes a generic_type_expr whose base has the location of the entire expression, not just that of C (which is the case for the current logic).

Similarly, in

switch x { case a, b: c }

the locations of the or_pattern a, b and the body c used to be just those nodes, but now they both have the location of the entire case a, b: c.

At this point, I'm inclined to just wave this through, however. It (presumably) improves locations for the bits that you care about, and I can sit down and implement a better solution after we merge this.

What do you think?

[aschackmull]

At this point, I'm inclined to just wave this through, however. It (presumably) improves locations for the bits that you care about, and I can sit down and implement a better solution after we merge this.

What do you think?

I agree that those do sound like regressions, but I'm out of my depth re. actually properly fixing them. I'd need to invest a lot more time to get to know this codebase. And it doesn't make much sense for me to just act as meat-proxy between you and copilot - that's bound to just lead to worse code. So if you're happy with taking on the followup, then I'll humbly accept.

[tausbn]

Let's get it in. I'll handle the follow-up.