From 20c5fbb1af08c655792e40f30055c4070880b52b Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Wed, 2 Oct 2019 10:01:53 +0000 Subject: [PATCH] [clangd] SelectionTree should mark a node as fully-selected if the only claimed tokens were early-claimed. Summary: Previously they would be marked as partially-selected based on the early claim, and never updated as no more tokens were claimed. This affects simple VarDecls like "int x". Reviewers: SureYeaah Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D66872 llvm-svn: 373442 --- clang-tools-extra/clangd/Selection.cpp | 40 +++++++++++-------- .../refactor/tweaks/ExtractFunction.cpp | 9 ++--- .../clangd/unittests/SelectionTests.cpp | 2 + .../clangd/unittests/TweakTests.cpp | 6 +-- 4 files changed, 30 insertions(+), 27 deletions(-) diff --git a/clang-tools-extra/clangd/Selection.cpp b/clang-tools-extra/clangd/Selection.cpp index 1877b1cf7b67..8451c988ab66 100644 --- a/clang-tools-extra/clangd/Selection.cpp +++ b/clang-tools-extra/clangd/Selection.cpp @@ -61,13 +61,13 @@ public: // Associates any tokens overlapping [Begin, End) with an AST node. // Tokens that were already claimed by another AST node are not claimed again. - // Returns whether the node is selected in the sense of SelectionTree. - SelectionTree::Selection claim(unsigned Begin, unsigned End) { + // Updates Result if the node is selected in the sense of SelectionTree. + void claim(unsigned Begin, unsigned End, SelectionTree::Selection &Result) { assert(Begin <= End); // Fast-path for missing the selection entirely. if (Begin >= SelEnd || End <= SelBegin) - return SelectionTree::Unselected; + return; // We will consider the range (at least partially) selected if it hit any // selected and previously unclaimed token. @@ -98,9 +98,13 @@ public: } } - if (!ClaimedAnyToken) - return SelectionTree::Unselected; - return PartialSelection ? SelectionTree::Partial : SelectionTree::Complete; + // If some tokens were previously claimed (Result != Unselected), we may + // upgrade from Partial->Complete, even if no new tokens were claimed. + // Important for [[int a]]. + if (ClaimedAnyToken || Result) { + Result = std::max(Result, PartialSelection ? SelectionTree::Partial + : SelectionTree::Complete); + } } private: @@ -162,7 +166,9 @@ public: assert(V.Stack.size() == 1 && "Unpaired push/pop?"); assert(V.Stack.top() == &V.Nodes.front()); // We selected TUDecl if tokens were unclaimed (or the file is empty). - if (V.Nodes.size() == 1 || V.Claimed.claim(Begin, End)) { + SelectionTree::Selection UnclaimedTokens = SelectionTree::Unselected; + V.Claimed.claim(Begin, End, UnclaimedTokens); + if (UnclaimedTokens || V.Nodes.size() == 1) { StringRef FileContent = AST.getSourceManager().getBufferData(File); // Don't require the trailing newlines to be selected. bool SelectedAll = Begin == 0 && End >= FileContent.rtrim().size(); @@ -333,10 +339,11 @@ private: Nodes.emplace_back(); Nodes.back().ASTNode = std::move(Node); Nodes.back().Parent = Stack.top(); - // Early hit detection never selects the whole node. Stack.push(&Nodes.back()); - Nodes.back().Selected = - claimRange(Early) ? SelectionTree::Partial : SelectionTree::Unselected; + claimRange(Early, Nodes.back().Selected); + // Early hit detection never selects the whole node. + if (Nodes.back().Selected) + Nodes.back().Selected = SelectionTree::Partial; } // Pops a node off the ancestor stack, and finalizes it. Pairs with push(). @@ -344,8 +351,7 @@ private: void pop() { Node &N = *Stack.top(); dlog("{1}pop: {0}", printNodeToString(N.ASTNode, PrintPolicy), indent(-1)); - if (auto Sel = claimRange(N.ASTNode.getSourceRange())) - N.Selected = Sel; + claimRange(N.ASTNode.getSourceRange(), N.Selected); if (N.Selected || !N.Children.empty()) { // Attach to the tree. N.Parent->Children.push_back(&N); @@ -375,9 +381,10 @@ private: // Perform hit-testing of a complete Node against the selection. // This runs for every node in the AST, and must be fast in common cases. // This is usually called from pop(), so we can take children into account. - SelectionTree::Selection claimRange(SourceRange S) { + // The existing state of Result is relevant (early/late claims can interact). + void claimRange(SourceRange S, SelectionTree::Selection &Result) { if (!S.isValid()) - return SelectionTree::Unselected; + return; // toHalfOpenFileRange() allows selection of constructs in macro args. e.g: // #define LOOP_FOREVER(Body) for(;;) { Body } // void IncrementLots(int &x) { @@ -391,17 +398,16 @@ private: auto E = SM.getDecomposedLoc(Range->getEnd()); // Otherwise, nodes in macro expansions can't be selected. if (B.first != SelFile || E.first != SelFile) - return SelectionTree::Unselected; + return; // Attempt to claim the remaining range. If there's nothing to claim, only // children were selected. - SelectionTree::Selection Result = Claimed.claim(B.second, E.second); + Claimed.claim(B.second, E.second, Result); if (Result) dlog("{1}hit selection: {0}", SourceRange(SM.getComposedLoc(B.first, B.second), SM.getComposedLoc(E.first, E.second)) .printToString(SM), indent()); - return Result; } std::string indent(int Offset = 0) { diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp index 1532c951a26b..e9f617e2dc73 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp @@ -113,15 +113,12 @@ const Node *getParentOfRootStmts(const Node *CommonAnc) { return nullptr; switch (CommonAnc->Selected) { case SelectionTree::Selection::Unselected: + // Typicaly a block, with the { and } unselected, could also be ForStmt etc // Ensure all Children are RootStmts. return llvm::all_of(CommonAnc->Children, isRootStmt) ? CommonAnc : nullptr; case SelectionTree::Selection::Partial: - // Treat Partially selected VarDecl as completely selected since - // SelectionTree doesn't always select VarDecls correctly. - // FIXME: Remove this after D66872 is upstream) - if (!CommonAnc->ASTNode.get()) - return nullptr; - LLVM_FALLTHROUGH; + // Only a fully-selected single statement can be selected. + return nullptr; case SelectionTree::Selection::Complete: // If the Common Ancestor is completely selected, then it's a root statement // and its parent will be unselected. diff --git a/clang-tools-extra/clangd/unittests/SelectionTests.cpp b/clang-tools-extra/clangd/unittests/SelectionTests.cpp index 964f0e98e286..e2cbff37b3db 100644 --- a/clang-tools-extra/clangd/unittests/SelectionTests.cpp +++ b/clang-tools-extra/clangd/unittests/SelectionTests.cpp @@ -354,6 +354,8 @@ TEST(SelectionTest, Selected) { #define ECHO(X) X ECHO(EC^HO([[$C[[int]]) EC^HO(a]])); ]])cpp", + R"cpp( $C[[^$C[[int]] a^]]; )cpp", + R"cpp( $C[[^$C[[int]] a = $C[[5]]^]]; )cpp", }; for (const char *C : Cases) { Annotations Test(C); diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp index c3384af08222..6234fb78c1ee 100644 --- a/clang-tools-extra/clangd/unittests/TweakTests.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -540,13 +540,12 @@ TEST_F(ExtractFunctionTest, FunctionTest) { EXPECT_EQ(apply("int x = 0; [[x++;]]"), "unavailable"); // We don't support extraction from lambdas. EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable"); + // Partial statements aren't extracted. + EXPECT_THAT(apply("int [[x = 0]];"), "unavailable"); // Ensure that end of Zone and Beginning of PostZone being adjacent doesn't // lead to break being included in the extraction zone. EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted")); - // FIXME: This should be unavailable since partially selected but - // selectionTree doesn't always work correctly for VarDecls. - EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted")); // FIXME: ExtractFunction should be unavailable inside loop construct // initalizer/condition. EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted")); @@ -554,7 +553,6 @@ TEST_F(ExtractFunctionTest, FunctionTest) { EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail")); // Don't extract return EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail")); - } TEST_F(ExtractFunctionTest, FileTest) {