dgoldman updated this revision to Diff 277601.
dgoldman added a comment.
Rebase
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D83501/new/
https://reviews.llvm.org/D83501
Files:
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/index/SymbolCollector.cpp
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -674,7 +674,57 @@
enum class E { [[A]], B };
E e = E::A^;
};
- )cpp"};
+ )cpp",
+
+ R"objc(
+ @protocol Dog;
+ @protocol $decl[[Dog]]
+ - (void)bark;
+ @end
+ id<Do^g> getDoggo() {
+ return 0;
+ }
+ )objc",
+
+ R"objc(
+ @interface Cat
+ @end
+ @implementation Cat
+ @end
+ @interface $decl[[Cat]] (Exte^nsion)
+ - (void)meow;
+ @end
+ @implementation $def[[Cat]] (Extension)
+ - (void)meow {}
+ @end
+ )objc",
+
+ R"objc(
+ @class $decl[[Foo]];
+ Fo^o * getFoo() {
+ return 0;
+ }
+ )objc",
+
+ R"objc(
+ @class Foo;
+ @interface $decl[[Foo]]
+ @end
+ Fo^o * getFoo() {
+ return 0;
+ }
+ )objc",
+
+ R"objc(
+ @class Foo;
+ @interface $decl[[Foo]]
+ @end
+ @implementation $def[[Foo]]
+ @end
+ Fo^o * getFoo() {
+ return 0;
+ }
+ )objc"};
for (const char *Test : Tests) {
Annotations T(Test);
llvm::Optional<Range> WantDecl;
@@ -692,6 +742,7 @@
// FIXME: Auto-completion in a template requires disabling delayed template
// parsing.
TU.ExtraArgs.push_back("-fno-delayed-template-parsing");
+ TU.ExtraArgs.push_back("-xobjective-c++");
auto AST = TU.build();
auto Results = locateSymbolAt(AST, T.point());
@@ -709,6 +760,131 @@
}
}
+TEST(LocateSymbol, AllMulti) {
+ // Ranges in tests:
+ // $declN is the declaration location
+ // $defN is the definition location (if absent, symbol has no definition)
+ //
+ // NOTE:
+ // N starts at 0.
+ // Due to limitations in clang's Annotations, you can't annotate the same
+ // text with multiple ranges, e.g. `$def0$def1[[Foo]]` is invalid.
+ struct ExpectedRanges {
+ Range WantDecl;
+ llvm::Optional<Range> WantDef;
+ };
+ const char *Tests[] = {
+ R"objc(
+ @interface $decl0[[Cat]]
+ @end
+ @implementation $def0[[Cat]]
+ @end
+ @interface $decl1[[Ca^t]] (Extension)
+ - (void)meow;
+ @end
+ @implementation $def1[[Cat]] (Extension)
+ - (void)meow {}
+ @end
+ )objc",
+
+ R"objc(
+ @interface $decl0[[Cat]]
+ @end
+ @implementation $def0[[Cat]]
+ @end
+ @interface $decl1[[Cat]] (Extension)
+ - (void)meow;
+ @end
+ @implementation $def1[[Ca^t]] (Extension)
+ - (void)meow {}
+ @end
+ )objc",
+ };
+ for (const char *Test : Tests) {
+ Annotations T(Test);
+ std::vector<ExpectedRanges> Ranges;
+ for (int i = 0; true; i++) {
+ bool HasDecl = !T.ranges("decl" + std::to_string(i)).empty();
+ bool HasDef = !T.ranges("def" + std::to_string(i)).empty();
+ if (!HasDecl && !HasDef)
+ break;
+ ExpectedRanges Range;
+ if (HasDecl)
+ Range.WantDecl = T.range("decl" + std::to_string(i));
+ if (HasDef)
+ Range.WantDef = T.range("def" + std::to_string(i));
+ Ranges.push_back(Range);
+ }
+
+ TestTU TU;
+ TU.Code = std::string(T.code());
+ TU.ExtraArgs.push_back("-xobjective-c++");
+
+ auto AST = TU.build();
+ auto Results = locateSymbolAt(AST, T.point());
+
+ ASSERT_THAT(Results, ::testing::SizeIs(Ranges.size())) << Test;
+ for (size_t i = 0; i < Ranges.size(); i++) {
+ EXPECT_EQ(Results[i].PreferredDeclaration.range, Ranges[i].WantDecl)
+ << "(i = " << i << ")" << Test;
+ llvm::Optional<Range> GotDef;
+ if (Results[i].Definition)
+ GotDef = Results[i].Definition->range;
+ EXPECT_EQ(GotDef, Ranges[i].WantDef) << "(i = " << i << ")" << Test;
+ }
+ }
+}
+
+TEST(LocateSymbol, MultipleDeclsWithSameDefinition) {
+ // Ranges in tests:
+ // $def is the shared definition location
+ // $declN is one of the declaration locations
+ //
+ // NOTE:
+ // N starts at 0.
+ const char *Tests[] = {
+ R"objc(
+ @interface $decl0[[Cat]]
+ @end
+ @interface $decl1[[Ca^t]] ()
+ - (void)meow;
+ @end
+ @implementation $def[[Cat]]
+ - (void)meow {}
+ @end
+ )objc",
+ };
+ for (const char *Test : Tests) {
+ Annotations T(Test);
+ std::vector<Range> DeclRanges;
+ Range WantDef;
+ for (int i = 0; true; i++) {
+ bool HasDecl = !T.ranges("decl" + std::to_string(i)).empty();
+ if (!HasDecl)
+ break;
+ DeclRanges.push_back(T.range("decl" + std::to_string(i)));
+ }
+ WantDef = T.range("def");
+
+ TestTU TU;
+ TU.Code = std::string(T.code());
+ TU.ExtraArgs.push_back("-xobjective-c++");
+
+ auto AST = TU.build();
+ auto Results = locateSymbolAt(AST, T.point());
+
+ ASSERT_THAT(Results, ::testing::SizeIs(DeclRanges.size())) << Test;
+ for (size_t i = 0; i < DeclRanges.size(); i++) {
+ EXPECT_EQ(Results[i].PreferredDeclaration.range, DeclRanges[i])
+ << "(i = " << i << ")" << Test;
+ llvm::Optional<Range> GotDef;
+ if (Results[i].Definition)
+ GotDef = Results[i].Definition->range;
+ EXPECT_EQ(GotDef, WantDef) << "(i = " << i << ")" << Test;
+ }
+ }
+}
+
// LocateSymbol test cases that produce warnings.
// These are separated out from All so that in All we can assert
// that there are no diagnostics.
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -542,6 +542,59 @@
// Figure out why it's platform-dependent.
}
+TEST_F(SymbolCollectorTest, ObjCLocations) {
+ Annotations Header(R"(
+ // Declared in header, defined in main.
+ @interface $dogdecl[[Dog]]
+ @end
+ @interface $fluffydecl[[Dog]] (Fluffy)
+ @end
+ )");
+ Annotations Main(R"(
+ @interface Dog ()
+ @end
+ @implementation $dogdef[[Dog]]
+ @end
+ @implementation $fluffydef[[Dog]] (Fluffy)
+ @end
+ )");
+ runSymbolCollector(Header.code(), Main.code(),
+ {"-xobjective-c++", "-Wno-objc-root-class"});
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("Dog"), DeclRange(Header.range("dogdecl")),
+ DefRange(Main.range("dogdef"))),
+ AllOf(QName("Fluffy"), DeclRange(Header.range("fluffydecl")),
+ DefRange(Main.range("fluffydef")))));
+}
+
+TEST_F(SymbolCollectorTest, ObjCForwardDecls) {
+ Annotations Header(R"(
+ // Forward declared in header, declared and defined in main.
+ @protocol Barker;
+ @class Dog;
+ )");
+ Annotations Main(R"(
+ @protocol $barkerdecl[[Barker]]
+ - (void)woof;
+ @end
+ @interface $dogdecl[[Dog]]<Barker>
+ - (void)woof;
+ @end
+ @implementation $dogdef[[Dog]]
+ - (void)woof {}
+ @end
+ )");
+ runSymbolCollector(Header.code(), Main.code(),
+ {"-xobjective-c++", "-Wno-objc-root-class"});
+ EXPECT_THAT(Symbols,
+ UnorderedElementsAre(
+ AllOf(QName("Dog"), DeclRange(Main.range("dogdecl")),
+ DefRange(Main.range("dogdef"))),
+ AllOf(QName("Barker"), DeclRange(Main.range("barkerdecl"))),
+ QName("Barker::woof"), QName("Dog::woof")));
+}
+
TEST_F(SymbolCollectorTest, Locations) {
Annotations Header(R"cpp(
// Declared in header, defined in main.
Index: clang-tools-extra/clangd/unittests/FindTargetTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -608,6 +608,25 @@
)cpp";
EXPECT_DECLS("ObjCInterfaceTypeLoc", "@interface Foo");
+ Code = R"cpp(
+ @interface Foo
+ @end
+ @implementation [[Foo]]
+ @end
+ )cpp";
+ EXPECT_DECLS("ObjCImplementationDecl", {"@interface Foo", Rel::Underlying});
+
+ Code = R"cpp(
+ @interface Foo
+ @end
+ @interface Foo (Ext)
+ @end
+ @implementation [[Foo]] (Ext)
+ @end
+ )cpp";
+ EXPECT_DECLS("ObjCCategoryImplDecl",
+ {"@interface Foo(Ext)", Rel::Underlying});
+
Code = R"cpp(
@protocol Foo
@end
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===================================================================
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -20,6 +20,7 @@
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclObjC.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
@@ -156,16 +157,48 @@
return Result;
}
-// Checks whether \p ND is a definition of a TagDecl (class/struct/enum/union)
-// in a header file, in which case clangd would prefer to use ND as a canonical
-// declaration.
+// Checks whether \p ND is a preferred declaration to clangd. Typically this
+// means preferring true declarations over forward declarations.
+//
+// For C++:
+// If \p ND is a definition of a TagDecl (class/struct/enum/union) in a header,
+// clangd would prefer to use ND as a canonical declaration.
// FIXME: handle symbol types that are not TagDecl (e.g. functions), if using
// the first seen declaration as canonical declaration is not a good enough
// heuristic.
+//
+// For Objective-C:
+// We prefer true class/protocol delarations over forward declarations.
bool isPreferredDeclaration(const NamedDecl &ND, index::SymbolRoleSet Roles) {
const auto &SM = ND.getASTContext().getSourceManager();
- return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
- isa<TagDecl>(&ND) && !isInsideMainFile(ND.getLocation(), SM);
+ if (isa<TagDecl>(ND))
+ return (Roles & static_cast<unsigned>(index::SymbolRole::Definition)) &&
+ !isInsideMainFile(ND.getLocation(), SM);
+ if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(&ND))
+ return ID->isThisDeclarationADefinition();
+ if (const auto *PD = dyn_cast<ObjCProtocolDecl>(&ND))
+ return PD->isThisDeclarationADefinition();
+ return false;
+}
+
+// Avoid recording certain NamedDecls as a declaration.
+//
+// For example, Objective-C classes can have three types of
+// declarations, all with the same USR:
+//
+// - forward declaration(s): @class MyClass;
+// - true declaration (interface definition): @interface MyClass ... @end
+// - true definition (implementation): @implementation MyClass ... @end
+//
+// We want the true declaration to be considered the canonical declaration and
+// the implementation to be considered the definition. However, we must avoid
+// recording the implementation as a declaration as well since it may override
+// the true declaration. The same is true for Objective-C categories although
+// they lack forward declarations.
+bool shouldRecordDeclaration(const NamedDecl &ND) {
+ if (isa<ObjCImplDecl>(ND))
+ return false;
+ return true;
}
RefKind toRefKind(index::SymbolRoleSet Roles, bool Spelled = false) {
@@ -330,14 +363,18 @@
return true;
const Symbol *BasicSymbol = Symbols.find(*ID);
- if (!BasicSymbol) // Regardless of role, ND is the canonical declaration.
- BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
- else if (isPreferredDeclaration(*OriginalDecl, Roles))
- // If OriginalDecl is preferred, replace the existing canonical
- // declaration (e.g. a class forward declaration). There should be at most
- // one duplicate as we expect to see only one preferred declaration per
- // TU, because in practice they are definitions.
- BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
+
+ // If OriginalDecl is preferred, create or replace the existing canonical
+ // declaration (e.g. a class forward declaration). There should be at most
+ // one duplicate as we expect to see only one preferred declaration per
+ // TU, because in practice they are definitions.
+ if (shouldRecordDeclaration(*ND)) {
+ if (isPreferredDeclaration(*OriginalDecl, Roles))
+ BasicSymbol =
+ addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
+ else
+ BasicSymbol = addDeclaration(*ND, std::move(*ID), IsMainFileOnly);
+ }
if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
addDefinition(*OriginalDecl, *BasicSymbol);
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -78,6 +78,32 @@
return VD->getDefinition();
if (const auto *FD = dyn_cast<FunctionDecl>(D))
return FD->getDefinition();
+ // Objective-C classes can have three types of declarations:
+ //
+ // - forward declaration: @class MyClass;
+ // - true declaration (interface definition): @interface MyClass ... @end
+ // - true definition (implementation): @implementation MyClass ... @end
+ //
+ // Objective-C categories are extensions are on classes:
+ //
+ // - declaration: @interface MyClass (Ext) ... @end
+ // - definition: @implementation MyClass (Ext) ... @end
+ //
+ // With one special case, a class extension, which is normally used to keep
+ // some declarations internal to a file without exposing them in a header.
+ //
+ // - class extension declaration: @interface MyClass () ... @end
+ // - which really links to class definition: @implementation MyClass ... @end
+ if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
+ return ID->getImplementation();
+ if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D)) {
+ if (CD->IsClassExtension()) {
+ if (const auto *ID = CD->getClassInterface())
+ return ID->getImplementation();
+ return nullptr;
+ }
+ return CD->getImplementation();
+ }
// Only a single declaration is allowed.
if (isa<ValueDecl>(D) || isa<TemplateTypeParmDecl>(D) ||
isa<TemplateTemplateParmDecl>(D)) // except cases above
@@ -223,6 +249,33 @@
return llvm::None;
}
+// A wrapper around `Decl::getCanonicalDecl` to support cases where Clang's
+// definition of a canonical declaration doesn't match up to what a programmer
+// would expect. For example, Objective-C classes can have three types of
+// declarations:
+//
+// - forward declaration(s): @class MyClass;
+// - true declaration (interface definition): @interface MyClass ... @end
+// - true definition (implementation): @implementation MyClass ... @end
+//
+// Clang will consider the forward declaration to be the canonical declaration
+// because it is first. We actually want the class definition if it is
+// available since that is what a programmer would consider the primary
+// declaration to be.
+const NamedDecl *getPreferredDecl(const NamedDecl *D) {
+ D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
+
+ // Prefer Objective-C class/protocol definitions over the forward declaration.
+ if (const auto *ID = dyn_cast<ObjCInterfaceDecl>(D))
+ if (const auto *DefinitionID = ID->getDefinition())
+ return DefinitionID;
+ if (const auto *PD = dyn_cast<ObjCProtocolDecl>(D))
+ if (const auto *DefinitionID = PD->getDefinition())
+ return DefinitionID;
+
+ return D;
+}
+
// Decls are more complicated.
// The AST contains at least a declaration, maybe a definition.
// These are up-to-date, and so generally preferred over index results.
@@ -238,7 +291,7 @@
llvm::DenseMap<SymbolID, size_t> ResultIndex;
auto AddResultDecl = [&](const NamedDecl *D) {
- D = llvm::cast<NamedDecl>(D->getCanonicalDecl());
+ D = getPreferredDecl(D);
auto Loc =
makeLocation(AST.getASTContext(), nameLocation(*D, SM), MainFilePath);
if (!Loc)
@@ -257,8 +310,8 @@
};
// Emit all symbol locations (declaration or definition) from AST.
- DeclRelationSet Relations =
- DeclRelation::TemplatePattern | DeclRelation::Alias;
+ DeclRelationSet Relations = DeclRelation::TemplatePattern |
+ DeclRelation::Alias | DeclRelation::Underlying;
for (const NamedDecl *D :
getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
// Special case: void foo() ^override: jump to the overridden method.
@@ -286,6 +339,19 @@
}
}
+ // Special case: if the class name is selected, also map Objective-C
+ // categories and category implementations back to their class interface.
+ //
+ // Since `TouchedIdentifier` might refer to the `ObjCCategoryImplDecl`
+ // instead of the `ObjCCategoryDecl` we intentionally check the contents
+ // of the locs when checking for class name equivalence.
+ if (auto *CD = dyn_cast<ObjCCategoryDecl>(D))
+ if (auto *ID = CD->getClassInterface())
+ if (TouchedIdentifier &&
+ (CD->getLocation() == TouchedIdentifier->location() ||
+ ID->getName() == TouchedIdentifier->text(SM)))
+ AddResultDecl(ID);
+
// Otherwise the target declaration is the right one.
AddResultDecl(D);
}
Index: clang-tools-extra/clangd/FindTarget.cpp
===================================================================
--- clang-tools-extra/clangd/FindTarget.cpp
+++ clang-tools-extra/clangd/FindTarget.cpp
@@ -302,7 +302,20 @@
// Record the underlying decl instead, if allowed.
D = USD->getTargetDecl();
Flags |= Rel::Underlying; // continue with the underlying decl.
+ } else if (const ObjCImplementationDecl *IID =
+ dyn_cast<ObjCImplementationDecl>(D)) {
+ // Objective-C implementation should map back to its interface.
+ D = IID->getClassInterface();
+ Flags |= Rel::Underlying;
+ } else if (const ObjCCategoryImplDecl *CID =
+ dyn_cast<ObjCCategoryImplDecl>(D)) {
+ // Objective-C category implementation should map back to its category
+ // declaration.
+ D = CID->getCategoryDecl();
+ Flags |= Rel::Underlying;
}
+ if (!D)
+ return;
if (const Decl *Pat = getTemplatePattern(D)) {
assert(Pat != D);
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
