[clang][Index] Visit the default parameter arguements in libindex.

[hokein]

Summary

We are missing the default parmeter arguments when IndexFunctionLocals
is true.

Fixes https://github.com/clangd/clangd/issues/285.

[kadircet]

Oh god, this code is a mess, so many knobs, and apparently I made it worst in the past sorry for that :D

I totally understand if you want to make minimal changes to the code like this, but it would be great to simplify it a little bit without regressing too much, currently there is a knob for function locals and then another for parameters in declarations, then there is an implicit conditioning for function definitions.
I would say it would be sensible to only have IndexFunctionLocals as an option and do full indexing of parameters, both in declarations and definitions if it is present (assuming this doesn't result in real regressions in the client code), but it is also OK to leave it as it is, since the mess is beyond the scope of this patch.

But if you leave it as it is, I believe we'll still be missing some references to decls inside default arguments in clangd, for example:

a.h:

int var = 2;
void foo(int x = var);

a.cc

void foo(int x) {
  va^r = x;
}

clangd won't have the reference occuring in default arg inside the index, as preambles are not indexed with IndexParametersInDeclarations option turned on.

clang/lib/Index/IndexDecl.cpp

102

this should move into the else clause down below, otherwise it would end up being indexed twice

[hokein]

address comment

[hokein]

I totally understand if you want to make minimal changes to the code like this, but it would be great to simplify it a little bit without regressing too much, currently there is a knob for function locals and then another for parameters in declarations, then there is an implicit conditioning for function definitions.
I would say it would be sensible to only have IndexFunctionLocals as an option and do full indexing of parameters, both in declarations and definitions if it is present (assuming this doesn't result in real regressions in the client code), but it is also OK to leave it as it is, since the mess is beyond the scope of this patch.

removing the shouldIndexParametersInDeclarations seems good to me, looks like clangd is the only user of this flag. Regression is the concern -- as we are changing the behavior of libindex, and the libindex doesn't seem to have enough test coverage. I'd land this fix as it-is, and cleanup afterwards.

But if you leave it as it is, I believe we'll still be missing some references to decls inside default arguments in clangd, for example:

a.h:

int var = 2;
void foo(int x = var);
a.cc

void foo(int x) {

va^r = x;

}
clangd won't have the reference occuring in default arg inside the index, as preambles are not indexed with IndexParametersInDeclarations option turned on.

Are you talking about dynamic index? clangd doesn't record any refs in preamble by design -- as we skip function bodies in preamble

clang/lib/Index/IndexDecl.cpp

102

My reading to the source is that

• IndexCtx.handleDecl(Parm) just reports the Parm decl occurrence, it doesn't traversal the default arguments unfortunately :(
• the if (const ParmVarDecl *Parm = dyn_cast<ParmVarDecl>(D)) here is only triggered for ObjC cases, C++ never run into this code path :(

a conservative fix is that we only need the change on Line 117. I removed the change in the current if clause.

[hokein]

remove an accident change.

[hokein]

Looking the source code closely, we also have another code path of indexing the ParmVarDecl in clang/lib/Index/IndexTypeSourceInfo.cpp, it was called via IndexCtx.indexTypeSourceInfo in handleDeclarator.

In summary:

1. default arguments will be visited if this is a non-definition decl, regardless the shouldIndexFunctionLocatl -- this is done in IndexTypeSourceInfo
2. default arguments will be visited if this is a definition decl and shouldIndexFunctionLocal is fasle -- this is done in IndexDecl.
3. default arguments will not be visited if this is a definition decl and shouldIndexFunctionLocal is true

This patch fixes 3).

[kadircet]

LGTM,

just wondering though if you've tried the solution we discussed offline regarding updating indexTypeSourceInfo instead of this call site, could you mention the results in here?
So that the next soul that touches these lines has an idea about what's expecting them?

[hokein]

In D74610#1879054, @kadircet wrote:

LGTM,

just wondering though if you've tried the solution we discussed offline regarding updating indexTypeSourceInfo instead of this call site, could you mention the results in here?
So that the next soul that touches these lines has an idea about what's expecting them?

It seemed breaking a few existing tests...

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

const char *Tests[] = {
       )cpp",
 
       R"cpp(// Macros
         #define [[MA^CRO]](X) (X+1)
         void test() {
           int x = [[MACRO]]([[MACRO]](1));
         }
       )cpp",
+
+      R"cpp(
+        int [[v^ar]] = 0;
+        void foo(int s = [[var]]);
+      )cpp",
   };
   for (const char *Test : Tests) {
     Annotations T(Test);
     auto AST = TestTU::withCode(T.code()).build();
     std::vector<Matcher<Location>> ExpectedLocations;
     for (const auto &R : T.ranges())
       ExpectedLocations.push_back(RangeIs(R));
     EXPECT_THAT(findReferences(AST, T.point(), 0).References,

clang/lib/Index/IndexDecl.cpp

void handleDeclarator(const DeclaratorDecl *D,
                         const NamedDecl *Parent = nullptr,
                         bool isIBType = false) {
     if (!Parent) Parent = D;
 
     IndexCtx.indexTypeSourceInfo(D->getTypeSourceInfo(), Parent,
                                  Parent->getLexicalDeclContext(),
                                  /*isBase=*/false, isIBType);
     IndexCtx.indexNestedNameSpecifierLoc(D->getQualifierLoc(), Parent);
+    auto IndexDefaultParmeterArgument = [&](const ParmVarDecl *Parm,
+                                            const NamedDecl *Parent) {
+      if (Parm->hasDefaultArg() && !Parm->hasUninstantiatedDefaultArg() &&
+          !Parm->hasUnparsedDefaultArg())
+        IndexCtx.indexBody(Parm->getDefaultArg(), Parent);
+    };
     if (IndexCtx.shouldIndexFunctionLocalSymbols()) {
       if (const ParmVarDecl *Parm = dyn_cast<ParmVarDecl>(D)) {
         auto *DC = Parm->getDeclContext();
         if (auto *FD = dyn_cast<FunctionDecl>(DC)) {
           if (IndexCtx.shouldIndexParametersInDeclarations() ||
               FD->isThisDeclarationADefinition())
             IndexCtx.handleDecl(Parm);
         } else if (auto *MD = dyn_cast<ObjCMethodDecl>(DC)) {
           if (MD->isThisDeclarationADefinition())
             IndexCtx.handleDecl(Parm);
         } else {
           IndexCtx.handleDecl(Parm);
         }
       } else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
         if (IndexCtx.shouldIndexParametersInDeclarations() ||
             FD->isThisDeclarationADefinition()) {
-          for (auto PI : FD->parameters()) {
+          for (const auto *PI : FD->parameters()) {
+            IndexDefaultParmeterArgument(PI, D);
             IndexCtx.handleDecl(PI);
           }
         }
       }
     } else {
       // Index the default parameter value for function definitions.
       if (const auto *FD = dyn_cast<FunctionDecl>(D)) {
         if (FD->isThisDeclarationADefinition()) {
           for (const auto *PV : FD->parameters()) {
-            if (PV->hasDefaultArg() && !PV->hasUninstantiatedDefaultArg() &&
-                !PV->hasUnparsedDefaultArg())
-              IndexCtx.indexBody(PV->getDefaultArg(), D);
+            IndexDefaultParmeterArgument(PV, D);
           }
         }
       }
     }
   }
 
   bool handleObjCMethod(const ObjCMethodDecl *D,
                         const ObjCPropertyDecl *AssociatedProp = nullptr) {

clang/unittests/Index/IndexTests.cpp

EXPECT_THAT(Index->Symbols,
                                    AllOf(QName("Foo::f"),
                                          Kind(SymbolKind::InstanceMethod),
                                          WrittenAt(Position(4, 12))),
                                    AllOf(QName("Foo"), Kind(SymbolKind::Class),
                                          HasRole(SymbolRole::Reference),
                                          WrittenAt(Position(4, 14)))));
 }
 
+TEST(IndexTest, VisitDefaultArgs) {
+  std::string Code = R"cpp(
+    int var = 0;
+    void f(int s = var) {}
+  )cpp";
+  auto Index = std::make_shared<Indexer>();
+  IndexingOptions Opts;
+  Opts.IndexFunctionLocals = true;
+  Opts.IndexParametersInDeclarations = true;
+  tooling::runToolOnCode(std::make_unique<IndexAction>(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols,
+              Contains(AllOf(QName("var"), HasRole(SymbolRole::Reference),
+                             WrittenAt(Position(3, 20)))));
+}
+
 } // namespace
 } // namespace index
 } // namespace clang