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 @@ -50,6 +50,7 @@ private: bool Extractable = false; const clang::Expr *Expr; + std::string Type; const SelectionTree::Node *ExprNode; // Stmt before which we will extract const clang::Stmt *InsertionPoint = nullptr; @@ -61,6 +62,7 @@ bool exprIsValidOutside(const clang::Stmt *Scope) const; // computes the Stmt before which we will extract out Expr const clang::Stmt *computeInsertionPoint() const; + std::string computeExprType(const clang::Expr *E) const; }; // Returns all the Decls referenced inside the given Expr @@ -90,6 +92,9 @@ InsertionPoint = computeInsertionPoint(); if (InsertionPoint) Extractable = true; + Type = computeExprType(Expr); + if (Type.empty()) + Extractable = false; } // checks whether extracting before InsertionPoint will take a @@ -156,6 +161,33 @@ return nullptr; } +std::string +ExtractionContext::computeExprType(const clang::Expr *E) const { + if (Ctx.getLangOpts().CPlusPlus) { + return "auto"; + } + QualType ExprType = E->getType(); + if (E->hasPlaceholderType(BuiltinType::PseudoObject)) { + if (const auto *PR = dyn_cast(E)) { + 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 ""; + } else if (PR->isMessagingGetter()) { + if (PR->isExplicitProperty()) + ExprType = PR->getExplicitProperty()->getType(); + else + ExprType = PR->getImplicitPropertyGetter()->getReturnType(); + } + } + } + // Avoid including outer nullability on local variables. + AttributedType::stripOuterNullability(ExprType); + return ExprType.getAsString(); +} + // returns the replacement for substituting the extraction with VarName tooling::Replacement ExtractionContext::replaceWithVar(SourceRange Chars, @@ -173,9 +205,8 @@ 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 = + Type + " " + VarName.str() + " = " + ExtractionCode.str() + "; "; return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl); } @@ -365,9 +396,15 @@ 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().CPlusPlus && + !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`? @@ -460,10 +497,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 @@ -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( @@ -123,7 +134,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 +302,78 @@ 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 = { + // We don't preserve types through typedefs. + // FIXME: Ideally we could preserve the non-canonical type. + {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