[IDE] Avoid uses of isBeforeInBuffer in TypeCheckASTNodeAtLocRequest
#81028
Conversation
This file contains hidden or 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
hamishknight
changed the title
hamishknight
force-pushed
the
fix-completion-sourceranges
branch
from
73a7115 to
392339c
Compare
hamishknight
force-pushed
the
fix-completion-sourceranges
branch
2 times, most recently
from
291ba9a to
a99b66f
Compare
hamishknight
force-pushed
the
fix-completion-sourceranges
branch
2 times, most recently
from
8768c53 to
f6ece61
Compare
hamishknight
changed the title
isBeforeInBuffer in TypeCheckASTNodeAtLocRequest
hamishknight
force-pushed
the
fix-completion-sourceranges
branch
from
f6ece61 to
f68b2bf
Compare
|
@swift-ci please test |
hamishknight
requested review from
ahoppen,
bnbarham,
rintaro,
hborla,
slavapestov,
xedin,
ktoso,
CodaFi and
DougGregor
as code owners
|
(this has been stress tested: https://ci.swift.org/view/Pull%20Requests/job/swift-PR-macos-with-sourcekit-stress-tester/398/) |
bnbarham
approved these changes
May 14, 2025
Almost all clients would crash for an invalid location, let's always assert.
Handle PatternBindingDecls with missing var locations, which can happen for loop iterator vars, and FuncDecls with missing name and func locations, which can happen for `defer`. Also while here make sure we set the source location of a parser-produced ErrorExpr.
This wasn't really sound since it could result in source ranges that have different buffers for the start and end loc. Instead, adjust the parser logic to look at the brace range.
These don't appear to be necessary, let's mirror the logic in `getStartLoc` and remove them.
Use the higher level APIs on SourceManager that handle locations in parent vs child buffers. This then allows us to fix `walkToDeclPre` such that we don't set the found DeclContext unless the location is actually within that decl (here the location may well be in a separate buffer as we may have a replaced function body).
Previously we could end up in cases where we pick the wrong DeclContext in `TypeCheckASTNodeAtLocRequest` since we previously weren't checking source ranges, which could result in skipping the type-checking of an outer closure. Now that we correctly pick the DeclContext, we should no longer hit that case, so we should be able to fall into `getTypeForCompletion` and assert that the solution has a type. Also while here let's upgrade that assert to a `CONDITIONAL_ASSERT`.
hamishknight
force-pushed
the
fix-completion-sourceranges
branch
from
f68b2bf to
d00f45a
Compare
|
@swift-ci please smoke test |
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.
Use the higher level APIs on SourceManager that handle locations in parent vs child buffers. This requires a couple of AST node source range fixes such that e.g the implicit function for a
deferstatement andforloop iterator vars have valid source ranges.This then allows us to fix
walkToDeclPresuch that we don't set the found DeclContext unless the location is actually within that decl (the location here may well be in a separate buffer as we may have a replaced function body). This is the more principled fix for #80985.