[clangd] ExtractVariable support for C and Objective-C

[dgoldman]

Summary

• 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.

[dgoldman]

Minor formatting fix

[sammccall]

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

193

(does this need to be a method?)

194

CPlusPlus11

197

if it has pseudo-object type and *isn't* handled by the ObjCPropertyRef case, we should disallow extraction right?

198

E->getObjCProperty()?

204

please don't use "" as a sentinel value, it will lead to bugs

Why are we checking this here rather than in eligibleForExtraction?

210

Type + Varname isn't correct for types in general, particularly those that use declarator syntax.
printType with varname as the placeholder should do what you want.

Can you add a testcase where the variable has function pointer type?

215

prefer printType from AST.h, which will get qualifiers, anonymous namespaces etc right

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

326

I'd drop the FIXME here: the fully-sugared type type of NSInteger * long is long, not NSInteger.

(Unless you're suggesting changing this in clang)

[dgoldman]

• Don't use a method for computing the type
• Compute the type using printType
• Support MemberExprs and add a test for function pointers

[dgoldman]

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

198

Hmm I'm not sure when that would be useful looking at the logic there, can you give an example just to make sure I understand what it would handle?

204

I put it in the ExtractionContext constructor since we would otherwise have to perform the same logic twice to compute the type string. I could make it into a function to be shared by both if you'd like?

210

I went ahead and added this but I couldn't think of a way to get a result like this without enabling this for MemberExprs, so I went ahead and re-enabled for them since I think it could be useful for them. LMK if I should revert that change. I guess if we don't want to support MemberExprs I should disable the ObjC property support since it probably makes to match the C++ equivalent?

[dgoldman]

• Remove extraneous computeExprType declaration

[sammccall]

Ok, this looks pretty good to me! Just nits

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

54

just QualType, it's internally nullable already
(some value in being explicit, but the code below handling two different null values is more confusing imo)

And this is more clearly VarType - it's the type you intend for the variable, and it's not necessarily the type of the extracted expression (see pseudo-object stuff)

120

sorry, please do pull this back out into a function

(my question was just can this be a function rather than a method, it seemed self-contained)

122

If you set ExprType to Ctx.getAutoDeductType(), I think you can avoid treating this as a special case when printing

124

nit: ExprType->getTypePtrOrNull() as a boolean is more directly !ExprType->isNull()

198

it's similar to the cast you have, but in addition to handling foo.bar it handles parens like (foo.bar), commas like (0, foo.bar) and casts. All together I think this covers more cases where a pseudo-object can be used as an lvalue and potentially modified.

210

Try:

int (*foo(char))(int);
void bar() {
  (void)foo('c');
}

extracting foo('c') to subexpr should yield int (*placeholder)(int) = foo('c');

374

The reason MemberExpr is banned is that we don't want to suggest extracting foo if it happens to be a member.
You can leave it banned, or allow it but disallow the case where the MemberExpr's base is a CXXThisExpr which is implicit.

402

I'd just change this to if (!ExprType || ExprType->isVoidType())

I don't think there's a real null-type case that does anything useful.

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

340

This comment is inaccurate: if the expression has type NSInteger, that code will be written.

The issue is that the type of the expression isn't NSInteger, it's just int. arithmetic doesn't produce typedef types in the way I think you're expecting.

If the expression was something that was an NSInteger (like a call to a function whose declared return type is NSInteger) then that type would be preserved.

[dgoldman]

• Extract var type handling to a function
• Other fixes for review

[dgoldman]

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

198

Gotcha, thanks. That makes sense, but it doesn't look like that has any usages and I'm not sure if it's safe to call from this context (e.g. if we can have a non ObjC property PseudoObject expr). I guess I could modify getObjCProperty to return nullptr in those invalid cases, WDYT?

210

Thanks, that worked

374

Went with the latter, but LMK. It does seem odd to me that the same expression is allowed if it's explicit - maybe we should just allow always, even if it's implicit? Xcode allows it, FWIW.

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

340

Gotcha, thanks. Deleted the comment, should I delete this test case as well?

[dgoldman]

Friendly ping

[sammccall]

Thank you!

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

418

oops, I forgot one detail: we want ME->getBase()->IgnoreImpCasts()

(in void nonConstMethod() { constMethod(); } there's an ImplicitCastExpr in there...

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

326

can you move the function pointer + NSInteger tests into a separate block with ExtraArgs = {"-xc"}?

340

Up to you, I think the test is fine.
I'd add a comment "arithmetic on typedef types yields plain integer types" or so to explain what it's about.

[dgoldman]

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

198

@sammccall WDYT about this? Leave as is?

[dgoldman]

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

418

Hmm, is this right, I tested out that example and it seems like that's actually a CXXMemberCallExpr which this doesn't handle?

[sammccall]

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

198

Yeah, that function is not ideal. Leaving as-is sounds fine to me.

418

The callee of the CXXMemberCallExpr is the MemberExpr i'm talking about here.

[dgoldman]

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

418

Right but this code here doesn't handle CXXMemberCallExpr, only MemberExpr - so we don't even see the callee here. If I add the following above:

if (const auto *MCE = dyn_cast<CXXMemberCallExpr>(E))
  E = MCE->getCallee();

and do ME->getBase()->IgnoreImpCasts() then:

class Test {
  int constMethod() const { return 1; }
  int nonConstMethod() { return [[constMethod()]]; }
};

will be unavailable.

[sammccall]

Please go ahead and land this, the remaining open comment doesn't really matter.

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

418

Right but this code here doesn't handle CXXMemberCallExpr, only MemberExpr - so we don't even see the callee here.

I don't understand what you mean.

If you select int nonConstMethod() { return [[constMethod]](); }, then isn't N such a MemberExpr?

[dgoldman]

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

418

Nope, still seems to be CXXMemberCallExpr. I'll go ahead and land this with ME->getBase()->IgnoreImpCasts() check although the example isn't working as expected.

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

 //===--- ExtractVariable.cpp ------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+#include "AST.h"
 #include "ParsedAST.h"
 #include "Protocol.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
tooling::Replacement replaceWithVar(SourceRange Chars,
                                       llvm::StringRef VarName) const;
   // Generate Replacement for declaring the selected Expr as a new variable
   tooling::Replacement insertDeclaration(llvm::StringRef VarName,
                                          SourceRange InitChars) const;
 
 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;
   const SourceManager &SM;
   const ASTContext &Ctx;
   // Decls referenced in the Expr
   std::vector<clang::Decl *> ReferencedDecls;
   // returns true if the Expr doesn't reference any variable declared in scope
bool VisitDeclRefExpr(DeclRefExpr *DeclRef) { // NOLINT
       return true;
     }
   };
   FindDeclRefsVisitor Visitor;
   Visitor.TraverseStmt(const_cast<Stmt *>(cast<Stmt>(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)
     : ExprNode(Node), SM(SM), Ctx(Ctx) {
   Expr = Node->ASTNode.get<clang::Expr>();
   ReferencedDecls = computeReferencedDecls(Expr);
   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
 // variable reference out of scope
 bool ExtractionContext::exprIsValidOutside(const clang::Stmt *Scope) const {
   SourceLocation ScopeBegin = Scope->getBeginLoc();
   SourceLocation ScopeEnd = Scope->getEndLoc();
   for (const Decl *ReferencedDecl : ReferencedDecls) {
if (const clang::Stmt *CurParent = CurNode->Parent->ASTNode.get<Stmt>()) {
         return CurInsertionPoint;
       }
     }
   }
   return nullptr;
 }
 
 // returns the replacement for substituting the extraction with VarName
 tooling::Replacement
 ExtractionContext::replaceWithVar(SourceRange Chars,
                                   llvm::StringRef VarName) const {
   unsigned ExtractionLength =
       SM.getFileOffset(Chars.getEnd()) - SM.getFileOffset(Chars.getBegin());
   return tooling::Replacement(SM, Chars.getBegin(), ExtractionLength, VarName);
 }
 // returns the Replacement for declaring a new variable storing the extraction
 tooling::Replacement
 ExtractionContext::insertDeclaration(llvm::StringRef VarName,
                                      SourceRange InitializerChars) const {
   llvm::StringRef ExtractionCode = toSourceCode(SM, InitializerChars);
   const SourceLocation InsertionLoc =
       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) + " = " +
   return tooling::Replacement(SM, InsertionLoc, 0, ExtractedVarDecl);
 }
 
 // Helpers for handling "binary subexpressions" like a + [[b + c]] + d.
 //
 // These are special, because the formal AST doesn't match what users expect:
 // - the AST is ((a + b) + c) + d, so the ancestor expression is `a + b + c`.
 // - but extracting `b + c` is reasonable, as + is (mathematically) associative.
 //
 // So we try to support these cases with some restrictions:
 //  - the operator must be associative
 //  - no mixing of operators is allowed

 
 // 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;
+  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)
 
   // 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;
   if (BinOp.parse(*N) && BinaryOperator::isAssignmentOp(BinOp.Kind))
     return false;
 
   const SelectionTree::Node &OuterImplicit = N->outerImplicit();
private:
   std::unique_ptr<ExtractionContext> Target;
 };
 REGISTER_TWEAK(ExtractVariable)
 bool ExtractVariable::prepare(const Selection &Inputs) {
   // we don't trigger on empty selections for now
   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()))
     Target = std::make_unique<ExtractionContext>(N, SM, Ctx);
   return Target && Target->isExtractable();
 }
 
 Expected<Tweak::Effect> ExtractVariable::apply(const Selection &Inputs) {

clang-tools-extra/clangd/unittests/tweaks/ExtractVariableTests.cpp

 TEST_F(ExtractVariableTest, Test) {
   const char *AvailableCases = R"cpp(
     int xyz(int a = 1) {
       struct T {
         int bar(int a = 1);
         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]]()]]]];
       // multivariable initialization
       if(1)
         int x = [[1]], y = [[a + 1]], a = [[1]], z = a + 1;
       // if without else
       if([[1]])
void f() {
       do
         a = [[1]] + 1;
       while(a < [[3]]);
     }
   )cpp";
   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(
     // error-ok: broken code, but shouldn't crash
     template<typename T, typename ...Args>
     struct Test<T, Args...> {
     Test(const T &v) :val[[(^]]) {}
       T val;
     };
   )cpp";
   EXPECT_UNAVAILABLE(NoCrashCases);
 
   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
     void f(int b = [[1]]) {
       // empty selection
       int a = ^1 ^+ ^2;
       // void expressions
       auto i = new int, j = new int;
void f(int b = [[1]]) {
       goto label;
       label:
         a = [[1]];
     }
   )cpp";
   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)]]);
                  })cpp",
        R"cpp(void varDecl() {
                    auto placeholder = (3 - 1); int a = 5 * (4 + placeholder);
                  })cpp"},
       // FIXME: extraction from switch case
std::vector<std::pair<std::string, std::string>> InputOutputs = {
        R"cpp(#define ECHO(X) X
                  void f() {
                    auto placeholder = 1 + ECHO(2) + ECHO(3) + 4; int x = placeholder + 5;
                  })cpp"},
   };
   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
 } // namespace clangd
 } // namespace clang