[ASTScope] Re-enable checkSourceRangeBeforeAddingChild
#80339
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Such bindings may have a local decl context, but their scope shouldn't extend past the `@abi` attribute.
Rather than looking at their DeclContext, look at the location of their storage. This allows us to differentiate accessors added by macro expansions vs accessors that are attached to storage that's in a macro expansion.
`visitParsedAccessors` currently iterates over any non-implicit accessor that happens to be present. This means we end up with accessors added by macro expansions if the expansion happens before the ASTScope expansion. Make sure we avoid adding such accessors to the ASTScope tree, they should instead use their own tree. Ideally we'd better formalize `visitParsedAccessors` such that it only ever visits non-expansion accessors, but I'm leaving that for another day.
Make sure it receives a SourceRange that covers the entire body, since that's what it replaces.
Given we're working with CharSourceRanges, the end location is past the end of the actual range, so when checking if e.g a function body macro is contained within the function decl, we fail since the end location is past the closing `}`. Check the start location instead, which is part of the range.
Update the logic to correctly handle replacement ranges, and re-enable the assertion. For now, carve out an exception for attributes on extensions, I'll try and tackle those in a follow-up.
hamishknight
requested review from
hborla,
slavapestov,
xedin and
DougGregor
as code owners
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please SourceKit stress test |
hamishknight
commented
Mar 27, 2025
Comment on lines
+839
to
+843
| ->originalSourceRange.getStart(); | ||
| SourceLoc secondLocInLCA = secondMismatch == secondAncestors.end() | ||
| ? secondLoc | ||
| : sourceMgr.getGeneratedSourceInfo(*secondMismatch) | ||
| ->originalSourceRange.getEnd(); | ||
| ->originalSourceRange.getStart(); |
hamishknight
commented
Mar 27, 2025
Comment on lines
+1014
to
+1021
| // For accessors, the storage decl is the more accurate thing to check | ||
| // since the entire property/subscript could be macro-generated, in which | ||
| // case the accessor shouldn't be considered "added by macro expansion". | ||
| if (auto *accessor = dyn_cast<AccessorDecl>(this)) { | ||
| auto storageLoc = accessor->getStorage()->getLoc(); | ||
| if (storageLoc.isValid()) | ||
| return mod->getSourceFileContainingLocation(storageLoc); | ||
| } |
There was a problem hiding this comment.
We could split this logic out into a separate method specific to AccessorDecl, but it seems to me that this is the behavior that clients would expect for isInMacroExpansionInContext, even if it's not strictly based on the DeclContext
There was a problem hiding this comment.
Yeah I agree that what you have here is the intended behavior of isInMacroExpansionInContext
Use the same approach as `ModuleFileSharedCore::fatal`, ensuring the verification error ends up in the crash log.
|
@swift-ci please smoke test |
|
Figured we also ought to be printing the verification error in a pretty stack trace rather than stderr, such that it's picked up in crash logs, pushed a commit for that |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems like this was accidentally disabled in #63023. This requires fixing a few different cases to avoid hitting the assertion, mostly around macro expansions. Attribute scopes on extensions are the only remaining case I'm aware of that hit this assertion, I've disabled checking them for now since it requires reworking how we handle extension scopes (in particular generic parameter lookup for extensions). I'll try and get to that in a follow-up PR.