[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
This commit is contained in:
Kadir Cetinkaya 2020-08-05 22:18:39 +02:00
parent efd8c9ed72
commit 2ff44935a5
No known key found for this signature in database
GPG Key ID: E39E36B8D2057ED6
2 changed files with 60 additions and 19 deletions

View File

@ -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) {

View File

@ -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