[clang-tools-extra] ae67984 - [clangd] ExtractVariable support for C and Objective-C
David Goldman via cfe-commits
cfe-commits at lists.llvm.org
Tue May 31 08:14:59 PDT 2022
Author: David Goldman
Date: 2022-05-31T11:14:51-04:00
New Revision: ae67984ca6d89c7ccdbdca258cd05c151d8b6431
URL: https://github.com/llvm/llvm-project/commit/ae67984ca6d89c7ccdbdca258cd05c151d8b6431
DIFF: https://github.com/llvm/llvm-project/commit/ae67984ca6d89c7ccdbdca258cd05c151d8b6431.diff
LOG: [clangd] ExtractVariable support for C and Objective-C
- Use the expression's type for non-C++ as the variable type. This works
well, but might not preserve the typedefs due to type
canonicalization.
- Improve support for Objective-C property references which are
represented using `ObjCPropertyRefExpr` and `BuiltinType::PseudoObject`.
Differential Revision: https://reviews.llvm.org/D124486
Added:
Modified:
clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
index 942d7eec6dbd8..5efe09fb3cb27 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp
@@ -5,6 +5,7 @@
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
+#include "AST.h"
#include "ParsedAST.h"
#include "Protocol.h"
#include "Selection.h"
@@ -50,6 +51,7 @@ class ExtractionContext {
private:
bool Extractable = false;
const clang::Expr *Expr;
+ QualType VarType;
const SelectionTree::Node *ExprNode;
// Stmt before which we will extract
const clang::Stmt *InsertionPoint = nullptr;
@@ -81,6 +83,31 @@ computeReferencedDecls(const clang::Expr *Expr) {
return Visitor.ReferencedDecls;
}
+static QualType computeVariableType(const Expr *Expr, const ASTContext &Ctx) {
+ if (Ctx.getLangOpts().CPlusPlus11)
+ return Ctx.getAutoDeductType();
+
+ if (Expr->hasPlaceholderType(BuiltinType::PseudoObject)) {
+ if (const auto *PR = dyn_cast<ObjCPropertyRefExpr>(Expr)) {
+ if (PR->isMessagingSetter()) {
+ // Don't support extracting a compound reference like `self.prop += 1`
+ // since the meaning changes after extraction since we'll no longer call
+ // the setter. Non compound access like `self.prop = 1` is invalid since
+ // it returns nil (setter method must have a void return type).
+ return QualType();
+ } else if (PR->isMessagingGetter()) {
+ if (PR->isExplicitProperty())
+ return PR->getExplicitProperty()->getType();
+ else
+ return PR->getImplicitPropertyGetter()->getReturnType();
+ }
+ } else {
+ return QualType();
+ }
+ }
+ return Expr->getType();
+}
+
ExtractionContext::ExtractionContext(const SelectionTree::Node *Node,
const SourceManager &SM,
const ASTContext &Ctx)
@@ -90,6 +117,12 @@ ExtractionContext::ExtractionContext(const SelectionTree::Node *Node,
InsertionPoint = computeInsertionPoint();
if (InsertionPoint)
Extractable = true;
+ VarType = computeVariableType(Expr, Ctx);
+ if (VarType.isNull())
+ Extractable = false;
+ else
+ // Strip the outer nullability since it's not common for local variables.
+ AttributedType::stripOuterNullability(VarType);
}
// checks whether extracting before InsertionPoint will take a
@@ -173,9 +206,9 @@ ExtractionContext::insertDeclaration(llvm::StringRef VarName,
toHalfOpenFileRange(SM, Ctx.getLangOpts(),
InsertionPoint->getSourceRange())
->getBegin();
- // FIXME: Replace auto with explicit type and add &/&& as necessary
- std::string ExtractedVarDecl = std::string("auto ") + VarName.str() + " = " +
- ExtractionCode.str() + "; ";
+ std::string ExtractedVarDecl =
+ printType(VarType, ExprNode->getDeclContext(), VarName) + " = " +
+ ExtractionCode.str() + "; ";
return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
}
@@ -365,15 +398,27 @@ bool eligibleForExtraction(const SelectionTree::Node *N) {
return false;
// Void expressions can't be assigned to variables.
- if (const Type *ExprType = E->getType().getTypePtrOrNull())
- if (ExprType->isVoidType())
- return false;
+ const Type *ExprType = E->getType().getTypePtrOrNull();
+ if (!ExprType || ExprType->isVoidType())
+ return false;
+
+ // Must know the type of the result in order to spell it, or instead use
+ // `auto` in C++.
+ if (!N->getDeclContext().getParentASTContext().getLangOpts().CPlusPlus11 &&
+ !ExprType)
+ 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))
+ if (llvm::isa<DeclRefExpr>(E))
return false;
+ // Similarly disallow extraction for member exprs with an implicit `this`.
+ if (const auto *ME = dyn_cast<MemberExpr>(E))
+ if (const auto *TE = dyn_cast<CXXThisExpr>(ME->getBase()->IgnoreImpCasts()))
+ if (TE->isImplicit())
+ return false;
+
// Extracting Exprs like a = 1 gives placeholder = a = 1 which isn't useful.
// FIXME: we could still hoist the assignment, and leave the variable there?
ParsedBinaryOperator BinOp;
@@ -460,10 +505,6 @@ bool ExtractVariable::prepare(const Selection &Inputs) {
if (Inputs.SelectionBegin == Inputs.SelectionEnd)
return false;
const ASTContext &Ctx = Inputs.AST->getASTContext();
- // FIXME: Enable non-C++ cases once we start spelling types explicitly instead
- // of making use of auto.
- if (!Ctx.getLangOpts().CPlusPlus)
- return false;
const SourceManager &SM = Inputs.AST->getSourceManager();
if (const SelectionTree::Node *N =
computeExtractedExpr(Inputs.ASTSelection.commonAncestor()))
diff --git a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
index 9740ea2d7098c..bd06f318b2a99 100644
--- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
+++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp
@@ -24,7 +24,7 @@ TEST_F(ExtractVariableTest, Test) {
int z;
} t;
// return statement
- return [[[[t.b[[a]]r]](t.z)]];
+ return [[[[t.b[[a]]r]]([[t.z]])]];
}
void f() {
int a = [[5 +]] [[4 * [[[[xyz]]()]]]];
@@ -59,11 +59,22 @@ TEST_F(ExtractVariableTest, Test) {
EXPECT_AVAILABLE(AvailableCases);
ExtraArgs = {"-xc"};
- const char *AvailableButC = R"cpp(
+ const char *AvailableC = R"cpp(
void foo() {
int x = [[1]];
})cpp";
- EXPECT_UNAVAILABLE(AvailableButC);
+ EXPECT_AVAILABLE(AvailableC);
+ ExtraArgs = {"-xobjective-c"};
+ const char *AvailableObjC = R"cpp(
+ __attribute__((objc_root_class))
+ @interface Foo
+ @end
+ @implementation Foo
+ - (void)method {
+ int x = [[1 + 2]];
+ }
+ @end)cpp";
+ EXPECT_AVAILABLE(AvailableObjC);
ExtraArgs = {};
const char *NoCrashCases = R"cpp(
@@ -79,10 +90,12 @@ TEST_F(ExtractVariableTest, Test) {
const char *UnavailableCases = R"cpp(
int xyz(int a = [[1]]) {
struct T {
- int bar(int a = [[1]]);
+ int bar(int a = [[1]]) {
+ int b = [[z]];
+ }
int z = [[1]];
} t;
- return [[t]].bar([[[[t]].z]]);
+ return [[t]].bar([[t]].z);
}
void v() { return; }
// function default argument
@@ -123,7 +136,7 @@ TEST_F(ExtractVariableTest, Test) {
EXPECT_UNAVAILABLE(UnavailableCases);
// vector of pairs of input and output strings
- const std::vector<std::pair<std::string, std::string>> InputOutputs = {
+ std::vector<std::pair<std::string, std::string>> InputOutputs = {
// extraction from variable declaration/assignment
{R"cpp(void varDecl() {
int a = 5 * (4 + (3 [[- 1)]]);
@@ -291,6 +304,99 @@ TEST_F(ExtractVariableTest, Test) {
for (const auto &IO : InputOutputs) {
EXPECT_EQ(IO.second, apply(IO.first)) << IO.first;
}
+
+ ExtraArgs = {"-xobjective-c"};
+ EXPECT_UNAVAILABLE(R"cpp(
+ __attribute__((objc_root_class))
+ @interface Foo
+ - (void)setMethod1:(int)a;
+ - (int)method1;
+ @property int prop1;
+ @end
+ @implementation Foo
+ - (void)method {
+ [[self.method1]] = 1;
+ [[self.method1]] += 1;
+ [[self.prop1]] = 1;
+ [[self.prop1]] += 1;
+ }
+ @end)cpp");
+ InputOutputs = {
+ // Function Pointers
+ {R"cpp(struct Handlers {
+ void (*handlerFunc)(int);
+ };
+ void runFunction(void (*func)(int)) {}
+ void f(struct Handlers *handler) {
+ runFunction([[handler->handlerFunc]]);
+ })cpp",
+ R"cpp(struct Handlers {
+ void (*handlerFunc)(int);
+ };
+ void runFunction(void (*func)(int)) {}
+ void f(struct Handlers *handler) {
+ void (*placeholder)(int) = handler->handlerFunc; runFunction(placeholder);
+ })cpp"},
+ {R"cpp(int (*foo(char))(int);
+ void bar() {
+ (void)[[foo('c')]];
+ })cpp",
+ R"cpp(int (*foo(char))(int);
+ void bar() {
+ int (*placeholder)(int) = foo('c'); (void)placeholder;
+ })cpp"},
+ {R"cpp(typedef long NSInteger;
+ void varDecl() {
+ NSInteger a = 2 * 5;
+ NSInteger b = [[a * 7]] + 3;
+ })cpp",
+ R"cpp(typedef long NSInteger;
+ void varDecl() {
+ NSInteger a = 2 * 5;
+ long placeholder = a * 7; NSInteger b = placeholder + 3;
+ })cpp"},
+ // Support ObjC property references (explicit property getter).
+ {R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ @property int prop1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int x = [[self.prop1]] + 1;
+ }
+ @end)cpp",
+ R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ @property int prop1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int placeholder = self.prop1; int x = placeholder + 1;
+ }
+ @end)cpp"},
+ // Support ObjC property references (implicit property getter).
+ {R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ - (int)method1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int x = [[self.method1]] + 1;
+ }
+ @end)cpp",
+ R"cpp(__attribute__((objc_root_class))
+ @interface Foo
+ - (int)method1;
+ @end
+ @implementation Foo
+ - (void)method {
+ int placeholder = self.method1; int x = placeholder + 1;
+ }
+ @end)cpp"},
+ };
+ for (const auto &IO : InputOutputs) {
+ EXPECT_EQ(IO.second, apply(IO.first)) << IO.first;
+ }
}
} // namespace
More information about the cfe-commits
mailing list