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,12 +78,32 @@ return VD->getDefinition(); if (const auto *FD = dyn_cast(D)) return FD->getDefinition(); - if (const auto *PD = dyn_cast(D)) - return PD->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 (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 @@ -235,8 +255,8 @@ // declarations: // // - forward declaration(s): @class MyClass; -// - definition declaration: @interface MyClass ... @end -// - implementation: @implementation MyClass ... @end +// - 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 @@ -245,10 +265,13 @@ const NamedDecl *getPreferredDecl(const NamedDecl *D) { D = llvm::cast(D->getCanonicalDecl()); - // Prefer Objective-C class definitions over the forward declaration. + // 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; } @@ -287,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. @@ -316,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(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); } 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/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 @@ -551,12 +551,15 @@ @end )"); Annotations Main(R"( + @interface Dog () + @end @implementation $dogdef[[Dog]] @end @implementation $fluffydef[[Dog]] (Fluffy) @end )"); - runSymbolCollector(Header.code(), Main.code(), {"-xobjective-c++"}); + runSymbolCollector(Header.code(), Main.code(), + {"-xobjective-c++", "-Wno-objc-root-class"}); EXPECT_THAT(Symbols, UnorderedElementsAre( AllOf(QName("Dog"), DeclRange(Header.range("dogdecl")), @@ -565,6 +568,33 @@ 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 @@ -677,8 +677,8 @@ )cpp", R"objc( - @protocol $decl[[Dog]]; - @protocol $def[[Dog]] + @protocol Dog; + @protocol $decl[[Dog]] - (void)bark; @end id getDoggo() { @@ -689,7 +689,9 @@ R"objc( @interface Cat @end - @interface $decl[[Ca^t]] (Extension) + @implementation Cat + @end + @interface $decl[[Cat]] (Exte^nsion) - (void)meow; @end @implementation $def[[Cat]] (Extension) @@ -758,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 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 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 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 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.