[clang-tools-extra] r368500 - [clangd] Disallow extraction of expression-statements.
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Aug 9 16:40:54 PDT 2019
Author: sammccall
Date: Fri Aug 9 16:40:54 2019
New Revision: 368500
URL: http://llvm.org/viewvc/llvm-project?rev=368500&view=rev
Log:
[clangd] Disallow extraction of expression-statements.
Summary:
I split out the "extract parent instead of this" logic from the "this isn't
worth extracting" logic (now in eligibleForExtraction()), because I found it
hard to reason about.
While here, handle overloaded as well as builtin assignment operators.
Also this uncovered a bug in getCallExpr() which I fixed.
Reviewers: SureYeaah
Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, cfe-commits
Tags: #clang
Differential Revision: https://reviews.llvm.org/D65337
Modified:
clang-tools-extra/trunk/clangd/Selection.cpp
clang-tools-extra/trunk/clangd/Selection.h
clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
Modified: clang-tools-extra/trunk/clangd/Selection.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Selection.cpp?rev=368500&r1=368499&r2=368500&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Selection.cpp (original)
+++ clang-tools-extra/trunk/clangd/Selection.cpp Fri Aug 9 16:40:54 2019
@@ -510,5 +510,11 @@ const SelectionTree::Node &SelectionTree
return *this;
}
+const SelectionTree::Node &SelectionTree::Node::outerImplicit() const {
+ if (Parent && Parent->ASTNode.getSourceRange() == ASTNode.getSourceRange())
+ return Parent->outerImplicit();
+ return *this;
+}
+
} // namespace clangd
} // namespace clang
Modified: clang-tools-extra/trunk/clangd/Selection.h
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/Selection.h?rev=368500&r1=368499&r2=368500&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/Selection.h (original)
+++ clang-tools-extra/trunk/clangd/Selection.h Fri Aug 9 16:40:54 2019
@@ -110,6 +110,9 @@ public:
// If this node is a wrapper with no syntax (e.g. implicit cast), return
// its contents. (If multiple wrappers are present, unwraps all of them).
const Node& ignoreImplicit() const;
+ // If this node is inside a wrapper with no syntax (e.g. implicit cast),
+ // return that wrapper. (If multiple are present, unwraps all of them).
+ const Node& outerImplicit() const;
};
// The most specific common ancestor of all the selected nodes.
// Returns nullptr if the common ancestor is the root.
Modified: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp?rev=368500&r1=368499&r2=368500&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp (original)
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractVariable.cpp Fri Aug 9 16:40:54 2019
@@ -368,32 +368,80 @@ Expected<Tweak::Effect> ExtractVariable:
return Effect::applyEdit(Result);
}
-// Find the CallExpr whose callee is an ancestor of the DeclRef
+// Find the CallExpr whose callee is the (possibly wrapped) DeclRef
const SelectionTree::Node *getCallExpr(const SelectionTree::Node *DeclRef) {
- // we maintain a stack of all exprs encountered while traversing the
- // selectiontree because the callee of the callexpr can be an ancestor of the
- // DeclRef. e.g. Callee can be an ImplicitCastExpr.
- std::vector<const clang::Expr *> ExprStack;
- for (auto *CurNode = DeclRef; CurNode; CurNode = CurNode->Parent) {
- const Expr *CurExpr = CurNode->ASTNode.get<Expr>();
- if (const CallExpr *CallPar = CurNode->ASTNode.get<CallExpr>()) {
- // check whether the callee of the callexpr is present in Expr stack.
- if (std::find(ExprStack.begin(), ExprStack.end(), CallPar->getCallee()) !=
- ExprStack.end())
- return CurNode;
- return nullptr;
- }
- ExprStack.push_back(CurExpr);
- }
- return nullptr;
+ const SelectionTree::Node &MaybeCallee = DeclRef->outerImplicit();
+ const SelectionTree::Node *MaybeCall = MaybeCallee.Parent;
+ if (!MaybeCall)
+ return nullptr;
+ const CallExpr *CE =
+ llvm::dyn_cast_or_null<CallExpr>(MaybeCall->ASTNode.get<Expr>());
+ if (!CE)
+ return nullptr;
+ if (CE->getCallee() != MaybeCallee.ASTNode.get<Expr>())
+ return nullptr;
+ return MaybeCall;
}
-// check if Expr can be assigned to a variable i.e. is non-void type
-bool canBeAssigned(const SelectionTree::Node *ExprNode) {
- const clang::Expr *Expr = ExprNode->ASTNode.get<clang::Expr>();
- if (const Type *ExprType = Expr->getType().getTypePtrOrNull())
- // FIXME: check if we need to cover any other types
- return !ExprType->isVoidType();
+// Returns true if Inner (which is a direct child of Outer) is appearing as
+// a statement rather than an expression whose value can be used.
+bool childExprIsStmt(const Stmt *Outer, const Expr *Inner) {
+ if (!Outer || !Inner)
+ return false;
+ // Blacklist the most common places where an expr can appear but be unused.
+ if (llvm::isa<CompoundStmt>(Outer))
+ return true;
+ if (llvm::isa<SwitchCase>(Outer))
+ return true;
+ // Control flow statements use condition etc, but not the body.
+ if (const auto* WS = llvm::dyn_cast<WhileStmt>(Outer))
+ return Inner == WS->getBody();
+ if (const auto* DS = llvm::dyn_cast<DoStmt>(Outer))
+ return Inner == DS->getBody();
+ if (const auto* FS = llvm::dyn_cast<ForStmt>(Outer))
+ return Inner == FS->getBody();
+ if (const auto* FS = llvm::dyn_cast<CXXForRangeStmt>(Outer))
+ return Inner == FS->getBody();
+ if (const auto* IS = llvm::dyn_cast<IfStmt>(Outer))
+ return Inner == IS->getThen() || Inner == IS->getElse();
+ // Assume all other cases may be actual expressions.
+ // This includes the important case of subexpressions (where Outer is Expr).
+ return false;
+}
+
+// check if N can and should be extracted (e.g. is not void-typed).
+bool eligibleForExtraction(const SelectionTree::Node *N) {
+ const Expr *E = N->ASTNode.get<Expr>();
+ if (!E)
+ return false;
+
+ // Void expressions can't be assigned to variables.
+ if (const Type *ExprType = E->getType().getTypePtrOrNull())
+ if (ExprType->isVoidType())
+ return false;
+
+ // A plain reference to a name (e.g. variable) isn't worth extracting.
+ // FIXME: really? What if it's e.g. `std::is_same<void, void>::value`?
+ if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+ return false;
+
+ // Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
+ // FIXME: we could still hoist the assignment, and leave the variable there?
+ ParsedBinaryOperator BinOp;
+ if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
+ return false;
+
+ // We don't want to extract expressions used as statements, that would leave
+ // a `dummy;` around that has no effect.
+ // Unfortunately because the AST doesn't have ExprStmt, we have to check in
+ // this roundabout way.
+ const SelectionTree::Node &OuterImplicit = N->outerImplicit();
+ if (!OuterImplicit.Parent ||
+ childExprIsStmt(OuterImplicit.Parent->ASTNode.get<Stmt>(),
+ OuterImplicit.ASTNode.get<Expr>()))
+ return false;
+
+ // FIXME: ban extracting the RHS of an assignment: `a = [[foo()]]`
return true;
}
@@ -406,32 +454,13 @@ bool ExtractVariable::computeExtractionC
const ASTContext &Ctx) {
if (!N)
return false;
- const clang::Expr *SelectedExpr = N->ASTNode.get<clang::Expr>();
const SelectionTree::Node *TargetNode = N;
- if (!SelectedExpr)
- return false;
- // Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
- if (const BinaryOperator *BinOpExpr =
- dyn_cast_or_null<BinaryOperator>(SelectedExpr)) {
- if (BinOpExpr->isAssignmentOp())
- return false;
- }
- // For function and member function DeclRefs, we look for a parent that is a
- // CallExpr
- if (const DeclRefExpr *DeclRef =
- dyn_cast_or_null<DeclRefExpr>(SelectedExpr)) {
- // Extracting just a variable isn't that useful.
- if (!isa<FunctionDecl>(DeclRef->getDecl()))
- return false;
- TargetNode = getCallExpr(N);
- }
- if (const MemberExpr *Member = dyn_cast_or_null<MemberExpr>(SelectedExpr)) {
- // Extracting just a field member isn't that useful.
- if (!isa<CXXMethodDecl>(Member->getMemberDecl()))
- return false;
- TargetNode = getCallExpr(N);
- }
- if (!TargetNode || !canBeAssigned(TargetNode))
+ // For function and member function DeclRefs, extract the whole call.
+ if (const Expr *E = N->ASTNode.get<clang::Expr>())
+ if (llvm::isa<DeclRefExpr>(E) || llvm::isa<MemberExpr>(E))
+ if (const SelectionTree::Node *Call = getCallExpr(N))
+ TargetNode = Call;
+ if (!eligibleForExtraction(TargetNode))
return false;
Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
return Target->isExtractable();
Modified: clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp?rev=368500&r1=368499&r2=368500&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/SelectionTests.cpp Fri Aug 9 16:40:54 2019
@@ -361,6 +361,8 @@ TEST(SelectionTest, Implicit) {
EXPECT_EQ("CXXConstructExpr", nodeKind(Str->Parent->Parent));
EXPECT_EQ(Str, &Str->Parent->Parent->ignoreImplicit())
<< "Didn't unwrap " << nodeKind(&Str->Parent->Parent->ignoreImplicit());
+
+ EXPECT_EQ("CXXConstructExpr", nodeKind(&Str->outerImplicit()));
}
} // namespace
Modified: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp?rev=368500&r1=368499&r2=368500&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp Fri Aug 9 16:40:54 2019
@@ -249,7 +249,7 @@ TEST(TweaksTest, ExtractVariable) {
a = [[2]];
// while
while(a < [[1]])
- [[a++]];
+ a = [[1]];
// do while
do
a = [[1]];
@@ -293,11 +293,14 @@ TEST(TweaksTest, ExtractVariable) {
// lambda
auto lamb = [&[[a]], &[[b]]](int r = [[1]]) {return 1;}
// assigment
- [[a = 5]];
- [[a >>= 5]];
- [[a *= 5]];
+ xyz([[a = 5]]);
+ xyz([[a *= 5]]);
// Variable DeclRefExpr
a = [[b]];
+ // statement expression
+ [[xyz()]];
+ while (a)
+ [[++a]];
// label statement
goto label;
label:
@@ -340,7 +343,7 @@ TEST(TweaksTest, ExtractVariable) {
// Macros
{R"cpp(#define PLUS(x) x++
void f(int a) {
- PLUS([[1+a]]);
+ int y = PLUS([[1+a]]);
})cpp",
/*FIXME: It should be extracted like this.
R"cpp(#define PLUS(x) x++
@@ -349,7 +352,7 @@ TEST(TweaksTest, ExtractVariable) {
})cpp"},*/
R"cpp(#define PLUS(x) x++
void f(int a) {
- auto dummy = PLUS(1+a); dummy;
+ auto dummy = PLUS(1+a); int y = dummy;
})cpp"},
// ensure InsertionPoint isn't inside a macro
{R"cpp(#define LOOP(x) while (1) {a = x;}
More information about the cfe-commits
mailing list