Skip to content

Commit 3c67b06

Browse files
ahoppenahoppen
authored
Merge pull request #62574 from ahoppen/ahoppen/reuse-ast-context
[SourceKit] Make sure we reuse ASTContext in function bodies for solver-based cursor info
2 parents a490db3 + 9dfeb0a commit 3c67b06

22 files changed

+418
-58
lines changed

‎include/swift/AST/TypeCheckRequests.h

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3103,7 +3103,8 @@ class LookupAllConformancesInContextRequest
31033103

31043104
class CheckRedeclarationRequest
31053105
: public SimpleRequest<
3106-
CheckRedeclarationRequest, evaluator::SideEffect(ValueDecl *),
3106+
CheckRedeclarationRequest,
3107+
evaluator::SideEffect(ValueDecl *, NominalTypeDecl *),
31073108
RequestFlags::SeparatelyCached | RequestFlags::DependencySource |
31083109
RequestFlags::DependencySink> {
31093110
public:
@@ -3113,10 +3114,13 @@ class CheckRedeclarationRequest
31133114
friend SimpleRequest;
31143115

31153116
// Evaluation.
3116-
evaluator::SideEffect
3117-
evaluate(Evaluator &evaluator, ValueDecl *VD) const;
3117+
/// \p SelfNominalType is \c VD->getDeclContext()->getSelfNominalType().
3118+
/// Passed as a parameter in here so this request doesn't tigger self nominal
3119+
/// type computation.
3120+
evaluator::SideEffect evaluate(Evaluator &evaluator, ValueDecl *VD,
3121+
NominalTypeDecl *SelfNominalType) const;
31183122

3119-
public:
3123+
public:
31203124
// Separate caching.
31213125
bool isCached() const { return true; }
31223126
Optional<evaluator::SideEffect> getCachedResult() const;

‎include/swift/Basic/SourceManager.h

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,17 @@ class SourceManager {
166166

167167
SourceLoc getIDEInspectionTargetLoc() const;
168168

169+
/// Returns whether `Range` contains `Loc`. This also respects the
170+
/// `ReplacedRanges`, i.e. if `Loc` is in a range that replaces a range which
171+
/// overlaps `Range`, this also returns `true`.
172+
bool containsRespectingReplacedRanges(SourceRange Range, SourceLoc Loc) const;
173+
174+
/// Returns whether `Enclosing` contains `Inner`. This also respects the
175+
/// `ReplacedRanges`, i.e. if `Inner` is contained in a range that replaces a
176+
/// range which overlaps `Range`, this also returns `true`.
177+
bool rangeContainsRespectingReplacedRanges(SourceRange Enclosing,
178+
SourceRange Inner) const;
179+
169180
const llvm::DenseMap<SourceRange, SourceRange> &getReplacedRanges() const {
170181
return ReplacedRanges;
171182
}

‎lib/AST/TypeCheckRequests.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1324,7 +1324,7 @@ void CheckRedeclarationRequest::writeDependencySink(
13241324
return;
13251325

13261326
if (currentDC->isTypeContext()) {
1327-
if (auto nominal = currentDC->getSelfNominalTypeDecl()) {
1327+
if (auto nominal = std::get<1>(getStorage())) {
13281328
tracker.addUsedMember(nominal, current->getBaseName());
13291329
}
13301330
} else {

‎lib/Basic/SourceLoc.cpp

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,31 @@ SourceLoc SourceManager::getIDEInspectionTargetLoc() const {
4545
.getAdvancedLoc(IDEInspectionTargetOffset);
4646
}
4747

48+
bool SourceManager::containsRespectingReplacedRanges(SourceRange Range,
49+
SourceLoc Loc) const {
50+
if (Loc.isInvalid() || Range.isInvalid()) {
51+
return false;
52+
}
53+
54+
if (Range.contains(Loc)) {
55+
return true;
56+
}
57+
for (const auto &pair : getReplacedRanges()) {
58+
auto OriginalRange = pair.first;
59+
auto NewRange = pair.second;
60+
if (NewRange.contains(Loc) && Range.overlaps(OriginalRange)) {
61+
return true;
62+
}
63+
}
64+
return false;
65+
}
66+
67+
bool SourceManager::rangeContainsRespectingReplacedRanges(
68+
SourceRange Enclosing, SourceRange Inner) const {
69+
return containsRespectingReplacedRanges(Enclosing, Inner.Start) &&
70+
containsRespectingReplacedRanges(Enclosing, Inner.End);
71+
}
72+
4873
StringRef SourceManager::getDisplayNameForLoc(SourceLoc Loc, bool ForceGeneratedSourceToDisk) const {
4974
// Respect #line first
5075
if (auto VFile = getVirtualFile(Loc))

‎lib/IDE/CursorInfo.cpp

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,13 @@ void typeCheckDeclAndParentClosures(ValueDecl *VD) {
5050
typeCheckASTNodeAtLoc(
5151
TypeCheckASTNodeAtLocContext::declContext(VD->getDeclContext()),
5252
VD->getLoc());
53+
if (auto VarD = dyn_cast<VarDecl>(VD)) {
54+
// Type check any attached property wrappers so the annotated declaration
55+
// can refer to their USRs.
56+
(void)VarD->getPropertyWrapperBackingPropertyType();
57+
// Visit emitted accessors so we generated accessors from property wrappers.
58+
VarD->visitEmittedAccessors([&](AccessorDecl *accessor) {});
59+
}
5360
}
5461

5562
// MARK: - NodeFinderResults
@@ -133,7 +140,7 @@ class NodeFinder : ASTWalker {
133140
DeclContext *getCurrentDeclContext() { return DeclContextStack.back(); }
134141

135142
bool rangeContainsLocToResolve(SourceRange Range) const {
136-
return Range.contains(LocToResolve);
143+
return getSourceMgr().containsRespectingReplacedRanges(Range, LocToResolve);
137144
}
138145

139146
PreWalkAction walkToDeclPre(Decl *D) override {
@@ -150,7 +157,10 @@ class NodeFinder : ASTWalker {
150157
}
151158

152159
if (auto VD = dyn_cast<ValueDecl>(D)) {
153-
if (VD->hasName()) {
160+
// FIXME: ParamDecls might be closure parameters that can have ambiguous
161+
// types. The current infrastructure of just asking for the VD's type
162+
// doesn't work here. We need to inspect the constraints system solution.
163+
if (VD->hasName() && !isa<ParamDecl>(D)) {
154164
assert(Result == nullptr);
155165
Result = std::make_unique<NodeFinderDeclResult>(VD);
156166
return Action::Stop();

‎lib/IDETool/IDEInspectionInstance.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -842,10 +842,9 @@ void swift::ide::IDEInspectionInstance::cursorInfo(
842842
CancellationFlag,
843843
[&](CancellableResult<IDEInspectionInstanceResult> CIResult) {
844844
CIResult.mapAsync<CursorInfoResults>(
845-
[&CancellationFlag, Offset](auto &Result, auto DeliverTransformed) {
845+
[&CancellationFlag](auto &Result, auto DeliverTransformed) {
846846
auto &Mgr = Result.CI->getSourceMgr();
847-
auto RequestedLoc = Mgr.getLocForOffset(
848-
Mgr.getIDEInspectionTargetBufferID(), Offset);
847+
auto RequestedLoc = Mgr.getIDEInspectionTargetLoc();
849848
ConsumerToCallbackAdapter Consumer(
850849
Result.DidReuseAST, CancellationFlag, DeliverTransformed);
851850
std::unique_ptr<IDEInspectionCallbacksFactory> callbacksFactory(

‎lib/Refactoring/Refactoring.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,11 @@ class ContextFinder : public SourceEntityWalker {
5252
std::function<bool(ASTNode)> IsContext;
5353
SmallVector<ASTNode, 4> AllContexts;
5454
bool contains(ASTNode Enclosing) {
55-
auto Result = SM.rangeContains(Enclosing.getSourceRange(), Target);
56-
if (Result && IsContext(Enclosing))
55+
auto Result = SM.rangeContainsRespectingReplacedRanges(
56+
Enclosing.getSourceRange(), Target);
57+
if (Result && IsContext(Enclosing)) {
5758
AllContexts.push_back(Enclosing);
59+
}
5860
return Result;
5961
}
6062
public:

‎lib/Sema/TypeCheckDeclPrimary.cpp

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -542,7 +542,8 @@ static void checkRedeclaration(PrecedenceGroupDecl *group) {
542542

543543
/// Check whether \c current is a redeclaration.
544544
evaluator::SideEffect
545-
CheckRedeclarationRequest::evaluate(Evaluator &eval, ValueDecl *current) const {
545+
CheckRedeclarationRequest::evaluate(Evaluator &eval, ValueDecl *current,
546+
NominalTypeDecl *SelfNominalType) const {
546547
// Ignore invalid and anonymous declarations.
547548
if (current->isInvalid() || !current->hasName())
548549
return std::make_tuple<>();
@@ -1872,8 +1873,11 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
18721873
// Force some requests, which can produce diagnostics.
18731874

18741875
// Check redeclaration.
1875-
(void) evaluateOrDefault(Context.evaluator,
1876-
CheckRedeclarationRequest{VD}, {});
1876+
(void)evaluateOrDefault(
1877+
Context.evaluator,
1878+
CheckRedeclarationRequest{
1879+
VD, VD->getDeclContext()->getSelfNominalTypeDecl()},
1880+
{});
18771881

18781882
// Compute access level.
18791883
(void) VD->getFormalAccess();

‎lib/Sema/TypeCheckStmt.cpp

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,11 @@ namespace {
281281
// Otherwise, it'll have been separately type-checked.
282282
if (!CE->isSeparatelyTypeChecked()) {
283283
SetLocalDiscriminators innerVisitor;
284+
if (auto params = CE->getParameters()) {
285+
for (auto *param : *params) {
286+
innerVisitor.setLocalDiscriminator(param);
287+
}
288+
}
284289
CE->getBody()->walk(innerVisitor);
285290
}
286291

@@ -332,16 +337,22 @@ namespace {
332337

333338
/// Set the local discriminator for a named declaration.
334339
void setLocalDiscriminator(ValueDecl *valueDecl) {
335-
if (valueDecl->hasLocalDiscriminator() &&
336-
valueDecl->getRawLocalDiscriminator() ==
337-
ValueDecl::InvalidDiscriminator) {
338-
// Assign the next discriminator.
339-
Identifier name = valueDecl->getBaseIdentifier();
340-
auto &discriminator = DeclDiscriminators[name];
341-
if (discriminator < InitialDiscriminator)
342-
discriminator = InitialDiscriminator;
343-
344-
valueDecl->setLocalDiscriminator(discriminator++);
340+
if (valueDecl->hasLocalDiscriminator()) {
341+
if (valueDecl->getRawLocalDiscriminator() ==
342+
ValueDecl::InvalidDiscriminator) {
343+
// Assign the next discriminator.
344+
Identifier name = valueDecl->getBaseIdentifier();
345+
auto &discriminator = DeclDiscriminators[name];
346+
if (discriminator < InitialDiscriminator)
347+
discriminator = InitialDiscriminator;
348+
349+
valueDecl->setLocalDiscriminator(discriminator++);
350+
} else {
351+
// Assign the next discriminator.
352+
Identifier name = valueDecl->getBaseIdentifier();
353+
auto &discriminator = DeclDiscriminators[name];
354+
discriminator = std::max(discriminator, std::max(InitialDiscriminator, valueDecl->getLocalDiscriminator() + 1));
355+
}
345356
}
346357

347358
// If this is a property wrapper, check for projected storage.

‎lib/SymbolGraphGen/SymbolGraph.cpp

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -547,6 +547,21 @@ void SymbolGraph::serialize(llvm::json::OStream &OS) {
547547
}
548548
});
549549

550+
#ifndef NDEBUG
551+
// FIXME (solver-based-verification-sorting): In assert builds sort the
552+
// edges so we get consistent symbol graph output. This allows us to compare
553+
// the string representation of the symbolgraph between the solver-based
554+
// and AST-based result.
555+
// This can be removed once the AST-based cursor info has been removed.
556+
SmallVector<Edge> Edges(this->Edges.begin(), this->Edges.end());
557+
std::sort(Edges.begin(), Edges.end(), [](const Edge &LHS, const Edge &RHS) {
558+
SmallString<256> LHSTargetUSR, RHSTargetUSR;
559+
LHS.Target.getUSR(LHSTargetUSR);
560+
RHS.Target.getUSR(RHSTargetUSR);
561+
return LHSTargetUSR < RHSTargetUSR;
562+
});
563+
#endif
564+
550565
OS.attributeArray("relationships", [&](){
551566
for (const auto &Relationship : Edges) {
552567
Relationship.serialize(OS);

‎test/SourceKit/CursorInfo/cursor_infer_nonmutating_from_property_wrapper.swift

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
func test() {
2+
struct MyStruct {
3+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):16 %s -- %s | %FileCheck %s
4+
@State var myState
5+
}
6+
}
7+
8+
@propertyWrapper struct State {
9+
init(initialValue value: Int) {}
10+
11+
public var wrappedValue: Int {
12+
get { return 1 }
13+
nonmutating set { }
14+
}
15+
}
16+
17+
// CHECK: <Declaration>@<Type usr="s:46cursor_infer_nonmutating_from_property_wrapper5StateV">State</Type> var myState: &lt;&lt;error type&gt;&gt; { get nonmutating set }</Declaration>

‎test/SourceKit/CursorInfo/cursor_local_var_with_property_wrapper.swift

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
@propertyWrapper struct State {
2+
public init(initialValue value: Int)
3+
public var wrappedValue: Int { get nonmutating set }
4+
}
5+
6+
func loadPage() {
7+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):14 %s -- %s
8+
@State var pageListener: Int
9+
}

‎test/SourceKit/CursorInfo/cursor_on_closure_parameter.swift

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
func save(_ record: Int, _ x: (String) -> Void) {}
2+
3+
func test() {
4+
let record = 2
5+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):19 %s -- %s
6+
save(record) { (record) in
7+
}
8+
}

‎test/SourceKit/CursorInfo/cursor_on_var_in_extension_in_func.swift

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
func foo() {
2+
extension XXX {
3+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 1):9 %s -- %s
4+
var hex
5+
}
6+
}

‎test/SourceKit/CursorInfo/cursor_reuses_astcontext.swift

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 2):7 %s -- %s == -req=cursor -pos=%(line + 3):7 %s -- %s | %FileCheck %s --check-prefix IN-FUNCTION
2+
func foo() {
3+
let inFunctionA = 1
4+
let inFunctionB = "hi"
5+
}
6+
7+
// IN-FUNCTION: source.lang.swift.decl.var.local
8+
// IN-FUNCTION-NEXT: inFunctionA
9+
// IN-FUNCTION: DID REUSE AST CONTEXT: 0
10+
// IN-FUNCTION: source.lang.swift.decl.var.local
11+
// IN-FUNCTION-NEXT: inFunctionB
12+
// IN-FUNCTION: DID REUSE AST CONTEXT: 1
13+
14+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 3):9 %s -- %s == -req=cursor -pos=%(line + 4):9 %s -- %s | %FileCheck %s --check-prefix IN-INSTANCE-METHOD
15+
struct MyStruct {
16+
func test() {
17+
let inInstanceMethod1 = 2
18+
let inInstanceMethod2 = "hello"
19+
}
20+
}
21+
22+
// IN-INSTANCE-METHOD: source.lang.swift.decl.var.local
23+
// IN-INSTANCE-METHOD-NEXT: inInstanceMethod1
24+
// IN-INSTANCE-METHOD: DID REUSE AST CONTEXT: 0
25+
// IN-INSTANCE-METHOD: source.lang.swift.decl.var.local
26+
// IN-INSTANCE-METHOD-NEXT: inInstanceMethod2
27+
// IN-INSTANCE-METHOD: DID REUSE AST CONTEXT: 1

0 commit comments

Comments
 (0)