From bb81e7083d25bfc9bcf2bd69f5431d0c53b86a32 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Tue, 20 Oct 2020 12:01:48 +0200 Subject: [PATCH] [clangd] Add basic support for attributes (selection, hover) These aren't terribly common, but we currently mishandle them badly. Not only do we not recogize the attributes themselves, but we often end up selecting some node other than the parent (because source ranges aren't accurate in the presence of attributes). Differential Revision: https://reviews.llvm.org/D89785 --- clang-tools-extra/clangd/AST.cpp | 19 +++++++++ clang-tools-extra/clangd/AST.h | 4 ++ clang-tools-extra/clangd/Hover.cpp | 17 ++++++++ clang-tools-extra/clangd/Selection.cpp | 10 ++++- clang-tools-extra/clangd/XRefs.cpp | 28 ++++++------- .../clangd/unittests/ASTTests.cpp | 39 +++++++++++++++++++ .../clangd/unittests/HoverTests.cpp | 9 +++++ .../clangd/unittests/SelectionTests.cpp | 14 ++++++- 8 files changed, 124 insertions(+), 16 deletions(-) diff --git a/clang-tools-extra/clangd/AST.cpp b/clang-tools-extra/clangd/AST.cpp index 778b945c47ca..a47a5a1aab1d 100644 --- a/clang-tools-extra/clangd/AST.cpp +++ b/clang-tools-extra/clangd/AST.cpp @@ -20,7 +20,9 @@ #include "clang/AST/NestedNameSpecifier.h" #include "clang/AST/PrettyPrinter.h" #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Stmt.h" #include "clang/AST/TemplateBase.h" +#include "clang/AST/TypeLoc.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Basic/Specifiers.h" @@ -481,6 +483,23 @@ llvm::Optional getDeducedType(ASTContext &ASTCtx, return V.DeducedType; } +std::vector getAttributes(const DynTypedNode &N) { + std::vector Result; + if (const auto *TL = N.get()) { + for (AttributedTypeLoc ATL = TL->getAs(); !ATL.isNull(); + ATL = ATL.getModifiedLoc().getAs()) { + Result.push_back(ATL.getAttr()); + assert(!ATL.getModifiedLoc().isNull()); + } + } + if (const auto *S = N.get()) + for (; S != nullptr; S = dyn_cast(S->getSubStmt())) + llvm::copy(S->getAttrs(), std::back_inserter(Result)); + if (const auto *D = N.get()) + llvm::copy(D->attrs(), std::back_inserter(Result)); + return Result; +} + std::string getQualification(ASTContext &Context, const DeclContext *DestContext, SourceLocation InsertionPoint, diff --git a/clang-tools-extra/clangd/AST.h b/clang-tools-extra/clangd/AST.h index 7793b6545c0f..53d9f16a4262 100644 --- a/clang-tools-extra/clangd/AST.h +++ b/clang-tools-extra/clangd/AST.h @@ -27,6 +27,7 @@ namespace clang { class SourceManager; class Decl; +class DynTypedNode; namespace clangd { @@ -121,6 +122,9 @@ QualType declaredType(const TypeDecl *D); /// If the type is an undeduced auto, returns the type itself. llvm::Optional getDeducedType(ASTContext &, SourceLocation Loc); +/// Return attributes attached directly to a node. +std::vector getAttributes(const DynTypedNode &); + /// Gets the nested name specifier necessary for spelling \p ND in \p /// DestContext, at \p InsertionPoint. It selects the shortest suffix of \p ND /// such that it is visible in \p DestContext. diff --git a/clang-tools-extra/clangd/Hover.cpp b/clang-tools-extra/clangd/Hover.cpp index c71a8c40ce94..228cd192e057 100644 --- a/clang-tools-extra/clangd/Hover.cpp +++ b/clang-tools-extra/clangd/Hover.cpp @@ -19,6 +19,7 @@ #include "support/Markup.h" #include "clang/AST/ASTContext.h" #include "clang/AST/ASTTypeTraits.h" +#include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclBase.h" #include "clang/AST/DeclCXX.h" @@ -720,6 +721,20 @@ llvm::Optional getHoverContents(const Expr *E, ParsedAST &AST, return llvm::None; } +// Generates hover info for attributes. +llvm::Optional 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? + return HI; +} + bool isParagraphBreak(llvm::StringRef Rest) { return Rest.ltrim(" \t").startswith("\n"); } @@ -960,6 +975,8 @@ llvm::Optional getHover(ParsedAST &AST, Position Pos, maybeAddCalleeArgInfo(N, *HI, PP); } else if (const Expr *E = N->ASTNode.get()) { HI = getHoverContents(E, AST, PP, Index); + } else if (const Attr *A = N->ASTNode.get()) { + HI = getHoverContents(A, AST); } // FIXME: support hovers for other nodes? // - built-in types diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-extra/clangd/Selection.cpp index b4f767fde095..e3a2e3199766 100644 --- a/clang-tools-extra/clangd/Selection.cpp +++ b/clang-tools-extra/clangd/Selection.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Selection.h" +#include "AST.h" #include "SourceCode.h" #include "support/Logger.h" #include "support/Trace.h" @@ -490,7 +491,6 @@ public: // Two categories of nodes are not "well-behaved": // - those without source range information, we don't record those // - those that can't be stored in DynTypedNode. - // We're missing some interesting things like Attr due to the latter. bool TraverseDecl(Decl *X) { if (X && isa(X)) return Base::TraverseDecl(X); // Already pushed by constructor. @@ -517,6 +517,9 @@ public: bool TraverseCXXBaseSpecifier(const CXXBaseSpecifier &X) { return traverseNode(&X, [&] { return Base::TraverseCXXBaseSpecifier(X); }); } + bool TraverseAttr(Attr *X) { + return traverseNode(X, [&] { return Base::TraverseAttr(X); }); + } // Stmt is the same, but this form allows the data recursion optimization. bool dataTraverseStmtPre(Stmt *X) { if (!X || isImplicit(X)) @@ -651,6 +654,11 @@ private: if (auto AT = TL->getAs()) S = AT.getModifiedLoc().getSourceRange(); } + // SourceRange often doesn't manage to accurately cover attributes. + // Fortunately, attributes are rare. + if (llvm::any_of(getAttributes(N), + [](const Attr *A) { return !A->isImplicit(); })) + return false; if (!SelChecker.mayHit(S)) { dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent()); dlog("{1}skipped range = {0}", S.printToString(SM), indent(1)); diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp index c21d3c3074ce..4cec85e20ad6 100644 --- a/clang-tools-extra/clangd/XRefs.cpp +++ b/clang-tools-extra/clangd/XRefs.cpp @@ -183,6 +183,12 @@ getDeclAtPositionWithRelations(ParsedAST &AST, SourceLocation Pos, if (const SelectionTree::Node *N = ST.commonAncestor()) { if (NodeKind) *NodeKind = N->ASTNode.getNodeKind(); + // Attributes don't target decls, look at the + // thing it's attached to. + // We still report the original NodeKind! + // This makes the `override` hack work. + if (N->ASTNode.get() && N->Parent) + N = N->Parent; llvm::copy_if(allTargetDecls(N->ASTNode, AST.getHeuristicResolver()), std::back_inserter(Result), [&](auto &Entry) { return !(Entry.second & ~Relations); }); @@ -343,7 +349,7 @@ std::vector findImplementors(llvm::DenseSet IDs, std::vector locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, ParsedAST &AST, llvm::StringRef MainFilePath, - const SymbolIndex *Index, ASTNodeKind *NodeKind) { + const SymbolIndex *Index, ASTNodeKind &NodeKind) { const SourceManager &SM = AST.getSourceManager(); // Results follow the order of Symbols.Decls. std::vector Result; @@ -376,7 +382,7 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, DeclRelationSet Relations = DeclRelation::TemplatePattern | DeclRelation::Alias; auto Candidates = - getDeclAtPositionWithRelations(AST, CurLoc, Relations, NodeKind); + getDeclAtPositionWithRelations(AST, CurLoc, Relations, &NodeKind); llvm::DenseSet VirtualMethods; for (const auto &E : Candidates) { const NamedDecl *D = E.first; @@ -392,13 +398,8 @@ locateASTReferent(SourceLocation CurLoc, const syntax::Token *TouchedIdentifier, } } // Special case: void foo() ^override: jump to the overridden method. - const InheritableAttr *Attr = D->getAttr(); - if (!Attr) - Attr = D->getAttr(); - if (Attr && TouchedIdentifier && - SM.getSpellingLoc(Attr->getLocation()) == - TouchedIdentifier->location()) { - LocateASTReferentMetric.record(1, "method-to-base"); + if (NodeKind.isSame(ASTNodeKind::getFromNodeKind()) || + NodeKind.isSame(ASTNodeKind::getFromNodeKind())) { // We may be overridding multiple methods - offer them all. for (const NamedDecl *ND : CMD->overridden_methods()) AddResultDecl(ND); @@ -800,7 +801,7 @@ std::vector locateSymbolAt(ParsedAST &AST, Position Pos, ASTNodeKind NodeKind; auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST, - *MainFilePath, Index, &NodeKind); + *MainFilePath, Index, NodeKind); if (!ASTResults.empty()) return ASTResults; @@ -816,9 +817,8 @@ std::vector locateSymbolAt(ParsedAST &AST, Position Pos, Word->Text); return {*std::move(Macro)}; } - ASTResults = - locateASTReferent(NearbyIdent->location(), NearbyIdent, AST, - *MainFilePath, Index, /*NodeKind=*/nullptr); + ASTResults = locateASTReferent(NearbyIdent->location(), NearbyIdent, AST, + *MainFilePath, Index, NodeKind); if (!ASTResults.empty()) { log("Found definition heuristically using nearby identifier {0}", NearbyIdent->text(SM)); @@ -1987,4 +1987,4 @@ llvm::DenseSet getNonLocalDeclRefs(ParsedAST &AST, } } // namespace clangd -} // namespace clang \ No newline at end of file +} // namespace clang diff --git a/clang-tools-extra/clangd/unittests/ASTTests.cpp b/clang-tools-extra/clangd/unittests/ASTTests.cpp index 67ce927bf765..df42a28d5ebe 100644 --- a/clang-tools-extra/clangd/unittests/ASTTests.cpp +++ b/clang-tools-extra/clangd/unittests/ASTTests.cpp @@ -11,8 +11,11 @@ #include "Annotations.h" #include "ParsedAST.h" #include "TestTU.h" +#include "clang/AST/ASTTypeTraits.h" +#include "clang/AST/Attr.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclBase.h" +#include "clang/Basic/AttrKinds.h" #include "clang/Basic/SourceManager.h" #include "llvm/ADT/StringRef.h" #include "llvm/Support/Casting.h" @@ -25,6 +28,8 @@ namespace clang { namespace clangd { namespace { +using testing::Contains; +using testing::Each; TEST(GetDeducedType, KwAutoKwDecltypeExpansion) { struct Test { @@ -389,6 +394,40 @@ TEST(ClangdAST, IsDeeplyNested) { EXPECT_FALSE( isDeeplyNested(&findUnqualifiedDecl(AST, "Bar"), /*MaxDepth=*/4)); } + +MATCHER_P(attrKind, K, "") { return arg->getKind() == K; } + +MATCHER(implicitAttr, "") { return arg->isImplicit(); } + +TEST(ClangdAST, GetAttributes) { + const char *Code = R"cpp( + class X{}; + class [[nodiscard]] Y{}; + void f(int * a, int * __attribute__((nonnull)) b); + void foo(bool c) { + if (c) + [[unlikely]] return; + } + )cpp"; + ParsedAST AST = TestTU::withCode(Code).build(); + auto DeclAttrs = [&](llvm::StringRef Name) { + return getAttributes(DynTypedNode::create(findUnqualifiedDecl(AST, Name))); + }; + // Implicit attributes may be present (e.g. visibility on windows). + ASSERT_THAT(DeclAttrs("X"), Each(implicitAttr())); + ASSERT_THAT(DeclAttrs("Y"), Contains(attrKind(attr::WarnUnusedResult))); + ASSERT_THAT(DeclAttrs("f"), Each(implicitAttr())); + ASSERT_THAT(DeclAttrs("a"), Each(implicitAttr())); + ASSERT_THAT(DeclAttrs("b"), Contains(attrKind(attr::NonNull))); + + Stmt *FooBody = cast(findDecl(AST, "foo")).getBody(); + IfStmt *FooIf = cast(cast(FooBody)->body_front()); + ASSERT_THAT(getAttributes(DynTypedNode::create(*FooIf)), + Each(implicitAttr())); + ASSERT_THAT(getAttributes(DynTypedNode::create(*FooIf->getThen())), + Contains(attrKind(attr::Unlikely))); +} + } // namespace } // namespace clangd } // namespace clang diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp index 9089a4859c14..5841e7166abf 100644 --- a/clang-tools-extra/clangd/unittests/HoverTests.cpp +++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp @@ -2377,6 +2377,15 @@ TEST(Hover, All) { HI.NamespaceScope = ""; HI.Value = "0"; }}, + {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 + }}, }; // Create a tiny index, so tests above can verify documentation is fetched. diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp b/clang-tools-extra/clangd/unittests/SelectionTests.cpp index 85b7d9fb541a..3898388d1ad2 100644 --- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp +++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp @@ -453,7 +453,19 @@ TEST(SelectionTest, CommonAncestor) { template