unified: improve node locations - #22608
Conversation
f662105 to
1a9354d
Compare
NB: This temporarily changes the test output for bulk imports, blocks, and things like synthesised Array and Option literals. Later commits will fix these up again.
The location 0..0 was being treated as "no useable location", which at the beginning of a file could cause problems. We now represent this as None instead, making Some(0..0) a valid empty location.
Adds two new macros, both variants of the existing `tree!` macro. First, `tree_at!` takes an extra argument, and sets the location of the root of the constructed tree to be that of the argument in question. Secondly, `tree_spanning!` does the same construction, but accepts an iterable of nodes instead. It then makes it so that the location of the root node of the constructed tree is the smallest span that contains all of the locations given by the iterable.
For such nodes, we assign them the zero-length location at the beginning of the matched input node. Note: This does mean some `expr_pattern`s now appear to have a fixed `let ` prefix in their source range (when really that `let` should belong to the modifier). This will be fixed in a later commit.
Blocks are slightly awkward, since we destructure them (as codeBlock) in the query, and then reconstruct them (as `block`) in the rule body, meaning the location of the block is (by default) assigned to the wrong place by the heuristic. To get around this, I added a helper function that updates the location appropriately, including handling cases where we only optionally match a block. (Also, in some cases we can fix this by just not destructuring `codeBlock`s in the first place -- there's already a rule that maps `codeBlock` to `block`.)
Handles things like `try!` (which is represented as two separate tokens -- we explicitly union their ranges) and "let" binding modifiers (where we reuse the bindingSpecifier, getting its location and string value for free).
Anchor synthesized generic type expressions for Swift array syntax to the original array node. This keeps the closing bracket in the type range without leaking it into surrounding calls or member accesses. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Trailing commas appear in a lot of places in the swift-syntax AST, and are all attached to the individual values separated by said commas, not the parent node. This means that the default heuristics thinks a call like `foo(1, 2)` has arguments `1,` and `2`, which is undesirable. Since this applies uniformly for a lot of different comma-separated lists, I decided that adding a generic mechanism for ignoring certain fields was the least invasive change. Thus, we now add `trailingComma` to that list, and this means it's automatically excluded from the location calculations, without need a bunch of specific changes to the existing rules.
In some cases (I'm looking at you, access_declaration) we link up things like the modifier of an accessor to a node that lives _outside_ of the declaration itself. Previously this was treated as an error (and the "fix" was to extend the source ranges), but this lead to some weird source skeletons in the output. Instead, we now allow the source of a node to be anywhere (within reason -- it has to be inside the file, and properly UTF-8 aligned), and simply add a trailing ` (external)` to the source annotation. Thus, the fields containing these values do not appear in the source skeleton for the node in question.
Keep inherited property metadata at its original source while limiting each accessor declaration to its own keyword and optional body. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1a9354d to
985155a
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
985155a to
364d59c
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Boundary exclusion can create invalid ranges, and bodyless Swift initializers lose their name node.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Improves Yeast source-range propagation and applies the new location semantics to Swift extraction.
Changes:
- Adds child-derived ranges, explicit location macros, boundary exclusions, and external-child diagnostics.
- Updates Swift desugaring to produce more precise node locations.
- Adds focused tests and regenerates Swift corpus and QL expectations.
| File | Description |
|---|---|
unified/ql/test/library-tests/dataflow/test.expected |
Updates data-flow locations. |
unified/ql/test/library-tests/controlflow/cfg.swift |
Updates inline CFG assertions. |
unified/ql/test/library-tests/controlflow/cfg.expected |
Updates CFG expectations. |
unified/ql/test/library-tests/controlflow/basicblock-slices.expected |
Updates block-range expectations. |
unified/ql/test/library-tests/BasicTest/test.expected |
Updates identifier locations. |
unified/extractor/tests/location_tests.rs |
Adds focused location tests. |
unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.output |
Regenerates tuple-binding output. |
unified/extractor/tests/corpus/swift/variables/property-with-willset-and-didset-observers.output |
Regenerates observer output. |
unified/extractor/tests/corpus/swift/variables/multiple-bindings-on-one-line.output |
Regenerates binding locations. |
unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.output |
Regenerates switch locations. |
unified/extractor/tests/corpus/swift/types/static-function.output |
Regenerates function locations. |
unified/extractor/tests/corpus/swift/types/protocol-with-read-only-and-read-write-property-requirements.output |
Regenerates accessor locations. |
unified/extractor/tests/corpus/swift/types/protocol-declaration.output |
Regenerates protocol output. |
unified/extractor/tests/corpus/swift/types/property-with-getter-and-setter.output |
Regenerates property output. |
unified/extractor/tests/corpus/swift/types/noncopyable-type.output |
Regenerates base-type locations. |
unified/extractor/tests/corpus/swift/types/generic-class-parameters-and-constraints.output |
Regenerates generic locations. |
unified/extractor/tests/corpus/swift/types/extension.output |
Regenerates extension output. |
unified/extractor/tests/corpus/swift/types/enum-with-comma-separated-cases-chained-declaration.output |
Regenerates enum-case output. |
unified/extractor/tests/corpus/swift/types/enum-with-cases.output |
Regenerates enum locations. |
unified/extractor/tests/corpus/swift/types/enum-with-associated-values.output |
Regenerates enum constructors. |
unified/extractor/tests/corpus/swift/types/constructor-with-parameters.output |
Regenerates constructor locations. |
unified/extractor/tests/corpus/swift/types/computed-property.output |
Regenerates computed-property output. |
unified/extractor/tests/corpus/swift/types/class-with-multiple-base-types.output |
Regenerates base-type ranges. |
unified/extractor/tests/corpus/swift/types/class-with-method.output |
Regenerates method locations. |
unified/extractor/tests/corpus/swift/types/class-with-initializer.output |
Regenerates initializer locations. |
unified/extractor/tests/corpus/swift/types/class-inheritance.output |
Regenerates inheritance output. |
unified/extractor/tests/corpus/swift/types/class-function.output |
Regenerates class-function output. |
unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output |
Regenerates accessor-body output. |
unified/extractor/tests/corpus/swift/optionals-and-errors/try-expression.output |
Regenerates try? locations. |
unified/extractor/tests/corpus/swift/optionals-and-errors/try-expression-2.output |
Regenerates try! locations. |
unified/extractor/tests/corpus/swift/optionals-and-errors/throwing-function.output |
Regenerates throwing-function output. |
unified/extractor/tests/corpus/swift/optionals-and-errors/optional-type-annotation.output |
Regenerates optional-type output. |
unified/extractor/tests/corpus/swift/optionals-and-errors/optional-enum-case-binding.output |
Regenerates optional-pattern output. |
unified/extractor/tests/corpus/swift/optionals-and-errors/force-unwrap.output |
Regenerates unwrap locations. |
unified/extractor/tests/corpus/swift/optionals-and-errors/do-catch.output |
Regenerates do-catch output. |
unified/extractor/tests/corpus/swift/optionals-and-errors/catch-where-clauses.output |
Regenerates catch-pattern locations. |
unified/extractor/tests/corpus/swift/operators/unresolved-operator-sequence.output |
Regenerates operator output. |
unified/extractor/tests/corpus/swift/operators/unresolved-operator-sequence-with-ternary.output |
Regenerates ternary-sequence output. |
unified/extractor/tests/corpus/swift/operators/unresolved-operator-sequence-with-casts.output |
Regenerates cast-sequence output. |
unified/extractor/tests/corpus/swift/literals/string-with-interpolation.output |
Regenerates interpolation locations. |
unified/extractor/tests/corpus/swift/functions/variadic-function.output |
Regenerates variadic-function output. |
unified/extractor/tests/corpus/swift/functions/nested-function-type.output |
Regenerates nested-function-type output. |
unified/extractor/tests/corpus/swift/functions/generic-type-alias.output |
Regenerates type-alias locations. |
unified/extractor/tests/corpus/swift/functions/generic-function.output |
Regenerates generic-function output. |
unified/extractor/tests/corpus/swift/functions/function-with-parameters-and-return-type.output |
Regenerates function signature output. |
unified/extractor/tests/corpus/swift/functions/function-with-no-parameters.output |
Regenerates function-body locations. |
unified/extractor/tests/corpus/swift/functions/function-with-named-parameters.output |
Regenerates named-parameter output. |
unified/extractor/tests/corpus/swift/functions/function-with-inout-parameter.output |
Regenerates inout-function output. |
unified/extractor/tests/corpus/swift/functions/function-with-default-parameter-value.output |
Regenerates default-parameter output. |
unified/extractor/tests/corpus/swift/functions/function-call.output |
Regenerates call locations. |
unified/extractor/tests/corpus/swift/functions/call-with-inout-argument.output |
Regenerates inout-call output. |
unified/extractor/tests/corpus/swift/expressions/unsafe-expression.output |
Regenerates unsafe-expression output. |
unified/extractor/tests/corpus/swift/expressions/super-expression.output |
Regenerates superclass locations. |
unified/extractor/tests/corpus/swift/expressions/array-type-metatype.output |
Regenerates array metatype output. |
unified/extractor/tests/corpus/swift/expressions/array-type-constructor.output |
Regenerates array-constructor output. |
unified/extractor/tests/corpus/swift/control-flow/switch-with-labeled-case-pattern-arguments.output |
Regenerates switch-pattern output. |
unified/extractor/tests/corpus/swift/control-flow/switch-with-binding-pattern.output |
Regenerates binding-pattern output. |
unified/extractor/tests/corpus/swift/control-flow/switch-statement.output |
Regenerates switch locations. |
unified/extractor/tests/corpus/swift/control-flow/switch-expression-pattern.output |
Regenerates expression-pattern output. |
unified/extractor/tests/corpus/swift/control-flow/switch-case-item-where-clauses.output |
Regenerates conditional-pattern output. |
unified/extractor/tests/corpus/swift/control-flow/nested-enum-case-pattern.output |
Regenerates nested-pattern output. |
unified/extractor/tests/corpus/swift/control-flow/if-let-optional-binding.output |
Regenerates optional-binding output. |
unified/extractor/tests/corpus/swift/control-flow/guard-let.output |
Regenerates guard-binding output. |
unified/extractor/tests/corpus/swift/control-flow/fallthrough.output |
Regenerates fallthrough output. |
unified/extractor/tests/corpus/swift/control-flow/discard-statement.output |
Regenerates discard-statement output. |
unified/extractor/tests/corpus/swift/control-flow/defer-statement.output |
Regenerates defer-statement output. |
unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.output |
Regenerates sibling-binding output. |
unified/extractor/tests/corpus/swift/collections/tuple-literal.output |
Regenerates tuple locations. |
unified/extractor/tests/corpus/swift/collections/empty-array-literal-with-type.output |
Regenerates array-type output. |
unified/extractor/tests/corpus/swift/closures/trailing-closure.output |
Regenerates trailing-closure output. |
unified/extractor/src/languages/swift/swift.rs |
Applies precise Swift location construction. |
unified/extractor/BUILD.bazel |
Registers location tests. |
shared/yeast/tests/test.rs |
Tests new range semantics. |
shared/yeast/src/range.rs |
Adds range operations and exclusions. |
shared/yeast/src/lib.rs |
Adds location APIs and configuration. |
shared/yeast/src/dump.rs |
Marks external child locations. |
shared/yeast/src/build.rs |
Tracks and finalizes constructed nodes. |
shared/yeast/doc/yeast.md |
Documents location behavior. |
shared/yeast-macros/src/parse.rs |
Finalizes rule results. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
asgerf
left a comment
There was a problem hiding this comment.
Looks good overall. I have some minor comments in the code itself.
As for the new behaviour, I'm quite happy with the new defaults, though I was a little surprised about the method for specifying locations.
Something we discussed in our meeting (which I clearly should have written down) was whether locations could just be specified via a pseudo-field called location.
For example, instead of
let constructor = tree_spanning!(
ctx,
[name, clause],
(constructor_declaration
parameter: {params}
body: {body})
);we could write
let constructor = tree!(
(constructor_declaration
parameter: {params}
body: {body}
location: {[name, clause]})
);which could seamlessly be inlined directly in the rule from which this example was taken
(enumCaseElement
name: @name
parameterClause: (enumCaseParameterClause parameters: _* @params) @@clause)
=>
(class_like_declaration
modifier: {ctx.outer_modifiers.clone()}
modifier: {chained_modifier(&mut ctx)}
modifier: (modifier "enum_case")
name_node: (identifier #{name})
member: (constructor_declaration
parameter: {params}
body: {body}
location: {[name, clause]}))I feel this would make it a lot easier to fix locations here and there without adding a lot of ceremony.
The field name location: can't clash with a real field, because a real field called location would clash with getLocation() on the QL side.
Would it be possible to implement this idea instead of the macro? (or as an alternative to the macro)
| /// range in either their content or constructor argument. Recursive | ||
| /// translations use their own context and therefore do not contribute to | ||
| /// this list. | ||
| created_nodes: BTreeSet<Id>, |
There was a problem hiding this comment.
Why is this a set and not simply a list?
There was a problem hiding this comment.
Coming back to this, the code would be easier to follow if this was called something very explicit like created_nodes_without_source_range.
There was a problem hiding this comment.
We use a set because, well, it's a set of nodes. We add elements to it, and remove elements from it, but the ordering is unimportant.
The comment has been updated, and the field renamed as you suggest in 4639e25.
|
|
||
| ```rust | ||
| let synthetic = tree_at!(ctx, source, (synthetic_node child: (nested value: {child}))); | ||
| ``` |
There was a problem hiding this comment.
Overall this is really confusing to read.
When the desired range belongs to another node,
tree_at!assigns that range
to the template's root.
- Did you mean that
tree_at!can be used to assign a range to the template's root? - "Belongs to another node" -> it's really unclear what this means
let synthetic = tree_at!(ctx, source, (synthetic_node child: (nested value: {child})));
What is source, the second argument passed to tree_at!? Would it make sense to rename to source_range?
Perhaps it would help to preface the descriptions of tree_at! and tree_spanning! with a paragraph like:
The following macros can be used to explicitly set the location associated with a node.
and then also include a full rule! example for each of the macros.
There was a problem hiding this comment.
I agree, the wording is a bit confusing. The intent was that tree_at! and tree_spanning! behave exactly as their tree! counterpart, but the new argument provides the location of the tree being constructed.
The wording has (hopefully) been clarified in 152bf4b.
| parameter: {params} | ||
| return_type: {ret} | ||
| body: (block stmt: {body})) | ||
| body: {block_with_anchor(&mut ctx, body, Some(closure))}) |
There was a problem hiding this comment.
I'd actually argue that the body should not include the curly braces for Swift closure expressions.
The syntax for closures is { [<captures>] <parameters> in <statements> }
The fact that curly braces are typically associated with blocks is just a coincidence IMO.
There was a problem hiding this comment.
Good point! The corresponding source skeletons did look a bit iffy to me.
Fixed in 9f59b2c.
| pattern: (identifierPattern identifier: @@name) | ||
| typeAnnotation: (typeAnnotation type: @ty) | ||
| accessorBlock: (accessorBlock accessors: (codeBlockItem)+ @body))) | ||
| accessorBlock: (accessorBlock accessors: (codeBlockItem)+ @body) @@accessor_block)) |
There was a problem hiding this comment.
Can we change this to a top-level rule translating accessorBlock to block instead?
| parameterClause: (enumCaseParameterClause parameters: _* @params)) @@element | ||
| => | ||
| class_like_declaration { | ||
| let body = tree!((block)); |
There was a problem hiding this comment.
Is there a reason body is not inlined in the template below?
| self = self.empty_at_start(); | ||
| continue; | ||
| } | ||
| if range.start_byte == self.start_byte && range.end_byte > range.start_byte { |
There was a problem hiding this comment.
The second part of this condition,
range.end_byte > range.start_byte
simply checks whether range is non-empty, but it's not really obvious why or whether that was even the intent. It looks like a typo in an attempt to compare range and self.
Two suggestions
- Add
is_empty/is_non_emptymethod to clarify the intent - Remove empty locations from
ignoredbefore entering the loop
Rename the set to describe that it tracks nodes created without an explicit source range, and document why set operations are required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Range::is_empty and filter empty ignored ranges before adjusting matched source boundaries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Translate implicit accessor blocks through a dedicated rule and keep closure delimiters on function expressions rather than their body blocks. Also inline the synthetic enum constructor body. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Explain how explicit location macros affect template roots and provide complete rule examples for tree_at! and tree_spanning!. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
I agree that a bespoke syntax would be preferable here. The reason I shied away from implementing it in this PR was mostly a matter of scope. The current PR already does a lot of things (what with all of the heuristics it implements), and modifying the I am happy to add support for However, there is one wrinkle with the Also, I would suggest that I do this as a follow-up PR, so that we can merge the present PR. It touches a lot of test files, and it has already had a bunch of merge conflicts that I had to fix. In contrast, a follow-up PR that just cleans up the syntax will likely not have many conflicts with work done in parallel. |
asgerf
left a comment
There was a problem hiding this comment.
Makes sense to do the location: field in a follow-up PR 👍.
Thanks for addressing my comments! LGTM

Changes the location-assigning heuristics to implement roughly the following principles:
middlein(outer foo: (middle bar: {baz}))), then it gets a location that spans all of its children (that is, all of the captures that have already been translated, that are assigned as descendants of this node).outerabove) gets the location of the entire node that matched that rule (unless that location has been assigned already, e.g. by recursive translation or explicit assignment).(identifier #{name}), wherenameis some captured node ID, then theidentifiernode gets the location of that capture, whereas the string value is what is actually interpolated.One consequence of the above rules is that if you have something like
then the location of the node emitted by the right hand side is not that of
foo, but rather whatever is assigned tobazwhen it is recursively translated.In cases where this heuristic is insufficient, newly added
tree_at!andtree_spanning!macros may be employed. These take a second argument that specifies the node (or iterable of nodes) from which to take the location of the tree being constructed.Should be reviewed commit-by-commit. I checked a bunch of the location changes in the last commit manually (and instructed Copilot to check the rest). In all cases, the locations are an improvement on what was there before (e.g. a call inside a tuple accidentally including the following
,in its range). This does produce a fair amount of churn, but I think it's somewhat unavoidable.