diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractVariable.cpp --- 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 @@ 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 @@ 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(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 @@ 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 @@ 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 @@ 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::value`? - if (llvm::isa(E) || llvm::isa(E)) + if (llvm::isa(E)) return false; + // Similarly disallow extraction for member exprs with an implicit `this`. + if (const auto *ME = dyn_cast(E)) + if (const auto *TE = dyn_cast(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 @@ 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 --- a/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp +++ b/clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp @@ -24,7 +24,7 @@ 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 @@ 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 @@ 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 @@ EXPECT_UNAVAILABLE(UnavailableCases); // vector of pairs of input and output strings - const std::vector> InputOutputs = { + std::vector> InputOutputs = { // extraction from variable declaration/assignment {R"cpp(void varDecl() { int a = 5 * (4 + (3 [[- 1)]]); @@ -291,6 +304,99 @@ 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