[SemaAnnotator] Fix bug causing custom attributes to be walked out of order

[ahoppen]

SemaAnnotator walks the tree in source order with respect to the source ranges excluding attributes, but RangeResolver considers attributes part of their declaration. Thus they disagree on what “walking in source order means”. SemaAnnotator will visit the attributes before the decl they are on, while RangeResolver as currently implemented expects them as part of the decl they are on.

Thus, for the purpose of RangeResolver, we need to assume that nodes are visited in arbitrary order and we might encounter enclosing nodes after their children.

Thus, when we find a new node, remove all nodes that it encloses from ContainedASTNodes.

Fixes rdar://64140713 [SR-12958]

[nathawes]

If I'm remembering correctly, I think custom decl attributes being treated as a sibling rather than child of the decl was intentional so that the decl loc and/or name range would be passed along in source-order relative to the loc/ranges of any symbol references in custom attributes. I think syntactic rename might be relying on that behaviour, so could you add a test case for that based on the occurrences of test in something like the below?

@propertyWrapper
struct Bar<T> {
    let wrappedValue: T
    init(wrappedValue: T, other: Int) {
        self.wrappedValue = wrappedValue
    }
}

struct Foo {
    @Bar(other: Foo.test) // does syntactic rename miss 'test' here if the attribute is visited as a child of the decl?
    static var test: Int = 10
}

[ahoppen]

I added a test for that case, which passes. Could you take a look if the test is actually testing what you thought of?

I don’t know if it makes a difference, but note that the attributes are still walked before any children of the declaration. Their visitation will only be triggered by ASTWalker after walkToDeclPre returns.

[nathawes]

I added a test for that case, which passes. Could you take a look if the test is actually testing what you thought of?

Oh sorry! It is testing the right thing, but after looking at the code it looks like syntactic rename is actually doing its own walk that doesn't use this logic.

I don’t know if it makes a difference, but note that the attributes are still walked before any children of the declaration. Their visitation will only be triggered by ASTWalker after walkToDeclPre returns.

I was worried about the case for SourceEntityWalker subclasses that were relying on getting the locs of the visited/passed things in source order, so for a decl with a custom attributes, getting pre/post/passReference calls for the attributes and any references they contain before the pre call for the decl itself (which has a getStartLoc and getLoc after the attributes), then any child references (with locs after the decl start/name loc).

The only other case I could think of where that might matter was in the ModelASTWalker because it consumes any tokens with locs before the start loc of the node/reference being visited. From the code though, it looks like ModelASTWalker is manually handling the attributes itself in the pre call of the decl they're attached to and consuming them then, so I think when the SemaAnnotator later passes along the attributes as children of the decl it'll end up being a no-op anyway, so isn't a problem.

This LGTM and sorry for the extra work!

[bnbarham]

The comment on SourceEntityWalker does say that "Visitation happens in source-order". Does that not actually matter 🤔 ?

[ahoppen]

The only other case I could think of where that might matter was in the ModelASTWalker because it consumes any tokens with locs before the start loc of the node/reference being visited. From the code though, it looks like ModelASTWalker is manually handling the attributes itself in the pre call of the decl they're attached to and consuming them then, so I think when the SemaAnnotator later passes along the attributes as children of the decl it'll end up being a no-op anyway, so isn't a problem.

IIUC ModelASTWalker is a separate ASTWalker, so it doesn’t share any implementation with SemaAnnotator.

The comment on SourceEntityWalker does say that "Visitation happens in source-order". Does that not actually matter 🤔 ?

From this discussion and Nathan’s comment I assume that it doesn’t. Also, I found the behavior of walking children before their parents very confusing. IIUC we are now visiting the nodes in the order in which they occur in the tree, so I would propose changing the comment to

/// Visitation happens in the order in which nodes appear in the source code and compiler-generated semantic info, like implicit declarations, is ignored.

Does this sound good to you @nathawes or can you think of another place where visitation order would be important that I should take a look at?

[nathawes]

IIUC ModelASTWalker is a separate ASTWalker, so it doesn’t share any implementation with SemaAnnotator.

You'd think I would've checked that after last time 😅

can you think of another place where visitation order would be important that I should take a look at?

AnnotationPrinter::annotateSourceEntity has an assert about the passed-in loc not being beyond the offset it's previously printed to. I think that might get hit with the original custom attribute example I gave. It's only used in swift-ide-test from what I can see, though.

I can see how having it visit in source order is useful, as it makes cases like the annotation printer above easier to implement and more generally makes resolving a list of source locations more efficient, but we don't currently seem to rely on or make use of the source-ordering anywhere besides AnnotationPrinter from what I can see.

[ahoppen]

Sorry to both of you busy with this. It turns out the indexer is relying on walking in source order (or so the indexer test suite tells me in CI) and I agree with Nathan that visitation in source order might be useful. To avoid any larger impact, I am now adjusting RangeResolver instead of SemaAnnotator and don’t make any assumptions in RangeResolver about the order nodes are visited in. I’ve updated the description to explain what I’m doing now.

[ahoppen]

@swift-ci Please smoke test