[AST][clangd] Expose documentation of Attrs on hover.

[sammccall]

Summary

This adds a method to Attr to get at the documentation programmatically.

[aaron.ballman]

clang/utils/TableGen/ClangAttrEmitter.cpp

4220

FWIW, this will miss omp::sequence and omp::directive, but that's not the end of the world. May be worth a fixme comment in case we want to solve it someday.

4242

Hmmmm, I am not 100% certain this assertion is correct -- the user may have plugin attributes, which I believe are given a "kind" that's larger than the last-known builtin attribute kind.

[sammccall]

Change assertion to defensive check

clang/utils/TableGen/ClangAttrEmitter.cpp

4220

Well, those

• don't themselves have Attr nodes or AttrKinds, so the API couldn't query for them
• don't actually have any documentation as far as I can tell

So I'm not sure there's much to fix.

---

The ParsedAttr-kind vs Attr-kind distinction, and the fact that many attributes don't leave any AST nodes at all, mean this method doesn't seem "universal". But the goal is to show docs in clangd when hovering on an attribute (which we only support for those with AST nodes).

If we want to do something different (like show documenation while code-completing) we'll probably have to add a second ParsedAttrKind-based API.

4242

Thanks, changed it to an if() instead.
(I don't think it's particularly pressing to allow them to be documented)

[aaron.ballman]

This generally LGTM but the only other thing I'm wondering is whether there should be any clang-specific testing for this functionality. Right now, it relies on running the clangd unit tests to notice failures and that's not ideal because not everyone builds clang-tools-extra as part of their usual Clang development. However, given that this functionality isn't really *used* by Clang, I'm not certain there's much to test and there's not a whole lot of failure mode beyond "the file didn't get generated".

So giving this my LGTM, but if you think of a way to introduce clang-specific tests for this, I'm happy to do further review.

clang/utils/TableGen/ClangAttrEmitter.cpp

4220

So I'm not sure there's much to fix.

The big reason they're not documented is because I've not thought of a good way to expose the documentation via AttrDocs.td. Currently, that documentation needs the attributes to live in Attr.td, and we don't expose the attributes there because we have no way to express their arguments.

At some point in the future, I expect we'll make these two attributes "more normal" by getting them into Attr.td and AttrDocs.td, and at that point this probably fixes itself? So I think you're right, maybe not much to fix *here*.

4242

Agreed that we can figure out how to let plugin attributes add documentation at a later date.

[sammccall]

add unit test for getDocumentation

[sammccall]

In D107703#2937183, @aaron.ballman wrote:

This generally LGTM but the only other thing I'm wondering is whether there should be any clang-specific testing

Makes sense. Here the "clang feature" is just the API, so I've added a very basic unittest for that.

[aaron.ballman]

clang/unittests/AST/CMakeLists.txt

18

I don't think this new file got attached to the patch.

[sammccall]

Oops, forgot test file

[sammccall]

Oops, I'm just bad at git. Here's the test file!

[aaron.ballman]

LGTM, thanks!

[kadircet]

thanks, LG from my side as well, just a couple of clarification questions regarding tablegen.

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

2389

nit: maybe ASSERT_FALSE(HI.Documentation.empty()) ? (I actually think the test in AST/AttrTest is enough and we can drop this assertion).

clang/utils/TableGen/ClangAttrEmitter.cpp

4231

not sure if this comment will stay useful.

4237

i am not well-versed in tablegen, so sorry if i am being dense here, butt what happens to the attributes we skipped above (e.g. the ones without a ASTNode flag)?

Maybe we should be emitting a ... AttrDoc_X[] = ""; for all attributes, no matter what?

[aaron.ballman]

clang/unittests/AST/AttrTest.cpp

20

It seems this is failing the premerge CI -- I think you need a call to .str() in here to convert the StringRef to a std::string?

clang/utils/TableGen/ClangAttrEmitter.cpp

4237

The ones without an AST node don't get an attr::Kind enumeration generated for them, so I believe this interface is safe.

[sammccall]

clang/unittests/AST/AttrTest.cpp

20

Thanks - no idea why this only breaks MSVC, but gmock is a subtle beast...

clang/utils/TableGen/ClangAttrEmitter.cpp

4231

I want a comment to avoid a chesterton's fence:

• the motivation for doing something lazy is that this is really rare
• it's sensible to revisit this if it stops being rare

Reworded it to make this more explicit.

4237

Right, all those extra strings would just tickle -Wunused :-(

I'd love to have an API that can also be used for attrs with ASTNode=0 (many are documented!). However:

• for the motivating use case (hover), we can only trigger when we find an AST node anyway (sadly)
• the obvious alternative is AttributeCommonInfo::Kind, (i.e. ParsedAttr kind), which isn't actually better, just differently bad. e.g. there's one ParsedAttr kind for interrupt, but three target-specific Attr kinds, each of which is documented separately. For hover you want to get the right one, and ParsedAttr kind can't choose it.

[thakis]

The docs are in rST format. Is that the format LSP wants?

[thakis]

Also, I tweaked the tablegen bits a bit to be more like all the other tablegen bits in 189911203779e793cb7767ad233d9994a88c7ea3. Hope that's ok :)

[sammccall]

In D107703#2942853, @thakis wrote:

The docs are in rST format. Is that the format LSP wants?

Not really :-) LSP's favorite would be markdown.
But we're used to ingesting comments in unknown formats and trying to make sense of them, and our users are used to us failing. (Those heuristics could use some love).
So the rst will be thrown into that meatgrinder as if they're comments, and at the very least the text will survive, which is the main thing.

Hopefully one day we can do real conversion.

Edit: and thanks for fixing up the tablegen!

[RKSimon]

clang/utils/TableGen/ClangAttrEmitter.cpp

4231

coverity is complaining that the for loop will never execute more than once, would it be worth refactoring?

if (!Docs.empty) {
  const auto *D = Docs[0];
  ...
}

[sammccall]

clang/utils/TableGen/ClangAttrEmitter.cpp

4231

Sure, done in eabb1f0732ac5e20d2e169024befaf2e9f166a8d

clang-tools-extra/clangd/Hover.cpp

llvm::Optional<HoverInfo> getHoverContents(const Attr *A, ParsedAST &AST) {
   HoverInfo HI;
   HI.Name = A->getSpelling();
   if (A->hasScope())
     HI.LocalScope = A->getScopeName()->getName().str();
   {
     llvm::raw_string_ostream OS(HI.Definition);
     A->printPretty(OS, AST.getASTContext().getPrintingPolicy());
   }
-  // FIXME: attributes have documentation, can we get at that?
+  HI.Documentation = Attr::getDocumentation(A->getKind()).str();
   return HI;
 }
 
 bool isParagraphBreak(llvm::StringRef Rest) {
   return Rest.ltrim(" \t").startswith("\n");
 }
 
 bool punctuationIndicatesLineBreak(llvm::StringRef Line) {

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

 //===-- HoverTests.cpp ----------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "AST.h"
 #include "Annotations.h"
 #include "Hover.h"
 #include "TestIndex.h"
 #include "TestTU.h"
 #include "index/MemIndex.h"
+#include "clang/AST/Attr.h"
 #include "clang/Basic/Specifiers.h"
 #include "clang/Index/IndexSymbol.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/StringRef.h"
 
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <string>

        }},
       {R"cpp(
          void foo(int * __attribute__(([[non^null]], noescape)) );
          )cpp",
        [](HoverInfo &HI) {
          HI.Name = "nonnull";
          HI.Kind = index::SymbolKind::Unknown; // FIXME: no suitable value
          HI.Definition = "__attribute__((nonnull))";
-         HI.Documentation = ""; // FIXME
+         HI.Documentation = Attr::getDocumentation(attr::NonNull).str();
        }},
   };
 
   // Create a tiny index, so tests above can verify documentation is fetched.
   Symbol IndexSym = func("indexSymbol");
   IndexSym.Documentation = "comment from index";
   SymbolSlab::Builder Symbols;
   Symbols.insert(IndexSym);
   auto Index =

clang/include/clang/AST/Attr.h

public:
 
   // Clone this attribute.
   Attr *clone(ASTContext &C) const;
 
   bool isLateParsed() const { return IsLateParsed; }
 
   // Pretty print this attribute.
   void printPretty(raw_ostream &OS, const PrintingPolicy &Policy) const;
+
+  static StringRef getDocumentation(attr::Kind);
 };
 
 class TypeAttr : public Attr {
 protected:
   TypeAttr(ASTContext &Context, const AttributeCommonInfo &CommonInfo,
            attr::Kind AK, bool IsLateParsed)
       : Attr(Context, CommonInfo, AK, IsLateParsed) {}
 

clang/lib/AST/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BinaryFormat
   Core
   FrontendOpenMP
   Support
   )
 
 # FIXME: the entry points to the interpreter should be moved out of clangAST
 # into the parser or Sema in order to allow the interpreter to be moved to
 # another library which depends on clangAST.
 clang_tablegen(Opcodes.inc
   -gen-clang-opcodes
   SOURCE Interp/Opcodes.td
   TARGET Opcodes)
 
+clang_tablegen(AttrDocTable.cpp -gen-clang-attr-doc-table
+  -I ${CMAKE_CURRENT_SOURCE_DIR}/../../include/
+  SOURCE ${CMAKE_CURRENT_SOURCE_DIR}/../../include/clang/Basic/Attr.td
+  TARGET ClangAttrDocTable)
+
 add_clang_library(clangAST
   APValue.cpp
   ASTConcept.cpp
   ASTConsumer.cpp
   ASTContext.cpp
   ASTDiagnostic.cpp
   ASTDumper.cpp
   ASTImporter.cpp
   ASTImporterLookupTable.cpp
   ASTStructuralEquivalence.cpp
   ASTTypeTraits.cpp
+  AttrDocTable.cpp
   AttrImpl.cpp
   Comment.cpp
   CommentBriefParser.cpp
   CommentCommandTraits.cpp
   CommentLexer.cpp
   CommentParser.cpp
   CommentSema.cpp
   ComparisonCategories.cpp

clang/unittests/AST/AttrTest.cpp

+//===- unittests/AST/AttrTests.cpp --- Attribute tests --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/Attr.h"
+#include "clang/Basic/AttrKinds.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+using namespace clang;
+
+namespace {
+
+TEST(Attr, Doc) {
+  EXPECT_THAT(Attr::getDocumentation(attr::Used).str(),
+              testing::HasSubstr("The compiler must emit the definition even "
+                                 "if it appears to be unused"));
+}
+
+} // namespace

clang/unittests/AST/CMakeLists.txt

add_clang_unittest(ASTTests
   ASTImporterTest.cpp
   ASTImporterObjCTest.cpp
   ASTImporterGenericRedeclTest.cpp
   ASTImporterODRStrategiesTest.cpp
   ASTImporterVisibilityTest.cpp
   ASTTraverserTest.cpp
   ASTTypeTraitsTest.cpp
   ASTVectorTest.cpp
+  AttrTest.cpp
   CommentLexer.cpp
   CommentParser.cpp
   CommentTextTest.cpp
   DataCollectionTest.cpp
   DeclPrinterTest.cpp
   DeclTest.cpp
   EvaluateAsRValueTest.cpp
   ExternalASTSourceTest.cpp

clang/utils/TableGen/ClangAttrEmitter.cpp

void EmitClangAttrParserStringSwitches(RecordKeeper &Records,
   emitClangAttrLateParsedList(Records, OS);
 }
 
 void EmitClangAttrSubjectMatchRulesParserStringSwitches(RecordKeeper &Records,
                                                         raw_ostream &OS) {
   getPragmaAttributeSupport(Records).generateParsingHelpers(OS);
 }
 
+void EmitClangAttrDocTable(RecordKeeper &Records, raw_ostream &OS) {
+  emitSourceFileHeader("Clang attribute documentation", OS);
+
+  OS << R"cpp(
+  #include "clang/AST/Attr.h"
+  #include "llvm/ADT/StringRef.h"
+  )cpp";
+  std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
+    if (!A->getValueAsBit("ASTNode"))
+      continue;
+    std::vector<Record *> Docs = A->getValueAsListOfDefs("Documentation");
+    for (const auto *D : Docs) {
+      OS << "\nstatic const char AttrDoc_" << A->getName() << "[] = "
+         << "R\"reST("
+         << D->getValueAsOptionalString("Content").getValueOr("").trim()
+         << ")reST\";\n";
+      // Only look at the first documentation if there are several.
+      // (Currently there's only one such attr, revisit if this becomes common).
+      break;
+    }
+  }
+  OS << R"cpp(
+  static const llvm::StringRef AttrDoc[] = {
+  #define ATTR(NAME) AttrDoc_##NAME,
+  #include "clang/Basic/AttrList.inc"
+  };
+
+  llvm::StringRef clang::Attr::getDocumentation(clang::attr::Kind K) {
+    if(K < llvm::array_lengthof(AttrDoc))
+      return AttrDoc[K];
+    return "";
+  }
+  )cpp";
+}
+
 enum class SpellingKind {
   GNU,
   CXX11,
   C2x,
   Declspec,
   Microsoft,
   Keyword,
   Pragma,

clang/utils/TableGen/TableGen.cpp

 enum ActionType {
   PrintRecords,
   DumpJSON,
   GenClangAttrClasses,
   GenClangAttrParserStringSwitches,
   GenClangAttrSubjectMatchRulesParserStringSwitches,
   GenClangAttrImpl,
   GenClangAttrList,
+  GenClangAttrDocTable,
   GenClangAttrSubjectMatchRuleList,
   GenClangAttrPCHRead,
   GenClangAttrPCHWrite,
   GenClangAttrHasAttributeImpl,
   GenClangAttrSpellingListIndex,
   GenClangAttrASTVisitor,
   GenClangAttrTemplateInstantiate,
   GenClangAttrParsedAttrList,
cl::values(
         clEnumValN(GenClangAttrSubjectMatchRulesParserStringSwitches,
                    "gen-clang-attr-subject-match-rules-parser-string-switches",
                    "Generate all parser-related attribute subject match rule"
                    "string switches"),
         clEnumValN(GenClangAttrImpl, "gen-clang-attr-impl",
                    "Generate clang attribute implementations"),
         clEnumValN(GenClangAttrList, "gen-clang-attr-list",
                    "Generate a clang attribute list"),
+        clEnumValN(GenClangAttrDocTable, "gen-clang-attr-doc-table",
+                   "Generate a table of attribute documentation"),
         clEnumValN(GenClangAttrSubjectMatchRuleList,
                    "gen-clang-attr-subject-match-rule-list",
                    "Generate a clang attribute subject match rule list"),
         clEnumValN(GenClangAttrPCHRead, "gen-clang-attr-pch-read",
                    "Generate clang PCH attribute reader"),
         clEnumValN(GenClangAttrPCHWrite, "gen-clang-attr-pch-write",
                    "Generate clang PCH attribute writer"),
         clEnumValN(GenClangAttrHasAttributeImpl,
case GenClangAttrSubjectMatchRulesParserStringSwitches:
     EmitClangAttrSubjectMatchRulesParserStringSwitches(Records, OS);
     break;
   case GenClangAttrImpl:
     EmitClangAttrImpl(Records, OS);
     break;
   case GenClangAttrList:
     EmitClangAttrList(Records, OS);
     break;
+  case GenClangAttrDocTable:
+    EmitClangAttrDocTable(Records, OS);
+    break;
   case GenClangAttrSubjectMatchRuleList:
     EmitClangAttrSubjectMatchRuleList(Records, OS);
     break;
   case GenClangAttrPCHRead:
     EmitClangAttrPCHRead(Records, OS);
     break;
   case GenClangAttrPCHWrite:
     EmitClangAttrPCHWrite(Records, OS);

clang/utils/TableGen/TableGenBackends.h

 void EmitClangAttrParsedAttrImpl(llvm::RecordKeeper &Records,
                                  llvm::raw_ostream &OS);
 void EmitClangAttrParsedAttrKinds(llvm::RecordKeeper &Records,
                                   llvm::raw_ostream &OS);
 void EmitClangAttrTextNodeDump(llvm::RecordKeeper &Records,
                                llvm::raw_ostream &OS);
 void EmitClangAttrNodeTraverse(llvm::RecordKeeper &Records,
                                llvm::raw_ostream &OS);
+void EmitClangAttrDocTable(llvm::RecordKeeper &Records, llvm::raw_ostream &OS);
 
 void EmitClangDiagsDefs(llvm::RecordKeeper &Records, llvm::raw_ostream &OS,
                         const std::string &Component);
 void EmitClangDiagGroups(llvm::RecordKeeper &Records, llvm::raw_ostream &OS);
 void EmitClangDiagsIndexName(llvm::RecordKeeper &Records,
                              llvm::raw_ostream &OS);
 
 void EmitClangSACheckers(llvm::RecordKeeper &Records, llvm::raw_ostream &OS);