Fix crash due to ObjCPropertyDecl

[dgoldman]

Summary

With ObjCPropertyDecl, ASTNode.OrigD can be a ObjCPropertyImplDecl
which is not a NamedDecl, leading to a crash since the code
incorrectly assumes ASTNode.OrigD will always be a NamedDecl.

[ilya-biryukov]

Thanks for fixing this.
Quick LGTM to fix a crash, albeit a fews NITs.

clangd/index/SymbolCollector.cpp

350

Could you please leave a FIXME mentioning objc properties are not indexed properly here?

352

Maybe use llvm_dyn_cast and check for null? (To avoid double-cast)

[dgoldman]

• FIXME and dyn_cast

[ilya-biryukov]

@dgoldman, do you have commit access or should I land this for you?

clangd/index/SymbolCollector.cpp

353

NIT: use auto*, the type name is spelled in the RHS anyway.

[aaron.ballman]

Test case? Or does one already cover this crash?

[ilya-biryukov]

Good point, @aaron.ballman! @dgoldman, could you please add a test case?

[dgoldman]

• use auto

[dgoldman]

In D56916#1365357, @ilya-biryukov wrote:

Good point, @aaron.ballman! @dgoldman, could you please add a test case?

Unfortunately I can't get ninja check-clangd to build:

/Users/davg/dev/llvm/source/llvm/tools/clang/tools/extra/clang-tidy/bugprone/ParentVirtualCallCheck.cpp:54:27: error: no matching member function for call to 'getThisType'
        ActualMemberDecl->getThisType(ActualMemberDecl->getASTContext())
        ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
/Users/davg/dev/llvm/source/llvm/tools/clang/include/clang/AST/DeclCXX.h:2182:12: note: candidate function not viable: requires 0 arguments, but 1 was provided
  QualType getThisType() const;
           ^
/Users/davg/dev/llvm/source/llvm/tools/clang/include/clang/AST/DeclCXX.h:2184:19: note: candidate function not viable: requires 2 arguments, but 1 was provided
  static QualType getThisType(const FunctionProtoType *FPT,
                  ^
1 error generated.
[1230/1512] Building CXX object tools/clang/tools/extra/clang-tidy/cert/CMakeFiles/clangTidyCERTModule.dir/CERTTidyModule.cpp.o
ninja: build stopped: subcommand failed.

[ilya-biryukov]

Unfortunately I can't get ninja check-clangd to build:

Looks like clang-tools-extra uses an old revision. Rebasing after rL350916 should do the trick.

[dgoldman]

• FIXME and dyn_cast
• use auto
• Add test

[dgoldman]

In D56916#1366456, @ilya-biryukov wrote:

Unfortunately I can't get ninja check-clangd to build:

Looks like clang-tools-extra uses an old revision. Rebasing after rL350916 should do the trick.

Thanks, looks like my update script was skipping over that repo. Test case implemented. As I don't have commit access, can you land this for me?

[ilya-biryukov]

Done. I had to remove Container::_magic from the list of expeced symbols to make the test pass, let me know if it should actually be there.
Again, thanks for the fix!

[ilya-biryukov]

It looks like Container::_magic is a platform-dependent completion, I don't have it on Linux, but http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42665 fails because it's not in the list.
Submitted rL351943 to workaround the failure. @dgoldman, any idea why completion might be there on some platforms, but not the others?

[dgoldman]

In D56916#1367559, @ilya-biryukov wrote:

It looks like Container::_magic is a platform-dependent completion, I don't have it on Linux, but http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/42665 fails because it's not in the list.
Submitted rL351943 to workaround the failure. @dgoldman, any idea why completion might be there on some platforms, but not the others?

Perhaps property auto-synthesis isn't enabled there? I wonder if the following manual synthesis would help:

@implementation Container
@synthesize magic;
@end

clangd/index/SymbolCollector.cpp

bool SymbolCollector::handleDeclOccurence(
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
     return true;
 
   auto ID = getSymbolID(ND);
   if (!ID)
     return true;
 
-  const NamedDecl &OriginalDecl = *cast<NamedDecl>(ASTNode.OrigD);
+  // FIXME: ObjCPropertyDecl are not properly indexed here:
+  // - ObjCPropertyDecl may have an OrigD of ObjCPropertyImplDecl, which is
+  // not a NamedDecl.
+  auto *OriginalDecl = dyn_cast<NamedDecl>(ASTNode.OrigD);
+  if (!OriginalDecl)
+    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))
+  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);
+    BasicSymbol = addDeclaration(*OriginalDecl, std::move(*ID), IsMainFileOnly);
 
   if (Roles & static_cast<unsigned>(index::SymbolRole::Definition))
-    addDefinition(OriginalDecl, *BasicSymbol);
+    addDefinition(*OriginalDecl, *BasicSymbol);
   return true;
 }
 
 bool SymbolCollector::handleMacroOccurence(const IdentifierInfo *Name,
                                            const MacroInfo *MI,
                                            index::SymbolRoleSet Roles,
                                            SourceLocation Loc) {
   if (!Opts.CollectMacro)

unittests/clangd/SymbolCollectorTests.cpp

TEST_F(SymbolCollectorTest, ObjCSymbols) {
   runSymbolCollector(Header, /*Main=*/"", {"-fblocks", "-xobjective-c++"});
   EXPECT_THAT(Symbols,
               UnorderedElementsAre(
                   QName("Person"), QName("Person::someMethodName:lastName:"),
                   QName("MyCategory"), QName("Person::someMethodName2:"),
                   QName("MyProtocol"), QName("MyProtocol::someMethodName3:")));
 }
 
+TEST_F(SymbolCollectorTest, ObjCPropertyImpl) {
+  const std::string Header = R"(
+    @interface Container
+    @property(nonatomic) int magic;
+    @end
+
+    @implementation Container
+    @end
+  )";
+  TestFileName = testPath("test.m");
+  runSymbolCollector(Header, /*Main=*/"", {"-xobjective-c++"});
+  EXPECT_THAT(Symbols,
+              UnorderedElementsAre(
+                  QName("Container"), QName("Container::magic"),
+                  QName("Container::_magic")));
+}
+
 TEST_F(SymbolCollectorTest, Locations) {
   Annotations Header(R"cpp(
     // Declared in header, defined in main.
     extern int $xdecl[[X]];
     class $clsdecl[[Cls]];
     void $printdecl[[print]]();
 
     // Declared in header, defined nowhere.