Skip to content

Commit fcc5d98

Browse files
committed
[CursorInfo] Deliver results from solver-based cursor info
Running the SourceKit stress tester with verification of solver-based cursor info returned quite a few differences but in all of them, the old AST-based implementation was actually incorrect. So, instead of verifying the results, deliver the results from solver-baesd cursor info and only fall back to AST-based cursor info if the solver-based implementation returned no results. rdar://103369449
1 parent 6ee695a commit fcc5d98

File tree

10 files changed

+50
-175
lines changed

10 files changed

+50
-175
lines changed

test/SourceKit/CompileNotifications/cursor-info.swift

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,3 @@
1-
// rdar://103369449
2-
// REQUIRES: asserts
3-
41
// RUN: %sourcekitd-test -req=track-compiles == -req=cursor %s -offset=0 -- %s | %FileCheck %s -check-prefix=COMPILE_1 --enable-yaml-compatibility
52
// COMPILE_1: <empty cursor info; internal diagnostic: "Unable to resolve cursor info.">
63
// COMPILE_1: {
@@ -12,7 +9,7 @@
129
// COMPILE_1: key.notification: source.notification.compile-did-finish,
1310
// COMPILE_1: key.compileid: [[CID1]]
1411
// COMPILE_1: }
15-
// FIXME: Once we switch to only run solver-based cursor info, we should only receive a single compile notification
12+
// FIXME: Once all cursor info kinds are migrated to solver-based and we remove the fallback path to AST-based cursor info, we should only receive a single compile notification
1613
// COMPILE_1: {
1714
// COMPILE_1: key.notification: source.notification.compile-will-start,
1815
// COMPILE_1: key.filepath: "SOURCE_DIR{{.*}}cursor-info.swift",

test/SourceKit/CursorInfo/cursor_in_pound_if.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,5 @@ func foo() {
88
let xxx = "hello"
99
#endif
1010
}
11-
// TODO: Once we switch to use the solver-based cursor info implementation, we also receive results for the int case
12-
// CHECK-INT: Unable to resolve cursor info
11+
// CHECK-INT: <Declaration>let xxx: <Type usr="s:Si">Int</Type></Declaration>
1312
// CHECK-STR: <Declaration>let xxx: <Type usr="s:SS">String</Type></Declaration>

test/SourceKit/CursorInfo/discriminator.swift

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,12 +35,12 @@ func testNestedClosures() {
3535
}
3636

3737
func testReuseAST(bytes: Int) {
38-
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 2):7 -req-opts=verifysolverbasedcursorinfo=1 %s -- %s == \
39-
// RUN: -req=cursor -pos=%(line + 2):7 -req-opts=verifysolverbasedcursorinfo=1 %s -- %s | %FileCheck %s --check-prefix=REUSE_AST
38+
// RUN: %sourcekitd-test -req=cursor -pos=%(line + 2):7 %s -- %s == \
39+
// RUN: -req=cursor -pos=%(line + 2):7 %s -- %s | %FileCheck %s --check-prefix=REUSE_AST
4040
let size = 3
4141
var bytes = 6
4242
// REUSE_AST: source.lang.swift.decl.var.local (40:7-40:11)
4343
// REUSE_AST: s:13discriminator12testReuseAST5bytesySi_tF4sizeL_Sivp
4444
// REUSE_AST: source.lang.swift.decl.var.local (41:7-41:12)
4545
// REUSE_AST: s:13discriminator12testReuseAST5bytesySi_tFACL0_Sivp
46-
}
46+
}

tools/SourceKit/include/SourceKit/Core/LangSupport.h

Lines changed: 5 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -519,8 +519,7 @@ struct CursorSymbolInfo {
519519

520520
llvm::Optional<unsigned> ParentNameOffset;
521521

522-
void print(llvm::raw_ostream &OS, std::string Indentation,
523-
bool ForSolverBasedCursorInfoVerification = false) const {
522+
void print(llvm::raw_ostream &OS, std::string Indentation) const {
524523
OS << Indentation << "CursorSymbolInfo" << '\n';
525524
OS << Indentation << " Kind: " << Kind.getName() << '\n';
526525
OS << Indentation << " DeclarationLang: " << DeclarationLang.getName()
@@ -529,14 +528,7 @@ struct CursorSymbolInfo {
529528
OS << Indentation << " USR: " << USR << '\n';
530529
OS << Indentation << " TypeName: " << TypeName << '\n';
531530
OS << Indentation << " TypeUSR: " << TypeUSR << '\n';
532-
// The ContainerTypeUSR varies too much between the solver-based and
533-
// AST-based implementation. A few manual inspections showed that the
534-
// solver-based container is usually more correct than the old. Instead of
535-
// fixing the AST-based container type computation, exclude the container
536-
// type from the verification.
537-
if (!ForSolverBasedCursorInfoVerification) {
538-
OS << Indentation << " ContainerTypeUSR: " << ContainerTypeUSR << '\n';
539-
}
531+
OS << Indentation << " ContainerTypeUSR: " << ContainerTypeUSR << '\n';
540532
OS << Indentation << " DocComment: " << DocComment << '\n';
541533
OS << Indentation << " GroupName: " << GroupName << '\n';
542534
OS << Indentation << " LocalizationKey: " << LocalizationKey << '\n';
@@ -600,23 +592,17 @@ struct CursorInfoData {
600592
/// Whether the ASTContext was reused for this cursor info.
601593
bool DidReuseAST = false;
602594

603-
/// If \p ForSolverBasedCursorInfoVerification is \c true, fields that are
604-
/// acceptable to differ between the AST-based and the solver-based result,
605-
/// will be excluded.
606-
void print(llvm::raw_ostream &OS, std::string Indentation,
607-
bool ForSolverBasedCursorInfoVerification = false) const {
595+
void print(llvm::raw_ostream &OS, std::string Indentation) const {
608596
OS << Indentation << "CursorInfoData" << '\n';
609597
OS << Indentation << " Symbols:" << '\n';
610598
for (auto Symbol : Symbols) {
611-
Symbol.print(OS, Indentation + " ", ForSolverBasedCursorInfoVerification);
599+
Symbol.print(OS, Indentation + " ");
612600
}
613601
OS << Indentation << " AvailableActions:" << '\n';
614602
for (auto AvailableAction : AvailableActions) {
615603
AvailableAction.print(OS, Indentation + " ");
616604
}
617-
if (!ForSolverBasedCursorInfoVerification) {
618-
OS << Indentation << "DidReuseAST: " << DidReuseAST << '\n';
619-
}
605+
OS << Indentation << "DidReuseAST: " << DidReuseAST << '\n';
620606
}
621607

622608
SWIFT_DEBUG_DUMP { print(llvm::errs(), ""); }
@@ -973,7 +959,6 @@ class LangSupport {
973959
bool SymbolGraph, bool CancelOnSubsequentRequest,
974960
ArrayRef<const char *> Args, Optional<VFSOptions> vfsOptions,
975961
SourceKitCancellationToken CancellationToken,
976-
bool VerifySolverBasedCursorInfo,
977962
std::function<void(const RequestResult<CursorInfoData> &)> Receiver) = 0;
978963

979964
virtual void

tools/SourceKit/lib/SwiftLang/SwiftLangSupport.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,6 @@ class SwiftLangSupport : public LangSupport {
637637
ArrayRef<const char *> Args,
638638
Optional<VFSOptions> vfsOptions,
639639
SourceKitCancellationToken CancellationToken,
640-
bool VerifySolverBasedCursorInfo,
641640
std::function<void(const RequestResult<CursorInfoData> &)>
642641
Receiver) override;
643642

tools/SourceKit/lib/SwiftLang/SwiftSourceDocInfo.cpp

Lines changed: 39 additions & 135 deletions
Original file line numberDiff line numberDiff line change
@@ -1913,7 +1913,6 @@ void SwiftLangSupport::getCursorInfo(
19131913
bool SymbolGraph, bool CancelOnSubsequentRequest,
19141914
ArrayRef<const char *> Args, Optional<VFSOptions> vfsOptions,
19151915
SourceKitCancellationToken CancellationToken,
1916-
bool VerifySolverBasedCursorInfo,
19171916
std::function<void(const RequestResult<CursorInfoData> &)> Receiver) {
19181917

19191918
std::string error;
@@ -1963,141 +1962,46 @@ void SwiftLangSupport::getCursorInfo(
19631962
return;
19641963
}
19651964

1966-
/// Counts how many symbols \p Res contains.
1967-
auto ResultCount = [](const RequestResult<CursorInfoData> &Res) -> size_t {
1968-
if (Res.isValue()) {
1969-
return Res.value().Symbols.size();
1970-
} else {
1971-
return 0;
1972-
}
1973-
};
1974-
1975-
/// Serializes \c CursorInfoData into a string.
1976-
auto ResultDescription =
1977-
[](const RequestResult<CursorInfoData> &Res) -> std::string {
1978-
if (Res.isCancelled()) {
1979-
return "cancelled";
1980-
} else if (Res.isError()) {
1981-
return Res.getError().str();
1982-
} else {
1983-
std::string Description;
1984-
llvm::raw_string_ostream OS(Description);
1985-
Res.value().print(OS, /*Indentation=*/"",
1986-
/*ForSolverBasedCursorInfoVerification=*/true);
1987-
return OS.str();
1988-
}
1989-
};
1990-
1991-
// Currently, we only verify that the solver-based cursor implementation
1992-
// produces the same results as the AST-based implementation. Only enable it
1993-
// in assert builds for now.
1994-
1995-
// If solver based completion is enabled, a string description of the cursor
1996-
// info result produced by the solver-based implementation. Once the AST-based
1997-
// result is produced, we verify that the solver-based result matches the
1998-
// AST-based result.
1999-
std::string SolverBasedResultDescription;
2000-
size_t SolverBasedResultCount = 0;
2001-
bool SolverBasedReusedAST = false;
2002-
if (VerifySolverBasedCursorInfo) {
2003-
std::string InputFileError;
2004-
llvm::SmallString<64> RealInputFilePath;
2005-
fileSystem->getRealPath(InputFile, RealInputFilePath);
2006-
std::unique_ptr<llvm::MemoryBuffer> UnresolvedInputFile =
2007-
getASTManager()->getMemoryBuffer(RealInputFilePath, fileSystem,
2008-
InputFileError);
2009-
if (UnresolvedInputFile) {
2010-
auto SolverBasedReceiver = [&](const RequestResult<CursorInfoData> &Res) {
2011-
SolverBasedResultCount = ResultCount(Res);
2012-
SolverBasedResultDescription = ResultDescription(Res);
2013-
if (Res.isValue()) {
2014-
SolverBasedReusedAST = Res.value().DidReuseAST;
2015-
}
2016-
};
2017-
2018-
CompilerInvocation CompInvok;
2019-
Invok->applyTo(CompInvok);
2020-
2021-
performWithParamsToCompletionLikeOperation(
2022-
UnresolvedInputFile.get(), Offset,
2023-
/*InsertCodeCompletionToken=*/false, Args, fileSystem,
2024-
CancellationToken,
2025-
[&](CancellableResult<CompletionLikeOperationParams> ParmsResult) {
2026-
ParmsResult.mapAsync<CursorInfoResults>(
2027-
[&](auto &Params, auto DeliverTransformed) {
2028-
getIDEInspectionInstance()->cursorInfo(
2029-
Params.Invocation, Args, fileSystem,
2030-
Params.completionBuffer, Offset, Params.DiagC,
2031-
Params.CancellationFlag, DeliverTransformed);
2032-
},
2033-
[&](auto Result) {
2034-
deliverCursorInfoResults(SolverBasedReceiver, Result, *this,
2035-
CompInvok, Actionables, SymbolGraph);
2036-
});
2037-
});
2038-
}
2039-
}
2040-
2041-
/// If the solver-based implementation returned a different result than the
2042-
/// AST-based implementation, return an error message, describing the
2043-
/// difference. Otherwise, return an empty string.
2044-
auto VerifySolverBasedResult =
2045-
[ResultCount, ResultDescription, SolverBasedResultCount,
2046-
SolverBasedResultDescription](
2047-
const RequestResult<CursorInfoData> &ASTBasedResult) -> std::string {
2048-
if (SolverBasedResultDescription.empty()) {
2049-
// We did not run the solver-based implementation. Nothing to check.
2050-
return "";
2051-
}
2052-
auto ASTResultDescription = ResultDescription(ASTBasedResult);
2053-
auto ASTResultCount = ResultCount(ASTBasedResult);
2054-
if (ASTResultCount == 0 && SolverBasedResultCount > 0) {
2055-
// The AST-based implementation did not return any results but the
2056-
// solver-based did. That's an improvement. Success.
2057-
return "";
2058-
}
2059-
if (SolverBasedResultDescription == ASTResultDescription) {
2060-
// The solver-based and AST-based implementation produced the same
2061-
// results. Success.
2062-
return "";
2063-
}
2064-
// The solver-based implementation differed from the AST-based
2065-
// implementation. Report a failure.
2066-
std::string ErrorMessage;
2067-
llvm::raw_string_ostream OS(ErrorMessage);
2068-
OS << "The solver-based implementation returned a different result than "
2069-
"the AST-based implementation:\n";
2070-
OS << SolverBasedResultDescription << "\n";
2071-
OS << "===== (solver-based vs. AST-based) =====\n";
2072-
OS << ASTResultDescription << "\n";
2073-
return OS.str();
2074-
};
2075-
2076-
// Thunk around `Receiver` that, if solver-based cursor info is enabled,
2077-
// verifies that the solver-based cursor info result matches the AST-based
2078-
// result.
2079-
auto ReceiverThunk =
2080-
[Receiver, VerifySolverBasedResult,
2081-
SolverBasedReusedAST](const RequestResult<CursorInfoData> &Res) {
2082-
auto VerificationError = VerifySolverBasedResult(Res);
2083-
if (VerificationError.empty()) {
2084-
if (Res.isValue()) {
2085-
// Report whether the solver-based implemenatation reused the AST so
2086-
// we can check it in test cases.
2087-
auto Value = Res.value();
2088-
Value.DidReuseAST = SolverBasedReusedAST;
2089-
Receiver(RequestResult<CursorInfoData>::fromResult(Value));
2090-
} else {
2091-
Receiver(Res);
2092-
}
2093-
} else {
2094-
Receiver(RequestResult<CursorInfoData>::fromError(VerificationError));
2095-
}
2096-
};
1965+
bool SolverBasedProducedResult = false;
1966+
std::string InputFileError;
1967+
llvm::SmallString<64> RealInputFilePath;
1968+
fileSystem->getRealPath(InputFile, RealInputFilePath);
1969+
std::unique_ptr<llvm::MemoryBuffer> UnresolvedInputFile =
1970+
getASTManager()->getMemoryBuffer(RealInputFilePath, fileSystem,
1971+
InputFileError);
1972+
if (UnresolvedInputFile) {
1973+
auto SolverBasedReceiver = [&](const RequestResult<CursorInfoData> &Res) {
1974+
SolverBasedProducedResult = true;
1975+
Receiver(Res);
1976+
};
20971977

2098-
resolveCursor(*this, InputFile, Offset, Length, Actionables, SymbolGraph,
2099-
Invok, /*TryExistingAST=*/true, CancelOnSubsequentRequest,
2100-
fileSystem, CancellationToken, ReceiverThunk);
1978+
CompilerInvocation CompInvok;
1979+
Invok->applyTo(CompInvok);
1980+
1981+
performWithParamsToCompletionLikeOperation(
1982+
UnresolvedInputFile.get(), Offset,
1983+
/*InsertCodeCompletionToken=*/false, Args, fileSystem,
1984+
CancellationToken,
1985+
[&](CancellableResult<CompletionLikeOperationParams> ParmsResult) {
1986+
ParmsResult.mapAsync<CursorInfoResults>(
1987+
[&](auto &Params, auto DeliverTransformed) {
1988+
getIDEInspectionInstance()->cursorInfo(
1989+
Params.Invocation, Args, fileSystem,
1990+
Params.completionBuffer, Offset, Params.DiagC,
1991+
Params.CancellationFlag, DeliverTransformed);
1992+
},
1993+
[&](auto Result) {
1994+
deliverCursorInfoResults(SolverBasedReceiver, Result, *this,
1995+
CompInvok, Actionables, SymbolGraph);
1996+
});
1997+
});
1998+
}
1999+
2000+
if (!SolverBasedProducedResult) {
2001+
resolveCursor(*this, InputFile, Offset, Length, Actionables, SymbolGraph,
2002+
Invok, /*TryExistingAST=*/true, CancelOnSubsequentRequest,
2003+
fileSystem, CancellationToken, Receiver);
2004+
}
21012005
}
21022006

21032007
void SwiftLangSupport::getDiagnostics(

tools/SourceKit/tools/sourcekitd-test/sourcekitd-test.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -772,8 +772,6 @@ static int handleTestInvocation(TestOptions Opts, TestOptions &InitOpts) {
772772
} else {
773773
sourcekitd_request_dictionary_set_int64(Req, KeyOffset, ByteOffset);
774774
}
775-
sourcekitd_request_dictionary_set_int64(Req, KeyVerifySolverBasedCursorInfo,
776-
true);
777775
addRequestOptionsDirect(Req, Opts);
778776
break;
779777
case SourceKitRequest::RangeInfo: {

tools/SourceKit/tools/sourcekitd/lib/Service/Requests.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1393,13 +1393,10 @@ handleRequestCursorInfo(const RequestDict &Req,
13931393
Req.getInt64(KeyRetrieveRefactorActions, Actionables, /*isOptional=*/true);
13941394
int64_t SymbolGraph = false;
13951395
Req.getInt64(KeyRetrieveSymbolGraph, SymbolGraph, /*isOptional=*/true);
1396-
int64_t VerifySolverBasedCursorInfo = false;
1397-
Req.getInt64(KeyVerifySolverBasedCursorInfo, VerifySolverBasedCursorInfo,
1398-
/*isOptional=*/true);
13991396
return Lang.getCursorInfo(
14001397
*SourceFile, Offset, Length, Actionables, SymbolGraph,
14011398
CancelOnSubsequentRequest, Args, std::move(vfsOptions),
1402-
CancellationToken, VerifySolverBasedCursorInfo,
1399+
CancellationToken,
14031400
[Rec](const RequestResult<CursorInfoData> &Result) {
14041401
reportCursorInfo(Result, Rec);
14051402
});

unittests/SourceKit/SwiftLang/CursorInfoTest.cpp

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,6 @@ class CursorInfoTest : public ::testing::Test {
166166
DocName, Offset, /*Length=*/0, /*Actionables=*/false,
167167
/*SymbolGraph=*/false, CancelOnSubsequentRequest, Args,
168168
/*vfsOptions=*/None, CancellationToken,
169-
/*VerifySolverBasedCursorInfo=*/true,
170169
[&](const RequestResult<CursorInfoData> &Result) {
171170
assert(!Result.isCancelled());
172171
if (Result.isError()) {
@@ -452,7 +451,6 @@ TEST_F(CursorInfoTest, CursorInfoCancelsPreviousRequest) {
452451
SlowDocName, SlowOffset, /*Length=*/0, /*Actionables=*/false,
453452
/*SymbolGraph=*/false, /*CancelOnSubsequentRequest=*/true, ArgsForSlow,
454453
/*vfsOptions=*/None, /*CancellationToken=*/nullptr,
455-
/*VerifySolverBasedCursorInfo=*/true,
456454
[&](const RequestResult<CursorInfoData> &Result) {
457455
EXPECT_TRUE(Result.isCancelled());
458456
FirstCursorInfoSema.signal();
@@ -496,7 +494,6 @@ TEST_F(CursorInfoTest, CursorInfoCancellation) {
496494
SlowDocName, SlowOffset, /*Length=*/0, /*Actionables=*/false,
497495
/*SymbolGraph=*/false, /*CancelOnSubsequentRequest=*/false, ArgsForSlow,
498496
/*vfsOptions=*/None, /*CancellationToken=*/CancellationToken,
499-
/*VerifySolverBasedCursorInfo=*/true,
500497
[&](const RequestResult<CursorInfoData> &Result) {
501498
EXPECT_TRUE(Result.isCancelled());
502499
CursorInfoSema.signal();

utils/gyb_sourcekit_support/UIDs.py

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,6 @@ def __init__(self, internal_name, external_name):
189189
KEY('OptimizeForIDE', 'key.optimize_for_ide'),
190190
KEY('RequiredBystanders', 'key.required_bystanders'),
191191
KEY('ReusingASTContext', 'key.reusingastcontext'),
192-
KEY('VerifySolverBasedCursorInfo', 'key.verifysolverbasedcursorinfo'),
193192
KEY('CompletionMaxASTContextReuseCount',
194193
'key.completion_max_astcontext_reuse_count'),
195194
KEY('CompletionCheckDependencyInterval',

0 commit comments

Comments
 (0)