kadircet created this revision.
kadircet added a reviewer: sammccall.
Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous.
Herald added a project: clang.
kadircet requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
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.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D85354
Files:
clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -592,6 +592,8 @@
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.
@@ -599,8 +601,6 @@
// 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
Index: clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
===================================================================
--- clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ 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,7 @@
#include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
#include "llvm/ADT/None.h"
#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/iterator_range.h"
@@ -152,6 +155,9 @@
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 +165,6 @@
// 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;
};
// Whether the code in the extraction zone is guaranteed to return, assuming
@@ -185,12 +187,6 @@
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 +277,46 @@
ExtZone.ZoneRange = *ZoneRange;
if (ExtZone.EnclosingFuncRange.isInvalid() || ExtZone.ZoneRange.isInvalid())
return llvm::None;
- ExtZone.generateRootStmts();
+
+ // Make sure declarations inside extraction zone are not accessed afterwards.
+ // This performs a partial AST traversal proportional to the size of the
+ // enclosing function, so it is possibly expensive. But, if we made it so far
+ // this is likely to be only action we are going to offer, so performing a
+ // single AST traversal shouldn't be too bad.
+ // FIXME: Get rid of the following checks once we have support for hoisting.
+
+ // We first figure out all the declarations that happened inside extraction
+ // zone.
+ llvm::SmallSet<const Decl *, 1> DeclsInExtZone;
+ for (const Node *Child : ExtZone.Parent->Children) {
+ auto *RootStmt = Child->ASTNode.get<Stmt>();
+ findExplicitReferences(RootStmt,
+ [&DeclsInExtZone](const ReferenceLoc &Loc) {
+ if (!Loc.IsDecl)
+ return;
+ DeclsInExtZone.insert(Loc.Targets.front());
+ });
+ ExtZone.RootStmts.insert(RootStmt);
+ }
+ // Early exit without performing expensive traversal below.
+ if (DeclsInExtZone.empty())
+ return ExtZone;
+ // Then make sure they are not used outside the zone.
+ for (const auto *S : ExtZone.EnclosingFunction->getBody()->children()) {
+ if (SM.isBeforeInTranslationUnit(S->getSourceRange().getEnd(),
+ ExtZone.ZoneRange.getEnd()))
+ continue;
+ bool HasPostUse = false;
+ findExplicitReferences(S, [&](const ReferenceLoc &Loc) {
+ if (HasPostUse ||
+ SM.isBeforeInTranslationUnit(Loc.NameLoc, ExtZone.ZoneRange.getEnd()))
+ return;
+ for (const auto *Target : Loc.Targets)
+ HasPostUse = HasPostUse || DeclsInExtZone.contains(Target);
+ });
+ if (HasPostUse)
+ return llvm::None;
+ }
return ExtZone;
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
