dgoldman updated this revision to Diff 277582.
dgoldman marked an inline comment as done.
dgoldman added a comment.
Fixes for ObjC in SymbolCollector as well as XRefs
- Make sure ObjC indexing doesn't use the forward decls and doesn't treat the
impl as the canonical decl
- Don't treat ObjC protocol forward declarations as true declarations
- Also AddDecl for ObjC interfaces when the interface name in a category is
selected
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/XRefs.cpp
clang-tools-extra/clangd/index/SymbolCollector.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
@@ -677,8 +677,8 @@
)cpp",
R"objc(
- @protocol $decl[[Dog]];
- @protocol $def[[Dog]]
+ @protocol Dog;
+ @protocol $decl[[Dog]]
- (void)bark;
@end
id<Do^g> 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<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
@@ -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]]<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/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,12 +78,32 @@
return VD->getDefinition();
if (const auto *FD = dyn_cast<FunctionDecl>(D))
return FD->getDefinition();
- if (const auto *PD = dyn_cast<ObjCProtocolDecl>(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<ObjCInterfaceDecl>(D))
return ID->getImplementation();
- if (const auto *CD = dyn_cast<ObjCCategoryDecl>(D))
+ 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
@@ -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<NamedDecl>(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<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;
}
@@ -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<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);
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
