[clang-tools-extra] f06f439 - [clangd] targetDecl() returns only NamedDecls.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Jan 3 09:18:48 PST 2020
Author: Sam McCall
Date: 2020-01-03T18:18:40+01:00
New Revision: f06f439fadf740ea9019f2eb7f26ff88198ed375
URL: https://github.com/llvm/llvm-project/commit/f06f439fadf740ea9019f2eb7f26ff88198ed375
DIFF: https://github.com/llvm/llvm-project/commit/f06f439fadf740ea9019f2eb7f26ff88198ed375.diff
LOG: [clangd] targetDecl() returns only NamedDecls.
Summary:
While it's perfectly reasonable for non-named decls such as
static_assert to resolve to themselves:
- nothing else ever resolves to them
- features based on references (hover, highlight, find refs etc) tend
to be uninteresting where only trivial references are possible
- returning NamedDecl is a more convenient API (we cast to it in many places)
- this aligns closer to findExplicitReferences/explicitReferenceTargets
This fixes a crash in explicitReferenceTargets: if the target is a
non-named decl then there's an invalid unchecked cast to NamedDecl.
In practice this means when hovering over e.g. a static_assert:
- before ac3f9e4842, we would show a (boring) hover card
- after ac3f9e4842, we would crash
- after this patch, we will show nothing
Reviewers: kadircet, ilya-biryukov
Subscribers: MaskRay, jkorous, arphaman, usaxena95, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D72163
Added:
Modified:
clang-tools-extra/clangd/FindTarget.cpp
clang-tools-extra/clangd/FindTarget.h
clang-tools-extra/clangd/XRefs.cpp
clang-tools-extra/clangd/refactor/Rename.cpp
clang-tools-extra/clangd/unittests/FindTargetTests.cpp
clang-tools-extra/clangd/unittests/HoverTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/FindTarget.cpp b/clang-tools-extra/clangd/FindTarget.cpp
index f9332ee4f220..f5d7f4a9326b 100644
--- a/clang-tools-extra/clangd/FindTarget.cpp
+++ b/clang-tools-extra/clangd/FindTarget.cpp
@@ -119,10 +119,10 @@ getMembersReferencedViaDependentName(const Type *T, const DeclarationName &Name,
struct TargetFinder {
using RelSet = DeclRelationSet;
using Rel = DeclRelation;
- llvm::SmallDenseMap<const Decl *, RelSet> Decls;
+ llvm::SmallDenseMap<const NamedDecl *, RelSet> Decls;
RelSet Flags;
- static const Decl *getTemplatePattern(const Decl *D) {
+ static const NamedDecl *getTemplatePattern(const NamedDecl *D) {
if (const CXXRecordDecl *CRD = dyn_cast<CXXRecordDecl>(D)) {
return CRD->getTemplateInstantiationPattern();
} else if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) {
@@ -134,12 +134,12 @@ struct TargetFinder {
} else if (const auto *ED = dyn_cast<EnumDecl>(D)) {
return ED->getInstantiatedFromMemberEnum();
} else if (isa<FieldDecl>(D) || isa<TypedefNameDecl>(D)) {
- const auto *ND = cast<NamedDecl>(D);
- if (const DeclContext *Parent = dyn_cast_or_null<DeclContext>(
- getTemplatePattern(llvm::cast<Decl>(ND->getDeclContext()))))
- for (const NamedDecl *BaseND : Parent->lookup(ND->getDeclName()))
- if (!BaseND->isImplicit() && BaseND->getKind() == ND->getKind())
- return BaseND;
+ if (const auto *Parent = llvm::dyn_cast<NamedDecl>(D->getDeclContext()))
+ if (const DeclContext *ParentPat =
+ dyn_cast_or_null<DeclContext>(getTemplatePattern(Parent)))
+ for (const NamedDecl *BaseND : ParentPat->lookup(D->getDeclName()))
+ if (!BaseND->isImplicit() && BaseND->getKind() == D->getKind())
+ return BaseND;
} else if (const auto *ECD = dyn_cast<EnumConstantDecl>(D)) {
if (const auto *ED = dyn_cast<EnumDecl>(ECD->getDeclContext())) {
if (const EnumDecl *Pattern = ED->getInstantiatedFromMemberEnum()) {
@@ -156,14 +156,15 @@ struct TargetFinder {
nodeToString(ast_type_traits::DynTypedNode::create(Node)));
}
- void report(const Decl *D, RelSet Flags) {
+ void report(const NamedDecl *D, RelSet Flags) {
dlog("--> [{0}] {1}", Flags,
nodeToString(ast_type_traits::DynTypedNode::create(*D)));
Decls[D] |= Flags;
}
public:
- void add(const Decl *D, RelSet Flags) {
+ void add(const Decl *Dcl, RelSet Flags) {
+ const NamedDecl *D = llvm::dyn_cast<NamedDecl>(Dcl);
if (!D)
return;
debug(*D, Flags);
@@ -405,7 +406,7 @@ struct TargetFinder {
} // namespace
-llvm::SmallVector<std::pair<const Decl *, DeclRelationSet>, 1>
+llvm::SmallVector<std::pair<const NamedDecl *, DeclRelationSet>, 1>
allTargetDecls(const ast_type_traits::DynTypedNode &N) {
dlog("allTargetDecls({0})", nodeToString(N));
TargetFinder Finder;
@@ -428,9 +429,9 @@ allTargetDecls(const ast_type_traits::DynTypedNode &N) {
return {Finder.Decls.begin(), Finder.Decls.end()};
}
-llvm::SmallVector<const Decl *, 1>
+llvm::SmallVector<const NamedDecl *, 1>
targetDecl(const ast_type_traits::DynTypedNode &N, DeclRelationSet Mask) {
- llvm::SmallVector<const Decl *, 1> Result;
+ llvm::SmallVector<const NamedDecl *, 1> Result;
for (const auto &Entry : allTargetDecls(N)) {
if (!(Entry.second & ~Mask))
Result.push_back(Entry.first);
@@ -456,12 +457,12 @@ explicitReferenceTargets(DynTypedNode N, DeclRelationSet Mask) {
if (D.second & ~Mask)
continue;
if (D.second & DeclRelation::TemplatePattern) {
- TemplatePatterns.push_back(llvm::cast<NamedDecl>(D.first));
+ TemplatePatterns.push_back(D.first);
continue;
}
if (D.second & DeclRelation::TemplateInstantiation)
SeenTemplateInstantiations = true;
- Targets.push_back(llvm::cast<NamedDecl>(D.first));
+ Targets.push_back(D.first);
}
if (!SeenTemplateInstantiations)
Targets.insert(Targets.end(), TemplatePatterns.begin(),
diff --git a/clang-tools-extra/clangd/FindTarget.h b/clang-tools-extra/clangd/FindTarget.h
index 2f6c26ca6912..eeb063b5f00e 100644
--- a/clang-tools-extra/clangd/FindTarget.h
+++ b/clang-tools-extra/clangd/FindTarget.h
@@ -75,15 +75,19 @@ class DeclRelationSet;
/// / \
/// RecordDecl S TypeAliasDecl T
///
+/// Note that this function only returns NamedDecls. Generally other decls
+/// don't have references in this sense, just the node itself.
+/// If callers want to support such decls, they should cast the node directly.
+///
/// FIXME: some AST nodes cannot be DynTypedNodes, these cannot be specified.
-llvm::SmallVector<const Decl *, 1>
+llvm::SmallVector<const NamedDecl *, 1>
targetDecl(const ast_type_traits::DynTypedNode &, DeclRelationSet Mask);
/// Similar to targetDecl(), however instead of applying a filter, all possible
/// decls are returned along with their DeclRelationSets.
/// This is suitable for indexing, where everything is recorded and filtering
/// is applied later.
-llvm::SmallVector<std::pair<const Decl *, DeclRelationSet>, 1>
+llvm::SmallVector<std::pair<const NamedDecl *, DeclRelationSet>, 1>
allTargetDecls(const ast_type_traits::DynTypedNode &);
enum class DeclRelation : unsigned {
diff --git a/clang-tools-extra/clangd/XRefs.cpp b/clang-tools-extra/clangd/XRefs.cpp
index 1ca388f49f86..fecc3668f98e 100644
--- a/clang-tools-extra/clangd/XRefs.cpp
+++ b/clang-tools-extra/clangd/XRefs.cpp
@@ -53,7 +53,7 @@ namespace {
// - for kinds that allow multiple definitions (e.g. namespaces), return nullptr
// Kinds of nodes that always return nullptr here will not have definitions
// reported by locateSymbolAt().
-const Decl *getDefinition(const Decl *D) {
+const NamedDecl *getDefinition(const NamedDecl *D) {
assert(D);
// Decl has one definition that we can find.
if (const auto *TD = dyn_cast<TagDecl>(D))
@@ -129,13 +129,14 @@ SymbolLocation getPreferredLocation(const Location &ASTLoc,
return Merged.CanonicalDeclaration;
}
-std::vector<const Decl *> getDeclAtPosition(ParsedAST &AST, SourceLocation Pos,
- DeclRelationSet Relations) {
+std::vector<const NamedDecl *> getDeclAtPosition(ParsedAST &AST,
+ SourceLocation Pos,
+ DeclRelationSet Relations) {
FileID FID;
unsigned Offset;
std::tie(FID, Offset) = AST.getSourceManager().getDecomposedSpellingLoc(Pos);
SelectionTree Selection(AST.getASTContext(), AST.getTokens(), Offset);
- std::vector<const Decl *> Result;
+ std::vector<const NamedDecl *> Result;
if (const SelectionTree::Node *N = Selection.commonAncestor()) {
auto Decls = targetDecl(N->ASTNode, Relations);
Result.assign(Decls.begin(), Decls.end());
@@ -251,9 +252,9 @@ std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
// Emit all symbol locations (declaration or definition) from AST.
DeclRelationSet Relations =
DeclRelation::TemplatePattern | DeclRelation::Alias;
- for (const Decl *D : getDeclAtPosition(AST, SourceLoc, Relations)) {
- const Decl *Def = getDefinition(D);
- const Decl *Preferred = Def ? Def : D;
+ for (const NamedDecl *D : getDeclAtPosition(AST, SourceLoc, Relations)) {
+ const NamedDecl *Def = getDefinition(D);
+ const NamedDecl *Preferred = Def ? Def : D;
// If we're at the point of declaration of a template specialization,
// it's more useful to navigate to the template declaration.
@@ -272,8 +273,7 @@ std::vector<LocatedSymbol> locateSymbolAt(ParsedAST &AST, Position Pos,
continue;
Result.emplace_back();
- if (auto *ND = dyn_cast<NamedDecl>(Preferred))
- Result.back().Name = printName(AST.getASTContext(), *ND);
+ Result.back().Name = printName(AST.getASTContext(), *D);
Result.back().PreferredDeclaration = *Loc;
// Preferred is always a definition if possible, so this check works.
if (Def == Preferred)
@@ -332,9 +332,9 @@ class ReferenceFinder : public index::IndexDataConsumer {
};
ReferenceFinder(ASTContext &AST, Preprocessor &PP,
- const std::vector<const Decl *> &TargetDecls)
+ const std::vector<const NamedDecl *> &TargetDecls)
: AST(AST) {
- for (const Decl *D : TargetDecls)
+ for (const NamedDecl *D : TargetDecls)
CanonicalTargets.insert(D->getCanonicalDecl());
}
@@ -372,7 +372,7 @@ class ReferenceFinder : public index::IndexDataConsumer {
};
std::vector<ReferenceFinder::Reference>
-findRefs(const std::vector<const Decl *> &Decls, ParsedAST &AST) {
+findRefs(const std::vector<const NamedDecl *> &Decls, ParsedAST &AST) {
ReferenceFinder RefFinder(AST.getASTContext(), AST.getPreprocessor(), Decls);
index::IndexingOptions IndexOpts;
IndexOpts.SystemSymbolFilter =
@@ -506,18 +506,16 @@ std::vector<SymbolDetails> getSymbolInfo(ParsedAST &AST, Position Pos) {
// DeclRelation::Underlying.
DeclRelationSet Relations = DeclRelation::TemplatePattern |
DeclRelation::Alias | DeclRelation::Underlying;
- for (const Decl *D : getDeclAtPosition(AST, Loc, Relations)) {
+ for (const NamedDecl *D : getDeclAtPosition(AST, Loc, Relations)) {
SymbolDetails NewSymbol;
- if (const NamedDecl *ND = dyn_cast<NamedDecl>(D)) {
- std::string QName = printQualifiedName(*ND);
- std::tie(NewSymbol.containerName, NewSymbol.name) =
- splitQualifiedName(QName);
-
- if (NewSymbol.containerName.empty()) {
- if (const auto *ParentND =
- dyn_cast_or_null<NamedDecl>(ND->getDeclContext()))
- NewSymbol.containerName = printQualifiedName(*ParentND);
- }
+ std::string QName = printQualifiedName(*D);
+ std::tie(NewSymbol.containerName, NewSymbol.name) =
+ splitQualifiedName(QName);
+
+ if (NewSymbol.containerName.empty()) {
+ if (const auto *ParentND =
+ dyn_cast_or_null<NamedDecl>(D->getDeclContext()))
+ NewSymbol.containerName = printQualifiedName(*ParentND);
}
llvm::SmallString<32> USR;
if (!index::generateUSRForDecl(D, USR)) {
@@ -680,7 +678,7 @@ const CXXRecordDecl *findRecordTypeAt(ParsedAST &AST, Position Pos) {
if (Decls.empty())
return nullptr;
- const Decl *D = Decls[0];
+ const NamedDecl *D = Decls[0];
if (const VarDecl *VD = dyn_cast<VarDecl>(D)) {
// If this is a variable, use the type of the variable.
diff --git a/clang-tools-extra/clangd/refactor/Rename.cpp b/clang-tools-extra/clangd/refactor/Rename.cpp
index 7d97ac8c9645..0241c00ef8b4 100644
--- a/clang-tools-extra/clangd/refactor/Rename.cpp
+++ b/clang-tools-extra/clangd/refactor/Rename.cpp
@@ -86,15 +86,12 @@ llvm::DenseSet<const Decl *> locateDeclAt(ParsedAST &AST,
return {};
llvm::DenseSet<const Decl *> Result;
- for (const auto *D :
+ for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
DeclRelation::Alias | DeclRelation::TemplatePattern)) {
- const auto *ND = llvm::dyn_cast<NamedDecl>(D);
- if (!ND)
- continue;
// Get to CXXRecordDecl from constructor or destructor.
- ND = tooling::getCanonicalSymbolDeclaration(ND);
- Result.insert(ND);
+ D = tooling::getCanonicalSymbolDeclaration(D);
+ Result.insert(D);
}
return Result;
}
diff --git a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
index a81569485e89..8c9727fb64eb 100644
--- a/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
+++ b/clang-tools-extra/clangd/unittests/FindTargetTests.cpp
@@ -34,7 +34,7 @@ namespace {
struct PrintedDecl {
PrintedDecl(const char *Name, DeclRelationSet Relations = {})
: Name(Name), Relations(Relations) {}
- PrintedDecl(const Decl *D, DeclRelationSet Relations = {})
+ PrintedDecl(const NamedDecl *D, DeclRelationSet Relations = {})
: Relations(Relations) {
std::string S;
llvm::raw_string_ostream OS(S);
diff --git a/clang-tools-extra/clangd/unittests/HoverTests.cpp b/clang-tools-extra/clangd/unittests/HoverTests.cpp
index ba8f773fdf83..69e65c98cbdf 100644
--- a/clang-tools-extra/clangd/unittests/HoverTests.cpp
+++ b/clang-tools-extra/clangd/unittests/HoverTests.cpp
@@ -598,6 +598,9 @@ TEST(Hover, NoHover) {
func<1>();
}
)cpp",
+ R"cpp(// non-named decls don't get hover. Don't crash!
+ ^static_assert(1, "");
+ )cpp",
};
for (const auto &Test : Tests) {
More information about the cfe-commits
mailing list