diff --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp --- a/clang-tools-extra/clangd/FindTarget.cpp +++ b/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(D)) { + // Objective-C implementation should map back to its interface. + D = IID->getClassInterface(); + Flags |= Rel::Underlying; + } else if (const ObjCCategoryImplDecl *CID = + dyn_cast(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); diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -78,6 +78,32 @@ return VD->getDefinition(); if (const auto *FD = dyn_cast(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(D)) + return ID->getImplementation(); + if (const auto *CD = dyn_cast(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(D) || isa(D) || isa(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(D->getCanonicalDecl()); + + // Prefer Objective-C class/protocol definitions over the forward declaration. + if (const auto *ID = dyn_cast(D)) + if (const auto *DefinitionID = ID->getDefinition()) + return DefinitionID; + if (const auto *PD = dyn_cast(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 ResultIndex; auto AddResultDecl = [&](const NamedDecl *D) { - D = llvm::cast(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 (const auto *CD = dyn_cast(D)) + if (const 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); } diff --git a/clang-tools-extra/clangd/index/SymbolCollector.cpp b/clang-tools-extra/clangd/index/SymbolCollector.cpp --- a/clang-tools-extra/clangd/index/SymbolCollector.cpp +++ b/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(index::SymbolRole::Definition)) && - isa(&ND) && !isInsideMainFile(ND.getLocation(), SM); + if (isa(ND)) + return (Roles & static_cast(index::SymbolRole::Definition)) && + !isInsideMainFile(ND.getLocation(), SM); + if (const auto *ID = dyn_cast(&ND)) + return ID->isThisDeclarationADefinition(); + if (const auto *PD = dyn_cast(&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(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(index::SymbolRole::Definition)) addDefinition(*OriginalDecl, *BasicSymbol); diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp --- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp +++ b/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 diff --git a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp b/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp --- a/clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp +++ b/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]] + - (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. diff --git a/clang-tools-extra/clangd/unittests/XRefsTests.cpp b/clang-tools-extra/clangd/unittests/XRefsTests.cpp --- a/clang-tools-extra/clangd/unittests/XRefsTests.cpp +++ b/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 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 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 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 Ranges; + for (int Idx = 0; true; Idx++) { + bool HasDecl = !T.ranges("decl" + std::to_string(Idx)).empty(); + bool HasDef = !T.ranges("def" + std::to_string(Idx)).empty(); + if (!HasDecl && !HasDef) + break; + ExpectedRanges Range; + if (HasDecl) + Range.WantDecl = T.range("decl" + std::to_string(Idx)); + if (HasDef) + Range.WantDef = T.range("def" + std::to_string(Idx)); + 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 Idx = 0; Idx < Ranges.size(); Idx++) { + EXPECT_EQ(Results[Idx].PreferredDeclaration.range, Ranges[Idx].WantDecl) + << "($decl" << Idx << ")" << Test; + llvm::Optional GotDef; + if (Results[Idx].Definition) + GotDef = Results[Idx].Definition->range; + EXPECT_EQ(GotDef, Ranges[Idx].WantDef) << "($def" << Idx << ")" << 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 DeclRanges; + Range WantDef; + for (int Idx = 0; true; Idx++) { + bool HasDecl = !T.ranges("decl" + std::to_string(Idx)).empty(); + if (!HasDecl) + break; + DeclRanges.push_back(T.range("decl" + std::to_string(Idx))); + } + 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 Idx = 0; Idx < DeclRanges.size(); Idx++) { + EXPECT_EQ(Results[Idx].PreferredDeclaration.range, DeclRanges[Idx]) + << "($decl" << Idx << ")" << Test; + llvm::Optional GotDef; + if (Results[Idx].Definition) + GotDef = Results[Idx].Definition->range; + EXPECT_EQ(GotDef, WantDef) << "(def for $decl" << Idx << ")" << 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.