From 2ff40ca367d5e4e0807b255e17b90a9fc49f86f8 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Wed, 24 Jul 2019 09:39:11 +0000 Subject: [PATCH] [clangd] Fix SelectionTree traversal of qualified types Summary: QualifiedTypeLoc isn't treated like a regular citizen by RecursiveASTVisitor. This meant we weren't intercepting the traversal of its inner TypeLoc. Most of the changes here are about exposing kind() so we can improve the precision of our tests. This should fix the issue raised in D65067. Reviewers: hokein Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D65100 llvm-svn: 366882 --- clang-tools-extra/clangd/Selection.cpp | 31 ++++++++++++++++------ clang-tools-extra/clangd/Selection.h | 2 ++ .../clangd/unittests/SelectionTests.cpp | 26 +++++++++--------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-extra/clangd/Selection.cpp index 77db371d..cbb83a1 100644 --- a/clang-tools-extra/clangd/Selection.cpp +++ b/clang-tools-extra/clangd/Selection.cpp @@ -63,10 +63,8 @@ private: std::vector> Ranges; // Always sorted. }; -// Dump a node for debugging. -// DynTypedNode::print() doesn't include the kind of node, which is useful. -void printNode(llvm::raw_ostream &OS, const DynTypedNode &N, - const PrintingPolicy &PP) { +// Show the type of a node for debugging. +void printNodeKind(llvm::raw_ostream &OS, const DynTypedNode &N) { if (const TypeLoc *TL = N.get()) { // TypeLoc is a hierarchy, but has only a single ASTNodeKind. // Synthesize the name from the Type subclass (except for QualifiedTypeLoc). @@ -77,14 +75,13 @@ void printNode(llvm::raw_ostream &OS, const DynTypedNode &N, } else { OS << N.getNodeKind().asStringRef(); } - OS << " "; - N.print(OS, PP); } std::string printNodeToString(const DynTypedNode &N, const PrintingPolicy &PP) { std::string S; llvm::raw_string_ostream OS(S); - printNode(OS, N, PP); + printNodeKind(OS, N); + OS << " "; return std::move(OS.str()); } @@ -155,6 +152,15 @@ public: pop(); return true; } + // QualifiedTypeLoc is handled strangely in RecursiveASTVisitor: the derived + // TraverseTypeLoc is not called for the inner UnqualTypeLoc. + // This means we'd never see 'int' in 'const int'! Work around that here. + // (The reason for the behavior is to avoid traversing the nested Type twice, + // but we ignore TraverseType anyway). + bool TraverseQualifiedTypeLoc(QualifiedTypeLoc QX) { + return traverseNode( + &QX, [&] { return TraverseTypeLoc(QX.getUnqualifiedLoc()); }); + } // Uninteresting parts of the AST that don't have locations within them. bool TraverseNestedNameSpecifier(NestedNameSpecifier *) { return true; } bool TraverseType(QualType) { return true; } @@ -361,12 +367,21 @@ void SelectionTree::print(llvm::raw_ostream &OS, const SelectionTree::Node &N, : '.'); else OS.indent(Indent); - printNode(OS, N.ASTNode, PrintPolicy); + printNodeKind(OS, N.ASTNode); + OS << ' '; + N.ASTNode.print(OS, PrintPolicy); OS << "\n"; for (const Node *Child : N.Children) print(OS, *Child, Indent + 2); } +std::string SelectionTree::Node::kind() const { + std::string S; + llvm::raw_string_ostream OS(S); + printNodeKind(OS, ASTNode); + return std::move(OS.str()); +} + // Decide which selection emulates a "point" query in between characters. static std::pair pointBounds(unsigned Offset, FileID FID, ASTContext &AST) { diff --git a/clang-tools-extra/clangd/Selection.h b/clang-tools-extra/clangd/Selection.h index 24d0609..a501a99 100644 --- a/clang-tools-extra/clangd/Selection.h +++ b/clang-tools-extra/clangd/Selection.h @@ -96,6 +96,8 @@ public: // Walk up the AST to get the DeclContext of this Node, // which is not the node itself. const DeclContext& getDeclContext() const; + // Printable node kind, like "CXXRecordDecl" or "AutoTypeLoc". + std::string kind() const; }; // The most specific common ancestor of all the selected nodes. // If there is no selection, this is nullptr. diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp b/clang-tools-extra/clangd/unittests/SelectionTests.cpp index 9d3b1fc..de8087e 100644 --- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp +++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp @@ -49,9 +49,7 @@ Range nodeRange(const SelectionTree::Node *N, ParsedAST &AST) { } std::string nodeKind(const SelectionTree::Node *N) { - if (!N) - return ""; - return N->ASTNode.getNodeKind().asStringRef().str(); + return N ? N->kind() : ""; } std::vector allNodes(const SelectionTree &T) { @@ -102,14 +100,14 @@ TEST(SelectionTest, CommonAncestor) { struct AAA { struct BBB { static int ccc(); };}; int x = AAA::[[B^B^B]]::ccc(); )cpp", - "TypeLoc", + "RecordTypeLoc", }, { R"cpp( struct AAA { struct BBB { static int ccc(); };}; int x = AAA::[[B^BB^]]::ccc(); )cpp", - "TypeLoc", + "RecordTypeLoc", }, { R"cpp( @@ -182,19 +180,19 @@ TEST(SelectionTest, CommonAncestor) { R"cpp( [[^void]] (*S)(int) = nullptr; )cpp", - "TypeLoc", + "BuiltinTypeLoc", }, { R"cpp( [[void (*S)^(int)]] = nullptr; )cpp", - "TypeLoc", + "FunctionProtoTypeLoc", }, { R"cpp( [[void (^*S)(int)]] = nullptr; )cpp", - "TypeLoc", + "FunctionProtoTypeLoc", }, { R"cpp( @@ -206,7 +204,7 @@ TEST(SelectionTest, CommonAncestor) { R"cpp( [[void ^(*S)(int)]] = nullptr; )cpp", - "TypeLoc", + "FunctionProtoTypeLoc", }, // Point selections. @@ -218,8 +216,8 @@ TEST(SelectionTest, CommonAncestor) { {"int bar; void foo() [[{ foo (); }]]^", "CompoundStmt"}, // Tricky case: FunctionTypeLoc in FunctionDecl has a hole in it. - {"[[^void]] foo();", "TypeLoc"}, - {"[[void foo^()]];", "TypeLoc"}, + {"[[^void]] foo();", "BuiltinTypeLoc"}, + {"[[void foo^()]];", "FunctionProtoTypeLoc"}, {"[[^void foo^()]];", "FunctionDecl"}, {"[[void ^foo()]];", "FunctionDecl"}, // Tricky case: two VarDecls share a specifier. @@ -229,6 +227,9 @@ TEST(SelectionTest, CommonAncestor) { {"[[st^ruct {int x;}]] y;", "CXXRecordDecl"}, {"[[struct {int x;} ^y]];", "VarDecl"}, {"struct {[[int ^x]];} y;", "FieldDecl"}, + // FIXME: the AST has no location info for qualifiers. + {"const [[a^uto]] x = 42;", "AutoTypeLoc"}, + {"[[co^nst auto x = 42]];", "VarDecl"}, {"^", nullptr}, {"void foo() { [[foo^^]] (); }", "DeclRefExpr"}, @@ -239,7 +240,8 @@ TEST(SelectionTest, CommonAncestor) { {"int x = 42^;", nullptr}, // Node types that have caused problems in the past. - {"template void foo() { [[^T]] t; }", "TypeLoc"}, + {"template void foo() { [[^T]] t; }", + "TemplateTypeParmTypeLoc"}, // No crash { -- 2.7.4