[clang-tools-extra] 2ff4493 - [clangd] Reduce availability of extract function
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 9 01:16:30 PDT 2020
Author: Kadir Cetinkaya
Date: 2020-10-09T10:12:48+02:00
New Revision: 2ff44935a5f56f755a4f47ad00ee0dec48e19832
URL: https://github.com/llvm/llvm-project/commit/2ff44935a5f56f755a4f47ad00ee0dec48e19832
DIFF: https://github.com/llvm/llvm-project/commit/2ff44935a5f56f755a4f47ad00ee0dec48e19832.diff
LOG: [clangd] Reduce availability of extract function
This patch introduces hoisting detection logic into prepare state with
a partial AST traversal of the enclosing function.
We had some complaints from the users about this code action being almost always
available but failing most of the time. Hopefully this should reduce that noise.
The latency/correctness tradeoff is a bunch of hand-waving, but at least today
we don't have any other actions that are available on selection of statements,
so when we get to do the traversal it is quite likely that all the other checks
will bail out early. But this is still up for discussion, I am happy to abandon
the patch if you believe this is not practical.
Differential Revision: https://reviews.llvm.org/D85354
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 1ba8c3c1d9ff..18fe7bf39160 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -47,6 +47,7 @@
//===----------------------------------------------------------------------===//
#include "AST.h"
+#include "FindTarget.h"
#include "ParsedAST.h"
#include "Selection.h"
#include "SourceCode.h"
@@ -54,6 +55,7 @@
#include "support/Logger.h"
#include "clang/AST/ASTContext.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/AST/RecursiveASTVisitor.h"
#include "clang/AST/Stmt.h"
@@ -65,6 +67,8 @@
#include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/iterator_range.h"
@@ -152,6 +156,9 @@ struct ExtractionZone {
const FunctionDecl *EnclosingFunction = nullptr;
// The half-open file range of the enclosing function.
SourceRange EnclosingFuncRange;
+ // Set of statements that form the ExtractionZone.
+ llvm::DenseSet<const Stmt *> RootStmts;
+
SourceLocation getInsertionPoint() const {
return EnclosingFuncRange.getBegin();
}
@@ -159,10 +166,45 @@ struct ExtractionZone {
// The last root statement is important to decide where we need to insert a
// semicolon after the extraction.
const Node *getLastRootStmt() const { return Parent->Children.back(); }
- void generateRootStmts();
-private:
- llvm::DenseSet<const Stmt *> RootStmts;
+ // Checks if declarations inside extraction zone are accessed afterwards.
+ //
+ // This performs a partial AST traversal proportional to the size of the
+ // enclosing function, so it is possibly expensive.
+ bool requiresHoisting(const SourceManager &SM) const {
+ // First find all the declarations that happened inside extraction zone.
+ llvm::SmallSet<const Decl *, 1> DeclsInExtZone;
+ for (auto *RootStmt : RootStmts) {
+ findExplicitReferences(RootStmt,
+ [&DeclsInExtZone](const ReferenceLoc &Loc) {
+ if (!Loc.IsDecl)
+ return;
+ DeclsInExtZone.insert(Loc.Targets.front());
+ });
+ }
+ // Early exit without performing expensive traversal below.
+ if (DeclsInExtZone.empty())
+ return false;
+ // Then make sure they are not used outside the zone.
+ for (const auto *S : EnclosingFunction->getBody()->children()) {
+ if (SM.isBeforeInTranslationUnit(S->getSourceRange().getEnd(),
+ ZoneRange.getEnd()))
+ continue;
+ bool HasPostUse = false;
+ findExplicitReferences(S, [&](const ReferenceLoc &Loc) {
+ if (HasPostUse ||
+ SM.isBeforeInTranslationUnit(Loc.NameLoc, ZoneRange.getEnd()))
+ return;
+ HasPostUse =
+ llvm::any_of(Loc.Targets, [&DeclsInExtZone](const Decl *Target) {
+ return DeclsInExtZone.contains(Target);
+ });
+ });
+ if (HasPostUse)
+ return true;
+ }
+ return false;
+ }
};
// Whether the code in the extraction zone is guaranteed to return, assuming
@@ -185,12 +227,6 @@ bool ExtractionZone::isRootStmt(const Stmt *S) const {
return RootStmts.find(S) != RootStmts.end();
}
-// Generate RootStmts set
-void ExtractionZone::generateRootStmts() {
- for (const Node *Child : Parent->Children)
- RootStmts.insert(Child->ASTNode.get<Stmt>());
-}
-
// Finds the function in which the zone lies.
const FunctionDecl *findEnclosingFunction(const Node *CommonAnc) {
// Walk up the SelectionTree until we find a function Decl
@@ -281,7 +317,10 @@ llvm::Optional<ExtractionZone> findExtractionZone(const Node *CommonAnc,
ExtZone.ZoneRange = *ZoneRange;
if (ExtZone.EnclosingFuncRange.isInvalid() || ExtZone.ZoneRange.isInvalid())
return llvm::None;
- ExtZone.generateRootStmts();
+
+ for (const Node *Child : ExtZone.Parent->Children)
+ ExtZone.RootStmts.insert(Child->ASTNode.get<Stmt>());
+
return ExtZone;
}
@@ -670,16 +709,18 @@ tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc,
}
bool ExtractFunction::prepare(const Selection &Inputs) {
- const Node *CommonAnc = Inputs.ASTSelection.commonAncestor();
- const SourceManager &SM = Inputs.AST->getSourceManager();
const LangOptions &LangOpts = Inputs.AST->getLangOpts();
if (!LangOpts.CPlusPlus)
return false;
- if (auto MaybeExtZone = findExtractionZone(CommonAnc, SM, LangOpts)) {
- ExtZone = std::move(*MaybeExtZone);
- return true;
- }
- return false;
+ const Node *CommonAnc = Inputs.ASTSelection.commonAncestor();
+ const SourceManager &SM = Inputs.AST->getSourceManager();
+ auto MaybeExtZone = findExtractionZone(CommonAnc, SM, LangOpts);
+ // FIXME: Get rid of this check once we support hoisting.
+ if (!MaybeExtZone || MaybeExtZone->requiresHoisting(SM))
+ return false;
+
+ ExtZone = std::move(*MaybeExtZone);
+ return true;
}
Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {
diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index 7a217220c384..f64d42a7eed7 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -593,6 +593,8 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
EXPECT_EQ(apply("auto lam = [](){ [[int x;]] }; "), "unavailable");
// Partial statements aren't extracted.
EXPECT_THAT(apply("int [[x = 0]];"), "unavailable");
+ // FIXME: Support hoisting.
+ EXPECT_THAT(apply(" [[int a = 5;]] a++; "), "unavailable");
// Ensure that end of Zone and Beginning of PostZone being adjacent doesn't
// lead to break being included in the extraction zone.
@@ -600,8 +602,6 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
// FIXME: ExtractFunction should be unavailable inside loop construct
// initializer/condition.
EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
- // Don't extract because needs hoisting.
- EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
// Extract certain return
EXPECT_THAT(apply(" if(true) [[{ return; }]] "), HasSubstr("extracted"));
// Don't extract uncertain return
More information about the cfe-commits
mailing list