Skip to content

Commit b0ed5be

Browse files
sureyeaahsureyeaah
committed
[Clangd] Fixed ExtractVariable for certain types of Exprs
Summary: - Modified ExtractVariable for extraction of MemberExpr, DeclRefExpr and Assignment Expr - Removed extraction from label statements. - Fixed unittests Reviewers: sammccall, kadircet Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D64717 llvm-svn: 366869
1 parent 09e6304 commit b0ed5be

File tree

2 files changed

+148
-57
lines changed

2 files changed

+148
-57
lines changed

‎clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp

Lines changed: 84 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
#include "refactor/Tweak.h"
1414
#include "clang/AST/ASTContext.h"
1515
#include "clang/AST/Expr.h"
16+
#include "clang/AST/ExprCXX.h"
1617
#include "clang/AST/OperationKinds.h"
1718
#include "clang/AST/RecursiveASTVisitor.h"
1819
#include "clang/AST/Stmt.h"
@@ -77,29 +78,15 @@ computeReferencedDecls(const clang::Expr *Expr) {
7778
return Visitor.ReferencedDecls;
7879
}
7980

80-
// An expr is not extractable if it's null or an expression of type void
81-
// FIXME: Ignore assignment (a = 1) Expr since it is extracted as dummy = a =
82-
static bool isExtractableExpr(const clang::Expr *Expr) {
83-
if (Expr) {
84-
const Type *ExprType = Expr->getType().getTypePtrOrNull();
85-
// FIXME: check if we need to cover any other types
86-
if (ExprType)
87-
return !ExprType->isVoidType();
88-
}
89-
return false;
90-
}
91-
9281
ExtractionContext::ExtractionContext(const SelectionTree::Node *Node,
9382
const SourceManager &SM,
9483
const ASTContext &Ctx)
9584
: ExprNode(Node), SM(SM), Ctx(Ctx) {
9685
Expr = Node->ASTNode.get<clang::Expr>();
97-
if (isExtractableExpr(Expr)) {
98-
ReferencedDecls = computeReferencedDecls(Expr);
99-
InsertionPoint = computeInsertionPoint();
100-
if (InsertionPoint)
101-
Extractable = true;
102-
}
86+
ReferencedDecls = computeReferencedDecls(Expr);
87+
InsertionPoint = computeInsertionPoint();
88+
if (InsertionPoint)
89+
Extractable = true;
10390
}
10491

10592
// checks whether extracting before InsertionPoint will take a
@@ -121,9 +108,9 @@ bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
121108
// the current Stmt. We ALWAYS insert before a Stmt whose parent is a
122109
// CompoundStmt
123110
//
124-
125-
// FIXME: Extraction from switch and case statements
111+
// FIXME: Extraction from label, switch and case statements
126112
// FIXME: Doens't work for FoldExpr
113+
// FIXME: Ensure extraction from loops doesn't change semantics.
127114
const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
128115
// returns true if we can extract before InsertionPoint
129116
auto CanExtractOutside =
@@ -141,8 +128,7 @@ const clang::Stmt *ExtractionContext::computeInsertionPoint() const {
141128
return isa<AttributedStmt>(Stmt) || isa<CompoundStmt>(Stmt) ||
142129
isa<CXXForRangeStmt>(Stmt) || isa<DeclStmt>(Stmt) ||
143130
isa<DoStmt>(Stmt) || isa<ForStmt>(Stmt) || isa<IfStmt>(Stmt) ||
144-
isa<LabelStmt>(Stmt) || isa<ReturnStmt>(Stmt) ||
145-
isa<WhileStmt>(Stmt);
131+
isa<ReturnStmt>(Stmt) || isa<WhileStmt>(Stmt);
146132
}
147133
if (InsertionPoint->ASTNode.get<VarDecl>())
148134
return true;
@@ -209,21 +195,23 @@ class ExtractVariable : public Tweak {
209195
return "Extract subexpression to variable";
210196
}
211197
Intent intent() const override { return Refactor; }
198+
// Compute the extraction context for the Selection
199+
bool computeExtractionContext(const SelectionTree::Node *N,
200+
const SourceManager &SM, const ASTContext &Ctx);
212201

213202
private:
214203
// the expression to extract
215204
std::unique_ptr<ExtractionContext> Target;
216205
};
217206
REGISTER_TWEAK(ExtractVariable)
218207
bool ExtractVariable::prepare(const Selection &Inputs) {
208+
// we don't trigger on empty selections for now
209+
if (Inputs.SelectionBegin == Inputs.SelectionEnd)
210+
return false;
219211
const ASTContext &Ctx = Inputs.AST.getASTContext();
220212
const SourceManager &SM = Inputs.AST.getSourceManager();
221213
const SelectionTree::Node *N = Inputs.ASTSelection.commonAncestor();
222-
// we don't trigger on empty selections for now
223-
if (!N || Inputs.SelectionBegin == Inputs.SelectionEnd)
224-
return false;
225-
Target = llvm::make_unique<ExtractionContext>(N, SM, Ctx);
226-
return Target->isExtractable();
214+
return computeExtractionContext(N, SM, Ctx);
227215
}
228216

229217
Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
@@ -239,6 +227,75 @@ Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {
239227
return Effect::applyEdit(Result);
240228
}
241229

230+
// Find the CallExpr whose callee is an ancestor of the DeclRef
231+
const SelectionTree::Node *getCallExpr(const SelectionTree::Node *DeclRef) {
232+
// we maintain a stack of all exprs encountered while traversing the
233+
// selectiontree because the callee of the callexpr can be an ancestor of the
234+
// DeclRef. e.g. Callee can be an ImplicitCastExpr.
235+
std::vector<const clang::Expr *> ExprStack;
236+
for (auto *CurNode = DeclRef; CurNode; CurNode = CurNode->Parent) {
237+
const Expr *CurExpr = CurNode->ASTNode.get<Expr>();
238+
if (const CallExpr *CallPar = CurNode->ASTNode.get<CallExpr>()) {
239+
// check whether the callee of the callexpr is present in Expr stack.
240+
if (std::find(ExprStack.begin(), ExprStack.end(), CallPar->getCallee()) !=
241+
ExprStack.end())
242+
return CurNode;
243+
return nullptr;
244+
}
245+
ExprStack.push_back(CurExpr);
246+
}
247+
return nullptr;
248+
}
249+
250+
// check if Expr can be assigned to a variable i.e. is non-void type
251+
bool canBeAssigned(const SelectionTree::Node *ExprNode) {
252+
const clang::Expr *Expr = ExprNode->ASTNode.get<clang::Expr>();
253+
if (const Type *ExprType = Expr->getType().getTypePtrOrNull())
254+
// FIXME: check if we need to cover any other types
255+
return !ExprType->isVoidType();
256+
return true;
257+
}
258+
259+
// Find the node that will form our ExtractionContext.
260+
// We don't want to trigger for assignment expressions and variable/field
261+
// DeclRefs. For function/member function, we want to extract the entire
262+
// function call.
263+
bool ExtractVariable::computeExtractionContext(const SelectionTree::Node *N,
264+
const SourceManager &SM,
265+
const ASTContext &Ctx) {
266+
if (!N)
267+
return false;
268+
const clang::Expr *SelectedExpr = N->ASTNode.get<clang::Expr>();
269+
const SelectionTree::Node *TargetNode = N;
270+
if (!SelectedExpr)
271+
return false;
272+
// Extracting Exprs like a = 1 gives dummy = a = 1 which isn't useful.
273+
if (const BinaryOperator *BinOpExpr =
274+
dyn_cast_or_null<BinaryOperator>(SelectedExpr)) {
275+
if (BinOpExpr->getOpcode() == BinaryOperatorKind::BO_Assign)
276+
return false;
277+
}
278+
// For function and member function DeclRefs, we look for a parent that is a
279+
// CallExpr
280+
if (const DeclRefExpr *DeclRef =
281+
dyn_cast_or_null<DeclRefExpr>(SelectedExpr)) {
282+
// Extracting just a variable isn't that useful.
283+
if (!isa<FunctionDecl>(DeclRef->getDecl()))
284+
return false;
285+
TargetNode = getCallExpr(N);
286+
}
287+
if (const MemberExpr *Member = dyn_cast_or_null<MemberExpr>(SelectedExpr)) {
288+
// Extracting just a field member isn't that useful.
289+
if (!isa<CXXMethodDecl>(Member->getMemberDecl()))
290+
return false;
291+
TargetNode = getCallExpr(N);
292+
}
293+
if (!TargetNode || !canBeAssigned(TargetNode))
294+
return false;
295+
Target = llvm::make_unique<ExtractionContext>(TargetNode, SM, Ctx);
296+
return Target->isExtractable();
297+
}
298+
242299
} // namespace
243300
} // namespace clangd
244301
} // namespace clang

‎clang-tools-extra/clangd/unittests/TweakTests.cpp

Lines changed: 64 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -291,18 +291,23 @@ TEST(TweakTest, DumpRecordLayout) {
291291
const char *Input = "struct ^X { int x; int y; }";
292292
EXPECT_THAT(getMessage(ID, Input), ::testing::HasSubstr("0 | int x"));
293293
}
294+
294295
TEST(TweakTest, ExtractVariable) {
295296
llvm::StringLiteral ID = "ExtractVariable";
296297
checkAvailable(ID, R"cpp(
297-
int xyz() {
298+
int xyz(int a = 1) {
299+
struct T {
300+
int bar(int a = 1);
301+
int z;
302+
} t;
298303
// return statement
299-
return [[1]];
304+
return [[[[t.b[[a]]r]](t.z)]];
300305
}
301306
void f() {
302307
int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
303308
// multivariable initialization
304309
if(1)
305-
int x = [[1]], y = [[a]] + 1, a = [[1]], z = a + 1;
310+
int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
306311
// if without else
307312
if([[1]])
308313
a = [[1]];
@@ -316,13 +321,13 @@ TEST(TweakTest, ExtractVariable) {
316321
a = [[4]];
317322
else
318323
a = [[5]];
319-
// for loop
324+
// for loop
320325
for(a = [[1]]; a > [[[[3]] + [[4]]]]; a++)
321326
a = [[2]];
322-
// while
327+
// while
323328
while(a < [[1]])
324-
[[a]]++;
325-
// do while
329+
[[a++]];
330+
// do while
326331
do
327332
a = [[1]];
328333
while(a < [[3]]);
@@ -332,36 +337,46 @@ TEST(TweakTest, ExtractVariable) {
332337
checkNotAvailable(ID, R"cpp(
333338
template<typename T, typename ...Args>
334339
struct Test<T, Args...> {
335-
Test(const T &v) :val(^) {}
340+
Test(const T &v) :val[[(^]]) {}
336341
T val;
337342
};
338343
)cpp");
339344
checkNotAvailable(ID, R"cpp(
340345
int xyz(int a = [[1]]) {
341-
return 1;
342-
class T {
343-
T(int a = [[1]]) {};
344-
int xyz = [[1]];
345-
};
346+
struct T {
347+
int bar(int a = [[1]]);
348+
int z = [[1]];
349+
} t;
350+
return [[t]].bar([[[[t]].z]]);
346351
}
352+
void v() { return; }
347353
// function default argument
348354
void f(int b = [[1]]) {
349355
// empty selection
350356
int a = ^1 ^+ ^2;
351357
// void expressions
352358
auto i = new int, j = new int;
353359
[[[[delete i]], delete j]];
360+
[[v]]();
354361
// if
355362
if(1)
356363
int x = 1, y = a + 1, a = 1, z = [[a + 1]];
357364
if(int a = 1)
358-
if([[a]] == 4)
365+
if([[a + 1]] == 4)
359366
a = [[[[a]] +]] 1;
360-
// for loop
361-
for(int a = 1, b = 2, c = 3; [[a]] > [[b + c]]; [[a]]++)
367+
// for loop
368+
for(int a = 1, b = 2, c = 3; a > [[b + c]]; [[a++]])
362369
a = [[a + 1]];
363-
// lambda
370+
// lambda
364371
auto lamb = [&[[a]], &[[b]]](int r = [[1]]) {return 1;}
372+
// assigment
373+
[[a = 5]];
374+
// Variable DeclRefExpr
375+
a = [[b]];
376+
// label statement
377+
goto label;
378+
label:
379+
a = [[1]];
365380
}
366381
)cpp");
367382
// vector of pairs of input and output strings
@@ -397,6 +412,15 @@ TEST(TweakTest, ExtractVariable) {
397412
break;
398413
}
399414
})cpp"},*/
415+
// Macros
416+
{R"cpp(#define PLUS(x) x++
417+
void f(int a) {
418+
PLUS([[1+a]]);
419+
})cpp",
420+
R"cpp(#define PLUS(x) x++
421+
void f(int a) {
422+
auto dummy = PLUS(1+a); dummy;
423+
})cpp"},
400424
// ensure InsertionPoint isn't inside a macro
401425
{R"cpp(#define LOOP(x) while (1) {a = x;}
402426
void f(int a) {
@@ -425,25 +449,35 @@ TEST(TweakTest, ExtractVariable) {
425449
auto dummy = 3; if(1)
426450
LOOP(5 + dummy)
427451
})cpp"},
428-
// label and attribute testing
452+
// attribute testing
429453
{R"cpp(void f(int a) {
430-
label: [ [gsl::suppress("type")] ] for (;;) a = [[1]];
454+
[ [gsl::suppress("type")] ] for (;;) a = [[1]];
431455
})cpp",
432456
R"cpp(void f(int a) {
433-
auto dummy = 1; label: [ [gsl::suppress("type")] ] for (;;) a = dummy;
457+
auto dummy = 1; [ [gsl::suppress("type")] ] for (;;) a = dummy;
434458
})cpp"},
435-
// macro testing
436-
{R"cpp(#define PLUS(x) x++
437-
void f(int a) {
438-
PLUS([[a]]);
459+
// MemberExpr
460+
{R"cpp(class T {
461+
T f() {
462+
return [[T().f()]].f();
463+
}
464+
};)cpp",
465+
R"cpp(class T {
466+
T f() {
467+
auto dummy = T().f(); return dummy.f();
468+
}
469+
};)cpp"},
470+
// Function DeclRefExpr
471+
{R"cpp(int f() {
472+
return [[f]]();
439473
})cpp",
440-
R"cpp(#define PLUS(x) x++
441-
void f(int a) {
442-
auto dummy = a; PLUS(dummy);
474+
R"cpp(int f() {
475+
auto dummy = f(); return dummy;
443476
})cpp"},
444-
// FIXME: Doesn't work correctly for \[\[clang::uninitialized\]\] int
445-
// b = [[1]]; since the attr is inside the DeclStmt and the bounds of
446-
// DeclStmt don't cover the attribute
477+
478+
// FIXME: Wrong result for \[\[clang::uninitialized\]\] int b = [[1]];
479+
// since the attr is inside the DeclStmt and the bounds of
480+
// DeclStmt don't cover the attribute.
447481
};
448482
for (const auto &IO : InputOutputs) {
449483
checkTransform(ID, IO.first, IO.second);

0 commit comments

Comments
 (0)