Skip to content

Commit

Permalink
[clangd] Be more explicit on testing the optional DefLoc in LocatedSy…
Browse files Browse the repository at this point in the history 
…mbol.

And also fix a bug where we may return a meaningless location.

Differential Revision: https://reviews.llvm.org/D84919
  • Loading branch information
@hokein
hokein committed Jul 31, 2020
1 parent 0d25d3b commit 638f0cf
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 36 deletions.
13 changes: 5 additions & 8 deletions clang-tools-extra/clangd/XRefs.cpp
View file
Expand Up @@ -405,15 +405,17 @@ locateSymbolTextually(const SpelledWord &Word, ParsedAST &AST,
log("locateSymbolNamedTextuallyAt: {0}", MaybeDeclLoc.takeError());
return;
}
Location DeclLoc = *MaybeDeclLoc;
Location DefLoc;
LocatedSymbol Located;
Located.PreferredDeclaration = *MaybeDeclLoc;
Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
if (Sym.Definition) {
auto MaybeDefLoc = indexToLSPLocation(Sym.Definition, MainFilePath);
if (!MaybeDefLoc) {
log("locateSymbolNamedTextuallyAt: {0}", MaybeDefLoc.takeError());
return;
}
DefLoc = *MaybeDefLoc;
Located.PreferredDeclaration = *MaybeDefLoc;
Located.Definition = *MaybeDefLoc;
}

if (ScoredResults.size() >= 3) {
Expand All @@ -424,11 +426,6 @@ locateSymbolTextually(const SpelledWord &Word, ParsedAST &AST,
return;
}

LocatedSymbol Located;
Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
Located.PreferredDeclaration = bool(Sym.Definition) ? DefLoc : DeclLoc;
Located.Definition = DefLoc;

SymbolQualitySignals Quality;
Quality.merge(Sym);
SymbolRelevanceSignals Relevance;
Expand Down
74 changes: 46 additions & 28 deletions clang-tools-extra/clangd/unittests/XRefsTests.cpp
View file
Expand Up @@ -41,6 +41,7 @@ using ::testing::ElementsAre;
using ::testing::Eq;
using ::testing::IsEmpty;
using ::testing::Matcher;
using ::testing::UnorderedElementsAre;
using ::testing::UnorderedElementsAreArray;

MATCHER_P2(FileRange, File, Range, "") {
Expand Down Expand Up @@ -264,19 +265,23 @@ MATCHER_P3(Sym, Name, Decl, DefOrNone, "") {
<< llvm::to_string(arg.PreferredDeclaration);
return false;
}
if (!Def && !arg.Definition)
return true;
if (Def && !arg.Definition) {
*result_listener << "Has no definition";
return false;
}
if (Def && arg.Definition->range != *Def) {
if (!Def && arg.Definition) {
*result_listener << "Definition is " << llvm::to_string(arg.Definition);
return false;
}
if (arg.Definition->range != *Def) {
*result_listener << "Definition is " << llvm::to_string(arg.Definition);
return false;
}
return true;
}
::testing::Matcher<LocatedSymbol> Sym(std::string Name, Range Decl) {
return Sym(Name, Decl, llvm::None);
}

MATCHER_P(Sym, Name, "") { return arg.Name == Name; }

MATCHER_P(RangeIs, R, "") { return arg.range == R; }
Expand Down Expand Up @@ -771,7 +776,7 @@ TEST(LocateSymbol, TextualSmoke) {
auto AST = TU.build();
auto Index = TU.index();
EXPECT_THAT(locateSymbolAt(AST, T.point(), Index.get()),
ElementsAre(Sym("MyClass", T.range())));
ElementsAre(Sym("MyClass", T.range(), T.range())));
}

TEST(LocateSymbol, Textual) {
Expand Down Expand Up @@ -891,18 +896,20 @@ TEST(LocateSymbol, Ambiguous) {
// FIXME: Target the constructor as well.
EXPECT_THAT(locateSymbolAt(AST, T.point("9")), ElementsAre(Sym("Foo")));
EXPECT_THAT(locateSymbolAt(AST, T.point("10")),
ElementsAre(Sym("Foo", T.range("ConstructorLoc"))));
ElementsAre(Sym("Foo", T.range("ConstructorLoc"), llvm::None)));
EXPECT_THAT(locateSymbolAt(AST, T.point("11")),
ElementsAre(Sym("Foo", T.range("ConstructorLoc"))));
ElementsAre(Sym("Foo", T.range("ConstructorLoc"), llvm::None)));
// These assertions are unordered because the order comes from
// CXXRecordDecl::lookupDependentName() which doesn't appear to provide
// an order guarantee.
EXPECT_THAT(locateSymbolAt(AST, T.point("12")),
UnorderedElementsAre(Sym("bar", T.range("NonstaticOverload1")),
Sym("bar", T.range("NonstaticOverload2"))));
EXPECT_THAT(locateSymbolAt(AST, T.point("13")),
UnorderedElementsAre(Sym("baz", T.range("StaticOverload1")),
Sym("baz", T.range("StaticOverload2"))));
UnorderedElementsAre(
Sym("bar", T.range("NonstaticOverload1"), llvm::None),
Sym("bar", T.range("NonstaticOverload2"), llvm::None)));
EXPECT_THAT(
locateSymbolAt(AST, T.point("13")),
UnorderedElementsAre(Sym("baz", T.range("StaticOverload1"), llvm::None),
Sym("baz", T.range("StaticOverload2"), llvm::None)));
}

TEST(LocateSymbol, TextualDependent) {
Expand Down Expand Up @@ -932,9 +939,10 @@ TEST(LocateSymbol, TextualDependent) {
// interaction between locateASTReferent() and
// locateSymbolNamedTextuallyAt().
auto Results = locateSymbolAt(AST, Source.point(), Index.get());
EXPECT_THAT(Results, UnorderedElementsAre(
Sym("uniqueMethodName", Header.range("FooLoc")),
Sym("uniqueMethodName", Header.range("BarLoc"))));
EXPECT_THAT(Results,
UnorderedElementsAre(
Sym("uniqueMethodName", Header.range("FooLoc"), llvm::None),
Sym("uniqueMethodName", Header.range("BarLoc"), llvm::None)));
}

TEST(LocateSymbol, TemplateTypedefs) {
Expand Down Expand Up @@ -992,20 +1000,23 @@ int [[bar_not_preamble]];
auto Locations =
runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p1"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo", SourceAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo", SourceAnnotations.range(),
SourceAnnotations.range())));

// Go to a definition in header_in_preamble.h.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p2"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(
*Locations,
ElementsAre(Sym("bar_preamble", HeaderInPreambleAnnotations.range())));
ElementsAre(Sym("bar_preamble", HeaderInPreambleAnnotations.range(),
HeaderInPreambleAnnotations.range())));

// Go to a definition in header_not_in_preamble.h.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("p3"));
EXPECT_TRUE(bool(Locations)) << "findDefinitions returned an error";
EXPECT_THAT(*Locations,
ElementsAre(Sym("bar_not_preamble",
HeaderNotInPreambleAnnotations.range(),
HeaderNotInPreambleAnnotations.range())));
}

Expand Down Expand Up @@ -1039,21 +1050,25 @@ TEST(GoToInclude, All) {
// Test include in preamble.
auto Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point());
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));

// Test include in preamble, last char.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("2"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));

Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("3"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));

// Test include outside of preamble.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("6"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));

// Test a few positions that do not result in Locations.
Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("4"));
Expand All @@ -1062,11 +1077,13 @@ TEST(GoToInclude, All) {

Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("5"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));

Locations = runLocateSymbolAt(Server, FooCpp, SourceAnnotations.point("7"));
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));

// Objective C #import directive.
Annotations ObjC(R"objc(
Expand All @@ -1078,7 +1095,8 @@ TEST(GoToInclude, All) {
Server.addDocument(FooM, ObjC.code());
Locations = runLocateSymbolAt(Server, FooM, ObjC.point());
ASSERT_TRUE(bool(Locations)) << "locateSymbolAt returned an error";
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range())));
EXPECT_THAT(*Locations, ElementsAre(Sym("foo.h", HeaderAnnotations.range(),
HeaderAnnotations.range())));
}

TEST(LocateSymbol, WithPreamble) {
Expand All @@ -1103,7 +1121,7 @@ TEST(LocateSymbol, WithPreamble) {
// LocateSymbol goes to a #include file: the result comes from the preamble.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithHeader.point())),
ElementsAre(Sym("foo.h", FooHeader.range())));
ElementsAre(Sym("foo.h", FooHeader.range(), FooHeader.range())));

// Only preamble is built, and no AST is built in this request.
Server.addDocument(FooCpp, FooWithoutHeader.code(), "null",
Expand All @@ -1112,7 +1130,7 @@ TEST(LocateSymbol, WithPreamble) {
// stale one.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
ElementsAre(Sym("foo", FooWithoutHeader.range())));
ElementsAre(Sym("foo", FooWithoutHeader.range(), llvm::None)));

// Reset test environment.
runAddDocument(Server, FooCpp, FooWithHeader.code());
Expand All @@ -1122,7 +1140,7 @@ TEST(LocateSymbol, WithPreamble) {
// Use the AST being built in above request.
EXPECT_THAT(
cantFail(runLocateSymbolAt(Server, FooCpp, FooWithoutHeader.point())),
ElementsAre(Sym("foo", FooWithoutHeader.range())));
ElementsAre(Sym("foo", FooWithoutHeader.range(), llvm::None)));
}

TEST(LocateSymbol, NearbyTokenSmoke) {
Expand All @@ -1133,7 +1151,7 @@ TEST(LocateSymbol, NearbyTokenSmoke) {
auto AST = TestTU::withCode(T.code()).build();
// We don't pass an index, so can't hit index-based fallback.
EXPECT_THAT(locateSymbolAt(AST, T.point()),
ElementsAre(Sym("err", T.range())));
ElementsAre(Sym("err", T.range(), T.range())));
}

TEST(LocateSymbol, NearbyIdentifier) {
Expand Down

0 comments on commit 638f0cf

Please sign in to comment.