[clangd] Fix a crash when indexing invalid ObjC method declaration

[adamcz]

Summary

This fix will make us not crash, but ideally we would handle this case
better.

[dgoldman]

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

1844–1848

It's hard to tell what Clang makes of this, I think instead we should test out the root cause of this crash, IIUC, are the legacy C style parameters:

@interface Foo
- (void)func:(bool)foo, bool bar;
@end

1853–1856

We should verify the method name is exactly as expected but comment we mostly care about not crashing.

clang/lib/Sema/SemaCodeComplete.cpp

3536

I think it might be simpler just to check the index:

unsigned NumSelectorArgs = Sel.getNumArgs();

P != PEnd && Idx < NumSelectorArgs

and note that we need to check this due to legacy use of C-style parameters in Objective-C method declarations which in practice only occurs due to typos

[adamcz]

addressed review comments

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

1844–1848

Oh yeah, thanks. Like I said, I don't know ObjC, so I was just randomly changing code until it crashed ;-)

1853–1856

Oops, that's a copy/paste leftover, sorry about that.

[dgoldman]

Looks good but I think for posterity we should add a similar test to clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp for the exact intended code complete behavior (or instead of SymbolCollectorTests.cpp - is there a particular reason you added a test there)?

[adamcz]

The crash happens while indexing the file, not during code completion. The code completion tests generally specify a cursor position, but in this case it is not relevant and could confuse the reader. This seemed like a logical place for indexing bug, in the spirit of using minimal repro case (i.e. you don't need to trigger code completion at all)

[dgoldman]

In D94919#2515987, @adamcz wrote:

The crash happens while indexing the file, not during code completion. The code completion tests generally specify a cursor position, but in this case it is not relevant and could confuse the reader. This seemed like a logical place for indexing bug, in the spirit of using minimal repro case (i.e. you don't need to trigger code completion at all)

I see, thanks.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp

TEST_F(SymbolCollectorTest, UndefOfModuleMacro) {
   TU.OverlayRealFileSystemForModules = true;
 
   TU.build();
   // We mostly care about not crashing, but verify that we didn't insert garbage
   // about X too.
   EXPECT_THAT(TU.headerSymbols(), Not(Contains(QName("X"))));
 }
 
+TEST_F(SymbolCollectorTest, NoCrashOnObjCMethodCStyleParam) {
+  auto TU = TestTU::withCode(R"objc(
+    @interface Foo
+    - (void)fun:(bool)foo, bool bar;
+    @end
+  )objc");
+  TU.ExtraArgs.push_back("-xobjective-c++");
+
+  TU.build();
+  // We mostly care about not crashing.
+  EXPECT_THAT(TU.headerSymbols(),
+              UnorderedElementsAre(QName("Foo"), QName("Foo::fun:")));
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang

clang/lib/Sema/SemaCodeComplete.cpp

else {
       Result.AddInformativeChunk(Result.getAllocator().CopyString(SelName));
 
       // If there is only one parameter, and we're past it, add an empty
       // typed-text chunk since there is nothing to type.
       if (Method->param_size() == 1)
         Result.AddTypedTextChunk("");
     }
     unsigned Idx = 0;
+    // The extra Idx < Sel.getNumArgs() check is needed due to legacy C-style
+    // method parameters.
     for (ObjCMethodDecl::param_const_iterator P = Method->param_begin(),
                                               PEnd = Method->param_end();
-         P != PEnd; (void)++P, ++Idx) {
+         P != PEnd && Idx < Sel.getNumArgs(); (void)++P, ++Idx) {
       if (Idx > 0) {
         std::string Keyword;
         if (Idx > StartParameter)
           Result.AddChunk(CodeCompletionString::CK_HorizontalSpace);
         if (IdentifierInfo *II = Sel.getIdentifierInfoForSlot(Idx))
           Keyword += II->getName();
         Keyword += ":";
         if (Idx < StartParameter || AllParametersAreInformative)