feat: Emit defs & refs for functions & methods (#122)

Author: varungandhi-src

docs/Design.md

@@ -566,6 +566,7 @@ There are 4 possible cases here:
    to the corresponding synthetic standard library package
    (e.g. `[email protected]`).
 
+<!-- TODO(def: handle-forward-decls) -->
 For simplicity, an initial implementation
 can put fake `SymbolInformation` values
 in the `external_symbols` list

indexer/Indexer.cc

@@ -297,6 +297,49 @@ void TuIndexer::saveEnumTypeLoc(const clang::EnumTypeLoc &enumTypeLoc) {
   this->saveTagTypeLoc(enumTypeLoc);
 }
 
+void TuIndexer::saveFunctionDecl(const clang::FunctionDecl &functionDecl) {
+  auto optSymbol = this->symbolFormatter.getFunctionSymbol(functionDecl);
+  if (!optSymbol.has_value()) {
+    return;
+  }
+  std::string_view symbol = optSymbol.value();
+
+  if (functionDecl.isPure() || functionDecl.isThisDeclarationADefinition()) {
+    scip::SymbolInformation symbolInfo{};
+    for (auto &docComment : this->tryGetDocComment(functionDecl)) {
+      *symbolInfo.add_documentation() = std::move(docComment.Text);
+    }
+    if (auto *cxxMethodDecl =
+            llvm::dyn_cast<clang::CXXMethodDecl>(&functionDecl)) {
+      for (auto &overridenMethodDecl : cxxMethodDecl->overridden_methods()) {
+        if (auto optOverridenSymbol =
+                this->symbolFormatter.getFunctionSymbol(*overridenMethodDecl)) {
+          scip::Relationship rel{};
+          rel.set_symbol(optOverridenSymbol->data(),
+                         optOverridenSymbol->size());
+          rel.set_is_implementation(true);
+          rel.set_is_reference(true);
+          *symbolInfo.add_relationships() = std::move(rel);
+        }
+      }
+    }
+    // FIXME(issue: https://github.com/sourcegraph/scip-clang/issues/123)
+    // Kythe uses DeclarationNameInfo::getSourceRange(), which seems like
+    // the right API for getting the right range for multi-token names like
+    // 'operator<<' etc., but when I tried using that here, it returned
+    // incorrect results for every kind of name.
+    //
+    // Specifically, for simple identifiers, it would return a blank range,
+    // and for 'operator<<', it would exclude the range of '<<'.
+    // So just rely on the single token implementation for now.
+    this->saveDefinition(symbol, functionDecl.getLocation(),
+                         std::move(symbolInfo));
+  } else {
+    // See TODO(ref: handle-forward-decls)
+    this->saveReference(symbol, functionDecl.getLocation());
+  }
+}
+
 void TuIndexer::saveNamespaceDecl(const clang::NamespaceDecl &namespaceDecl) {
   auto optSymbol = this->symbolFormatter.getNamespaceSymbol(namespaceDecl);
   if (!optSymbol.has_value()) {
@@ -420,9 +463,10 @@ void TuIndexer::saveTagDecl(const clang::TagDecl &tagDecl) {
   auto symbol = optSymbol.value();
 
   if (!tagDecl.isThisDeclarationADefinition()) {
-    // Forward declarations should be marked as references. In the future,
-    // we should add an extra role to SCIP for forward declarations,
-    // once https://github.com/sourcegraph/scip/issues/131 is addressed.
+    // 1. We should emit an external symbol here in some situations
+    //    See TODO(ref: handle-forward-decls)
+    // 2. Pass in a forward declaration SymbolRole here
+    //    once https://github.com/sourcegraph/scip/issues/131 is fixed.
     this->saveReference(symbol, tagDecl.getLocation());
     return;
   }
@@ -496,6 +540,25 @@ void TuIndexer::saveDeclRefExpr(const clang::DeclRefExpr &declRefExpr) {
   // ^ TODO: Add read-write access to the symbol role here
 }
 
+void TuIndexer::saveMemberExpr(const clang::MemberExpr &memberExpr) {
+  auto *namedDecl =
+      llvm::dyn_cast<clang::NamedDecl>(memberExpr.getMemberDecl());
+  if (!namedDecl) {
+    return;
+  }
+  auto optSymbol = this->symbolFormatter.getNamedDeclSymbol(*namedDecl);
+  if (!optSymbol.has_value()) {
+    return;
+  }
+  // FIXME(issue: https://github.com/sourcegraph/scip-clang/issues/123)
+  // The ideal API here is probably getMemberNameInfo().getSourceRange(),
+  // similar to the operator case, but that doesn't give the expected result.
+  if (!memberExpr.getMemberNameInfo().getName().isIdentifier()) {
+    return;
+  }
+  this->saveReference(optSymbol.value(), memberExpr.getMemberLoc());
+}
+
 void TuIndexer::emitDocumentOccurrencesAndSymbols(
     bool deterministic, clang::FileID fileId, scip::Document &scipDocument) {
   auto it = this->documentMap.find({fileId});

indexer/Indexer.h

@@ -21,6 +21,10 @@
 #include "indexer/ScipExtras.h"
 #include "indexer/SymbolFormatter.h"
 
+#define FOR_EACH_EXPR_TO_BE_INDEXED(F) \
+  F(DeclRef)                           \
+  F(Member)
+
 #define FOR_EACH_TYPE_TO_BE_INDEXED(F) \
   F(Enum)                              \
   F(Record)
@@ -30,13 +34,16 @@ namespace clang {
 FOR_EACH_DECL_TO_BE_INDEXED(FORWARD_DECLARE)
 #undef FORWARD_DECLARE
 
+#define FORWARD_DECLARE(ExprName) class ExprName##Expr;
+FOR_EACH_EXPR_TO_BE_INDEXED(FORWARD_DECLARE)
+#undef FORWARD_DECLARE
+
 #define FORWARD_DECLARE(TypeName) class TypeName##TypeLoc;
 FOR_EACH_TYPE_TO_BE_INDEXED(FORWARD_DECLARE)
 #undef FORWARD_DECLARE
 
 class ASTContext;
 class Decl;
-class DeclRefExpr;
 class MacroDefinition;
 class MacroInfo;
 class NestedNameSpecifierLoc;
@@ -213,7 +220,10 @@ class TuIndexer final {
   void saveTagDecl(const clang::TagDecl &);
   void saveTagTypeLoc(const clang::TagTypeLoc &);
 
-  void saveDeclRefExpr(const clang::DeclRefExpr &);
+#define SAVE_EXPR(ExprName) \
+  void save##ExprName##Expr(const clang::ExprName##Expr &);
+  FOR_EACH_EXPR_TO_BE_INDEXED(SAVE_EXPR)
+#undef SAVE_EXPR
   void saveNestedNameSpecifierLoc(const clang::NestedNameSpecifierLoc &);
 
 #define SAVE_TYPE_LOC(TypeName) \

indexer/SymbolFormatter.cc

@@ -371,23 +371,38 @@ SymbolFormatter::getEnumSymbol(const clang::EnumDecl &enumDecl) {
   return this->getTagSymbol(enumDecl);
 }
 
+std::optional<std::string_view>
+SymbolFormatter::getFunctionSymbol(const clang::FunctionDecl &functionDecl) {
+  return this->getSymbolCached(
+      functionDecl, [&]() -> std::optional<std::string> {
+        auto optContextSymbol =
+            this->getContextSymbol(*functionDecl.getDeclContext());
+        if (!optContextSymbol.has_value()) {
+          return {};
+        }
+        // See discussion in docs/Design.md for this choice of disambiguator.
+        auto typeString =
+            functionDecl.getType().getCanonicalType().getAsString();
+        auto name = functionDecl.getNameAsString();
+        return SymbolBuilder::formatContextual(
+            optContextSymbol.value(),
+            DescriptorBuilder{
+                .name = name,
+                .disambiguator = this->formatTemporary(
+                    "{:x}", HashValue::forText(typeString)),
+                .suffix = scip::Descriptor::Method,
+            });
+      });
+}
+
 std::optional<std::string_view>
 SymbolFormatter::getNamedDeclSymbol(const clang::NamedDecl &namedDecl) {
-  using Kind = clang::Decl::Kind;
-#define HANDLE(kind_)            \
-  case clang::Decl::Kind::kind_: \
-    return this->get##kind_##Symbol(llvm::cast<clang::kind_##Decl>(namedDecl));
-  switch (namedDecl.getKind()) {
-    FOR_EACH_DECL_TO_BE_INDEXED(HANDLE)
-  case Kind::VarTemplateSpecialization:
-  case Kind::VarTemplatePartialSpecialization:
-  case Kind::OMPCapturedExpr:
-  case Kind::Decomposition:
-  case Kind::ParmVar:
-    return this->getVarSymbol(*llvm::dyn_cast<clang::VarDecl>(&namedDecl));
-  default:
-    return {};
+#define HANDLE(kind_)                                                \
+  if (auto *decl = llvm::dyn_cast<clang::kind_##Decl>(&namedDecl)) { \
+    return this->get##kind_##Symbol(*decl);                          \
   }
+  FOR_EACH_DECL_TO_BE_INDEXED(HANDLE)
+  return {};
 }
 
 /// Returns nullopt for anonymous namespaces in files for which

indexer/SymbolFormatter.h

@@ -20,6 +20,7 @@
   F(Binding)                           \
   F(EnumConstant)                      \
   F(Enum)                              \
+  F(Function)                          \
   F(Namespace)                         \
   F(Record)                            \
   F(Var)

indexer/Worker.cc

@@ -705,10 +705,14 @@ class IndexerAstVisitor : public clang::RecursiveASTVisitor<IndexerAstVisitor> {
   FOR_EACH_DECL_TO_BE_INDEXED(VISIT_DECL)
 #undef VISIT_DECL
 
-  bool VisitDeclRefExpr(clang::DeclRefExpr *declRefExpr) {
-    this->tuIndexer.saveDeclRefExpr(*declRefExpr);
-    return true;
-  }
+#define VISIT_EXPR(ExprName)                                     \
+  bool Visit##ExprName##Expr(clang::ExprName##Expr *expr) {      \
+    ENFORCE(expr, "expected " #ExprName "Expr to be null-null"); \
+    this->tuIndexer.save##ExprName##Expr(*expr);                 \
+    return true;                                                 \
+  }
+  FOR_EACH_EXPR_TO_BE_INDEXED(VISIT_EXPR)
+#undef VISIT_EXPR
 
 #define VISIT_TYPE_LOC(TypeName)                                    \
   bool Visit##TypeName##TypeLoc(clang::TypeName##TypeLoc typeLoc) { \

test/Snapshot.cc

@@ -167,7 +167,9 @@ void formatSnapshot(const scip::Document &document,
       }
 
       ENFORCE(range.start.column < range.end.column,
-              "We shouldn't be emitting empty ranges 🙅");
+              "Found empty range for {} at {}:{}:{}", occ.symbol(),
+              sourceFilePath.asStringView(), range.start.line,
+              range.start.column);
 
       auto lineStart =
           absl::StrCat("//", std::string(range.start.column - 1, ' '));
@@ -293,13 +295,17 @@ void MultiTuSnapshotTest::run(SnapshotMode mode,
   absl::flat_hash_map<RootRelativePath, RootRelativePath> alreadyUsedFiles;
 
   for (auto &io : this->inputOutputs) {
-    auto &sourceFilePath = io.sourceFilePath.asStringRef();
-    if (!test::isTuMainFilePath(sourceFilePath)) {
+    auto &sourceFileRelPath = io.sourceFilePath.asStringRef();
+    if (!test::isTuMainFilePath(sourceFileRelPath)) {
       continue;
     }
-    std::vector<std::string> commandLine{"clang", "-I", ".", sourceFilePath};
+    std::vector<std::string> commandLine{"clang", "-I", ".", sourceFileRelPath};
     {
-      std::ifstream tuStream(sourceFilePath, std::ios::in | std::ios::binary);
+      auto sourceFileAbsPath =
+          this->rootPath.makeAbsolute(io.sourceFilePath.asRef());
+      ENFORCE(std::filesystem::exists(sourceFileAbsPath.asStringRef()));
+      std::ifstream tuStream(sourceFileAbsPath.asStringRef(),
+                             std::ios::in | std::ios::binary);
       std::string prefix = "// extra-args: ";
       for (std::string line; std::getline(tuStream, line);) {
         if (!line.starts_with(prefix)) {
@@ -314,8 +320,8 @@ void MultiTuSnapshotTest::run(SnapshotMode mode,
       }
     }
 
-    auto output =
-        compute(rootPath, io.sourceFilePath.asRef(), std::move(commandLine));
+    auto output = compute(this->rootPath, io.sourceFilePath.asRef(),
+                          std::move(commandLine));
 
     for (auto &[filePath, _] : output) {
       ENFORCE(!alreadyUsedFiles.contains(filePath),

test/index/functions/ctors_dtors.cc

@@ -0,0 +1,54 @@
+// extra-args: -std=c++20
+
+template<class T> struct remove_ref      { typedef T type; };
+template<class T> struct remove_ref<T&>  { typedef T type; };
+template<class T> struct remove_ref<T&&> { typedef T type; };
+
+template <typename T>
+typename remove_ref<T>::type&& move(T&& arg) {
+  return static_cast<typename remove_ref<T>::type&&>(arg);
+}
+
+struct C {
+  int x;
+  int y;
+};
+
+struct D {
+  int x;
+  int y;
+
+  D() = default;
+  D(const D &) = default;
+  D(D &&) = default;
+  D &operator=(const D &) = default;
+  D &operator=(D &&) = default;
+};
+
+void test_ctors() {
+  C c0;
+  D d0;
+  C c1{};
+  D d1{};
+  C c2{0, 1};
+  // TODO: Figure out a minimal stub for std::initializer_list,
+  // which we can use here, without running into Clang's
+  // "cannot compile this weird std::initializer_list yet" error
+  // D d2{0, 1};
+  C c3{move(c1)};
+  D d3{move(d1)};
+
+  C c4 = {};
+  D d4 = {};
+  C c5 = C();
+  D d5 = D();
+  C c6 = {0, 1};
+  // Uncomment after adding initializer_list
+  // D d6 = {0, 1};
+
+  C c7 = {.x = 0};
+  C c8 = {.x = 0, .y = 1};
+  C c9 = C{0, 1};
+  C c10 = move(c1);
+  D d10 = move(d1);
+}