sammccall created this revision.
sammccall added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman.
Herald added a project: clang.
sammccall requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
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).
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D89785
Files:
clang-tools-extra/clangd/AST.cpp
clang-tools-extra/clangd/AST.h
clang-tools-extra/clangd/Hover.cpp
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/unittests/ASTTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
clang-tools-extra/clangd/unittests/SelectionTests.cpp
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -415,7 +415,19 @@
template <template <typename> class Container> class A {};
A<[[V^ector]]> a;
)cpp",
- "TemplateArgumentLoc"}};
+ "TemplateArgumentLoc"},
+
+ // Attributes
+ {R"cpp(
+ void f(int * __attribute__(([[no^nnull]])) );
+ )cpp",
+ "NonNullAttr"},
+
+ {R"cpp(
+ // Digraph syntax for attributes to avoid accidental annotations.
+ class <:[gsl::Owner([[in^t]])]:> X{};
+ )cpp",
+ "BuiltinTypeLoc"}};
for (const Case &C : Cases) {
trace::TestTracer Tracer;
Index: clang-tools-extra/clangd/unittests/HoverTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -1991,6 +1991,16 @@
HI.NamespaceScope = "ObjC::"; // FIXME: fix it
HI.Definition = "char data";
}},
+ {
+ 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.
Index: clang-tools-extra/clangd/unittests/ASTTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/ASTTests.cpp
+++ clang-tools-extra/clangd/unittests/ASTTests.cpp
@@ -11,6 +11,7 @@
#include "Annotations.h"
#include "ParsedAST.h"
#include "TestTU.h"
+#include "clang/AST/ASTTypeTraits.h"
#include "clang/AST/Decl.h"
#include "clang/AST/DeclBase.h"
#include "clang/Basic/SourceManager.h"
@@ -206,6 +207,31 @@
}
}
}
+
+TEST(ClangdAST, HasAttributes) {
+ 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();
+ ASSERT_FALSE(hasAttributes(DynTypedNode::create(findDecl(AST, "X"))));
+ ASSERT_TRUE(hasAttributes(DynTypedNode::create(findDecl(AST, "Y"))));
+ ASSERT_FALSE(hasAttributes(DynTypedNode::create(findDecl(AST, "f"))));
+ ASSERT_FALSE(
+ hasAttributes(DynTypedNode::create(findUnqualifiedDecl(AST, "a"))));
+ ASSERT_TRUE(
+ hasAttributes(DynTypedNode::create(findUnqualifiedDecl(AST, "b"))));
+
+ Stmt *FooBody = cast<FunctionDecl>(findDecl(AST, "foo")).getBody();
+ IfStmt *FooIf = cast<IfStmt>(cast<CompoundStmt>(FooBody)->body_front());
+ ASSERT_FALSE(hasAttributes(DynTypedNode::create(*FooIf)));
+ ASSERT_TRUE(hasAttributes(DynTypedNode::create(*FooIf->getThen())));
+}
} // namespace
} // namespace clangd
} // namespace clang
Index: clang-tools-extra/clangd/XRefs.cpp
===================================================================
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -182,6 +182,12 @@
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<Attr>() && N->Parent)
+ N = N->Parent;
llvm::copy(targetDecl(N->ASTNode, Relations),
std::back_inserter(Result));
}
@@ -287,7 +293,7 @@
std::vector<LocatedSymbol>
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<LocatedSymbol> Result;
@@ -317,15 +323,11 @@
DeclRelationSet Relations =
DeclRelation::TemplatePattern | DeclRelation::Alias;
for (const NamedDecl *D :
- getDeclAtPosition(AST, CurLoc, Relations, NodeKind)) {
+ getDeclAtPosition(AST, CurLoc, Relations, &NodeKind)) {
// Special case: void foo() ^override: jump to the overridden method.
- if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
- const InheritableAttr *Attr = D->getAttr<OverrideAttr>();
- if (!Attr)
- Attr = D->getAttr<FinalAttr>();
- if (Attr && TouchedIdentifier &&
- SM.getSpellingLoc(Attr->getLocation()) ==
- TouchedIdentifier->location()) {
+ if (NodeKind.isSame(ASTNodeKind::getFromNodeKind<OverrideAttr>()) ||
+ NodeKind.isSame(ASTNodeKind::getFromNodeKind<FinalAttr>())) {
+ if (const auto *CMD = llvm::dyn_cast<CXXMethodDecl>(D)) {
// We may be overridding multiple methods - offer them all.
for (const NamedDecl *ND : CMD->overridden_methods())
AddResultDecl(ND);
@@ -653,7 +655,7 @@
ASTNodeKind NodeKind;
auto ASTResults = locateASTReferent(*CurLoc, TouchedIdentifier, AST,
- *MainFilePath, Index, &NodeKind);
+ *MainFilePath, Index, NodeKind);
if (!ASTResults.empty())
return ASTResults;
@@ -669,9 +671,8 @@
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));
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ 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"
@@ -491,6 +492,9 @@
return traverseNode(
X, [&] { return Base::TraverseConstructorInitializer(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))
@@ -617,6 +621,10 @@
if (auto DT = TL->getAs<DecltypeTypeLoc>())
S.setEnd(DT.getUnderlyingExpr()->getEndLoc());
}
+ // SourceRange often doesn't manage to accurately cover attributes.
+ // Fortunately, attributes are rare.
+ if (hasAttributes(N))
+ return false;
if (!SelChecker.mayHit(S)) {
dlog("{1}skip: {0}", printNodeToString(N, PrintPolicy), indent());
dlog("{1}skipped range = {0}", S.printToString(SM), indent(1));
Index: clang-tools-extra/clangd/Hover.cpp
===================================================================
--- clang-tools-extra/clangd/Hover.cpp
+++ 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"
@@ -651,6 +652,20 @@
return llvm::None;
}
+// Generates hover info for attributes.
+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?
+ return HI;
+}
+
bool isParagraphBreak(llvm::StringRef Rest) {
return Rest.ltrim(" \t").startswith("\n");
}
@@ -862,6 +877,8 @@
maybeAddCalleeArgInfo(N, *HI, AST.getASTContext().getPrintingPolicy());
} else if (const Expr *E = N->ASTNode.get<Expr>()) {
HI = getHoverContents(E, AST);
+ } else if (const Attr *A = N->ASTNode.get<Attr>()) {
+ HI = getHoverContents(A, AST);
}
// FIXME: support hovers for other nodes?
// - built-in types
Index: clang-tools-extra/clangd/AST.h
===================================================================
--- clang-tools-extra/clangd/AST.h
+++ clang-tools-extra/clangd/AST.h
@@ -26,6 +26,7 @@
namespace clang {
class SourceManager;
class Decl;
+class DynTypedNode;
namespace clangd {
@@ -112,6 +113,10 @@
/// It will return the underlying type.
llvm::Optional<QualType> getDeducedType(ASTContext &, SourceLocation Loc);
+/// Does this node have any attributes (Attr) attached directly to it?
+/// (Such nodes often have incorrect SourceRanges).
+bool hasAttributes(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.
Index: clang-tools-extra/clangd/AST.cpp
===================================================================
--- clang-tools-extra/clangd/AST.cpp
+++ 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"
@@ -425,6 +427,16 @@
return V.DeducedType;
}
+bool hasAttributes(const DynTypedNode &N) {
+ if (N.get<AttributedTypeLoc>() || N.get<AttributedType>())
+ return true;
+ if (const auto *S = N.get<AttributedStmt>())
+ return true;
+ if (const auto *D = N.get<Decl>())
+ return D->hasAttrs();
+ return false;
+}
+
std::string getQualification(ASTContext &Context,
const DeclContext *DestContext,
SourceLocation InsertionPoint,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
