[refactor][extract] Initial implementation of variable captures

[arphaman]

Summary

This patch adds an initial rudimentary support for capturing variables that should be passed to the extracted function.
The variables are passed to the extracted function if they're used in the extracted code. If they're defined they're ignored for now, but we'll be able to handle variables that defined in extracted code but that are used after extracted code later. The variables are sorted by name when passing them to the extracted function. The exact passing semantics (reference/pointer/const reference/const pointer) are not yet computed.

[ioeric]

Implementation looks good. Just some nits.

lib/Tooling/Refactoring/Extract/CaptureVariables.cpp

37

and this?

51

nit: s/Detemine/Determine/

It might make more sense to put these FIXME in class-level comment.

lib/Tooling/Refactoring/Extract/CaptureVariables.h

37

Maybe a FIXME here for Kind?

39

Does this include trailing commas? Maybe add an example in the comment?

Same below.

44

Maybe printFunctionCallArg? It's unclear where this is used.

lib/Tooling/Refactoring/Extract/Extract.cpp

154

nit: Capture.index() > 0 to be more explicit.

[arphaman]

Address review comments

lib/Tooling/Refactoring/Extract/CaptureVariables.cpp

37

This is a different kind of expression that won't be handled in this method, and there's a fixit for 'this' further down already.

lib/Tooling/Refactoring/Extract/CaptureVariables.h

37

I think this constructor will always set the same kind.

[ioeric]

lib/Tooling/Refactoring/Extract/CaptureVariables.cpp

98

A more informative message would be better.

lib/Tooling/Refactoring/Extract/CaptureVariables.h

29

The name is a bit hard to parse. Maybe just CapturedEntity or simply CapturedVar?

37

Then why do you need Kind? Is it changed somewhere else? Please document the behavior,

[arphaman]

Address review comments

lib/Tooling/Refactoring/Extract/CaptureVariables.h

37

The kind will be set to a different value by the other constructors that will be added later
I added docs to the enum

[ioeric]

Lg

(Sorry for losing track of this and the delay!)

lib/Tooling/Refactoring/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS Support)
 
 add_clang_library(clangToolingRefactor
   ASTSelection.cpp
   ASTSelectionRequirements.cpp
   AtomicChange.cpp
+  Extract/CaptureVariables.cpp
   Extract/Extract.cpp
   Extract/SourceExtraction.cpp
   RefactoringActions.cpp
   Rename/RenamingAction.cpp
   Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp

lib/Tooling/Refactoring/Extract/CaptureVariables.h

+//===--- CaptureVariables.cpp - Clang refactoring library -----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_TOOLING_REFACTORING_EXTRACT_CAPTURE_VARIABLES_H
+#define LLVM_CLANG_LIB_TOOLING_REFACTORING_EXTRACT_CAPTURE_VARIABLES_H
+
+#include "clang/Basic/LLVM.h"
+#include <vector>
+
+namespace clang {
+
+struct PrintingPolicy;
+class VarDecl;
+
+namespace tooling {
+
+class CodeRangeASTSelection;
+
+/// Represents a variable or a declaration that can be represented using a
+/// a variable that was captured in the extracted code and that should be
+/// passed to the extracted function as an argument.
+class CapturedEntity {
+  /// The kind of the entity that is captured by the extracted function.
+  enum EntityKind {
+    /// An entity that represents a local variable defined in the callee
+    /// that is captured by the extracted function.
+    CapturedVarDecl,
+
+    // FIXME: Add entity kinds for FieldDecl, this and self.
+  };
+
+public:
+  /// Creates a local variable capture entity.
+  explicit CapturedEntity(const VarDecl *VD) : Kind(CapturedVarDecl), VD(VD) {}
+
+  /// Print the parameter declaration for the captured entity.
+  ///
+  /// The declaration includes the type and the name of the parameter, and
+  /// doesn't include the trailing comma. This is a example of the output:
+  ///
+  ///   int *capture
+  void printParamDecl(llvm::raw_ostream &OS, const PrintingPolicy &PP) const;
+
+  /// Print the expression that should be used as an argument value when
+  /// calling the extracted function.
+  ///
+  /// The expression includes the name of the captured entity whose address
+  /// could be taken if needed. The trailing comma is not included. This is a
+  /// example of the output:
+  ///
+  ///   &capture
+  void printFunctionCallArg(llvm::raw_ostream &OS,
+                            const PrintingPolicy &PP) const;
+
+  StringRef getName() const;
+
+private:
+  EntityKind Kind;
+  union {
+    const VarDecl *VD;
+  };
+  // FIXME: Track things like type & qualifiers.
+};
+
+/// Scans the extracted AST to determine which variables have to be captured
+/// and passed to the extracted function.
+std::vector<CapturedEntity>
+findCapturedExtractedEntities(const CodeRangeASTSelection &Code);
+
+} // end namespace tooling
+} // end namespace clang
+
+#endif // LLVM_CLANG_LIB_TOOLING_REFACTORING_EXTRACT_CAPTURE_VARIABLES_H

lib/Tooling/Refactoring/Extract/CaptureVariables.cpp

+//===--- CapturedVariables.cpp - Clang refactoring library ----------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "CaptureVariables.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/Tooling/Refactoring/ASTSelection.h"
+#include "llvm/ADT/DenseMap.h"
+
+using namespace clang;
+
+namespace {
+
+// FIXME: Determine 'const' qualifier.
+// FIXME: Track variables defined in the extracted code.
+/// Scans the extracted AST to determine which variables have to be captured
+/// and passed to the extracted function.
+class ExtractedVariableCaptureVisitor
+    : public RecursiveASTVisitor<ExtractedVariableCaptureVisitor> {
+public:
+  struct ExtractedEntityInfo {
+    /// True if this entity is used in extracted code.
+    bool IsUsed = false;
+    /// True if this entity is defined in the extracted code.
+    bool IsDefined = false;
+  };
+
+  bool VisitDeclRefExpr(const DeclRefExpr *E) {
+    const VarDecl *VD = dyn_cast<VarDecl>(E->getDecl());
+    if (!VD)
+      return true;
+    // FIXME: Capture 'self'.
+    if (!VD->isLocalVarDeclOrParm())
+      return true;
+    captureVariable(VD);
+    return true;
+  }
+
+  bool VisitVarDecl(const VarDecl *VD) {
+    captureVariable(VD).IsDefined = true;
+    // FIXME: Track more information about variables defined in extracted code
+    // to support "use after defined in extracted" situation reasonably well.
+    return true;
+  }
+
+  const llvm::DenseMap<const VarDecl *, ExtractedEntityInfo> &
+  getVisitedVariables() const {
+    return Variables;
+  }
+
+private:
+  ExtractedEntityInfo &captureVariable(const VarDecl *VD) {
+    ExtractedEntityInfo &Result = Variables[VD];
+    Result.IsUsed = true;
+    return Result;
+  }
+
+  llvm::DenseMap<const VarDecl *, ExtractedEntityInfo> Variables;
+  // TODO: Track fields/this/self.
+};
+
+} // end anonymous namespace
+
+namespace clang {
+namespace tooling {
+
+void CapturedEntity::printParamDecl(llvm::raw_ostream &OS,
+                                    const PrintingPolicy &PP) const {
+  switch (Kind) {
+  case CapturedVarDecl:
+    VD->getType().print(OS, PP, /*PlaceHolder=*/VD->getName());
+    break;
+  }
+}
+
+void CapturedEntity::printFunctionCallArg(llvm::raw_ostream &OS,
+                                          const PrintingPolicy &PP) const {
+  // FIXME: Take address if needed.
+  switch (Kind) {
+  case CapturedVarDecl:
+    OS << VD->getName();
+    break;
+  }
+}
+
+StringRef CapturedEntity::getName() const {
+  switch (Kind) {
+  case CapturedVarDecl:
+    return VD->getName();
+  }
+  llvm_unreachable("invalid captured entity kind");
+}
+
+/// Scans the extracted AST to determine which variables have to be captured
+/// and passed to the extracted function.
+std::vector<CapturedEntity>
+findCapturedExtractedEntities(const CodeRangeASTSelection &Code) {
+  ExtractedVariableCaptureVisitor Visitor;
+  for (size_t I = 0, E = Code.size(); I != E; ++I)
+    Visitor.TraverseStmt(const_cast<Stmt *>(Code[I]));
+  std::vector<CapturedEntity> Entities;
+  for (const auto &I : Visitor.getVisitedVariables()) {
+    if (!I.getSecond().IsDefined)
+      Entities.push_back(CapturedEntity(I.getFirst()));
+    // FIXME: Handle variables used after definition in extracted code.
+  }
+  // Sort the entities by name.
+  std::sort(Entities.begin(), Entities.end(),
+            [](const CapturedEntity &X, const CapturedEntity &Y) {
+              return X.getName() < Y.getName();
+            });
+  // FIXME: Capture any field if necessary (method -> function extraction).
+  // FIXME: Capture 'this' / 'self' if necessary.
+  // FIXME: Compute the actual parameter types.
+  return Entities;
+}
+
+} // end namespace tooling
+} // end namespace clang

lib/Tooling/Refactoring/Extract/Extract.cpp

 //===--- Extract.cpp - Clang refactoring library --------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// \brief Implements the "extract" refactoring that can pull code into
 /// new functions, methods or declare new variables.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "clang/Tooling/Refactoring/Extract/Extract.h"
+#include "CaptureVariables.h"
 #include "SourceExtraction.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprObjC.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 
 namespace clang {
SourceRange ExtractedRange(Code[0]->getLocStart(),
                              Code[Code.size() - 1]->getLocEnd());
   // FIXME (Alex L): Add code that accounts for macro locations.
 
   ASTContext &AST = Context.getASTContext();
   SourceManager &SM = AST.getSourceManager();
   const LangOptions &LangOpts = AST.getLangOpts();
   Rewriter ExtractedCodeRewriter(SM, LangOpts);
 
-  // FIXME: Capture used variables.
+  std::vector<CapturedEntity> Captures = findCapturedExtractedEntities(Code);
 
   // Compute the return type.
   QualType ReturnType = AST.VoidTy;
   // FIXME (Alex L): Account for the return statement in extracted code.
   // FIXME (Alex L): Check for lexical expression instead.
   bool IsExpr = Code.size() == 1 && isa<Expr>(Code[0]);
   if (IsExpr) {
     // FIXME (Alex L): Get a more user-friendly type if needed.
     ReturnType = cast<Expr>(Code[0])->getType();
   }
 
   // FIXME: Rewrite the extracted code performing any required adjustments.
 
-  // FIXME: Capture any field if necessary (method -> function extraction).
-
-  // FIXME: Sort captured variables by name.
-
-  // FIXME: Capture 'this' / 'self' if necessary.
-
-  // FIXME: Compute the actual parameter types.
-
   // Compute the location of the extracted declaration.
   SourceLocation ExtractedDeclLocation =
       computeFunctionExtractionLocation(ParentDecl);
   // FIXME: Adjust the location to account for any preceding comments.
 
   // FIXME: Adjust with PP awareness like in Sema to get correct 'bool'
   // treatment.
   PrintingPolicy PP = AST.getPrintingPolicy();
   // FIXME: PP.UseStdFunctionForLambda = true;
   PP.SuppressStrongLifetime = true;
   PP.SuppressLifetimeQualifiers = true;
   PP.SuppressUnwrittenScope = true;
 
   ExtractionSemicolonPolicy Semicolons = ExtractionSemicolonPolicy::compute(
       Code[Code.size() - 1], ExtractedRange, SM, LangOpts);
   AtomicChange Change(SM, ExtractedDeclLocation);
   // Create the replacement for the extracted declaration.
   {
     std::string ExtractedCode;
     llvm::raw_string_ostream OS(ExtractedCode);
     // FIXME: Use 'inline' in header.
     OS << "static ";
     ReturnType.print(OS, PP, DeclName);
     OS << '(';
-    // FIXME: Arguments.
+    for (const auto &Capture : llvm::enumerate(Captures)) {
+      if (Capture.index() > 0)
+        OS << ", ";
+      Capture.value().printParamDecl(OS, PP);
+    }
     OS << ')';
 
     // Function body.
     OS << " {\n";
     if (IsExpr && !ReturnType->isVoidType())
       OS << "return ";
     OS << ExtractedCodeRewriter.getRewrittenText(ExtractedRange);
     if (Semicolons.isNeededInExtractedFunction())
       OS << ';';
     OS << "\n}\n\n";
     auto Err = Change.insert(SM, ExtractedDeclLocation, OS.str());
     if (Err)
       return std::move(Err);
   }
 
   // Create the replacement for the call to the extracted declaration.
   {
     std::string ReplacedCode;
     llvm::raw_string_ostream OS(ReplacedCode);
 
     OS << DeclName << '(';
-    // FIXME: Forward arguments.
+    for (const auto &Capture : llvm::enumerate(Captures)) {
+      if (Capture.index() > 0)
+        OS << ", ";
+      Capture.value().printFunctionCallArg(OS, PP);
+    }
     OS << ')';
     if (Semicolons.isNeededInOriginalFunction())
       OS << ';';
 
     auto Err = Change.replace(
         SM, CharSourceRange::getTokenRange(ExtractedRange), OS.str());
     if (Err)
       return std::move(Err);

test/Refactor/Extract/CaptureSimpleVariables.cpp

+// RUN: clang-refactor extract -selection=test:%s %s -- 2>&1 | grep -v CHECK | FileCheck %s
+
+void captureStaticVars() {
+  static int x;
+  /*range astaticvar=->+0:39*/int y = x;
+  x += 1;
+}
+// CHECK: 1 'astaticvar' results:
+// CHECK:      static void extracted(int x) {
+// CHECK-NEXT: int y = x;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void captureStaticVars() {
+// CHECK-NEXT: static int x;
+// CHECK-NEXT: extracted(x);{{$}}
+
+typedef struct {
+  int width, height;
+} Rectangle;
+
+void basicTypes(int i, float f, char c, const int *ip, float *fp, const Rectangle *structPointer) {
+  /*range basictypes=->+0:73*/basicTypes(i, f, c, ip, fp, structPointer);
+}
+// CHECK: 1 'basictypes' results:
+// CHECK:      static void extracted(char c, float f, float *fp, int i, const int *ip, const Rectangle *structPointer) {
+// CHECK-NEXT: basicTypes(i, f, c, ip, fp, structPointer);{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void basicTypes(int i, float f, char c, const int *ip, float *fp, const Rectangle *structPointer) {
+// CHECK-NEXT: extracted(c, f, fp, i, ip, structPointer);{{$}}
+
+int noGlobalsPlease = 0;
+
+void cantCaptureGlobals() {
+  /*range cantCaptureGlobals=->+0:62*/int y = noGlobalsPlease;
+}
+// CHECK: 1 'cantCaptureGlobals' results:
+// CHECK:      static void extracted() {
+// CHECK-NEXT: int y = noGlobalsPlease;{{$}}
+// CHECK-NEXT: }{{[[:space:]].*}}
+// CHECK-NEXT: void cantCaptureGlobals() {
+// CHECK-NEXT: extracted();{{$}}

test/Refactor/Extract/ExtractionSemicolonPolicy.cpp

 // RUN: clang-refactor extract -selection=test:%s %s -- -std=c++11 -fcxx-exceptions | grep -v CHECK | FileCheck %s
 
 struct Rectangle { int width, height; };
 
 void extractStatement(const Rectangle &r) {
   /*range adeclstmt=->+0:59*/int area = r.width * r.height;
 }
 // CHECK: 1 'adeclstmt' results:
-// CHECK:      static void extracted() {
+// CHECK:      static void extracted(const Rectangle &r) {
 // CHECK-NEXT: int area = r.width * r.height;{{$}}
 // CHECK-NEXT: }{{[[:space:]].*}}
 // CHECK-NEXT: void extractStatement(const Rectangle &r) {
-// CHECK-NEXT:   /*range adeclstmt=->+0:59*/extracted();{{$}}
+// CHECK-NEXT:   /*range adeclstmt=->+0:59*/extracted(r);{{$}}
 // CHECK-NEXT: }
 
 void extractStatementNoSemiIf(const Rectangle &r) {
   /*range bextractif=->+2:4*/if (r.width) {
     int x = r.height;
   }
 }
 // CHECK: 1 'bextractif' results:
-// CHECK:      static void extracted() {
+// CHECK:      static void extracted(const Rectangle &r) {
 // CHECK-NEXT: if (r.width) {
 // CHECK-NEXT: int x = r.height;
 // CHECK-NEXT: }{{$}}
 // CHECK-NEXT: }{{[[:space:]].*}}
 // CHECK-NEXT: void extractStatementNoSemiIf(const Rectangle &r) {
-// CHECK-NEXT:   /*range bextractif=->+2:4*/extracted();{{$}}
+// CHECK-NEXT:   /*range bextractif=->+2:4*/extracted(r);{{$}}
 // CHECK-NEXT: }
 
 void extractStatementDontExtraneousSemi(const Rectangle &r) {
   /*range cextractif=->+2:4*/if (r.width) {
     int x = r.height;
   } ;
 } //^ This semicolon shouldn't be extracted.
 // CHECK: 1 'cextractif' results:
-// CHECK:      static void extracted() {
+// CHECK:      static void extracted(const Rectangle &r) {
 // CHECK-NEXT: if (r.width) {
 // CHECK-NEXT: int x = r.height;
 // CHECK-NEXT: }{{$}}
 // CHECK-NEXT: }{{[[:space:]].*}}
 // CHECK-NEXT: void extractStatementDontExtraneousSemi(const Rectangle &r) {
-// CHECK-NEXT: extracted(); ;{{$}}
+// CHECK-NEXT: extracted(r); ;{{$}}
 // CHECK-NEXT: }
 
 void extractStatementNotSemiSwitch() {
   /*range dextract=->+5:4*/switch (2) {
   case 1:
     break;
   case 2:
     break;
struct XS {
   int *end() { return 0; }
 };
 
 void extractStatementNotSemiRangedFor(XS xs) {
   /*range gextract=->+1:4*/for (int i : xs) {
   }
 }
 // CHECK: 1 'gextract' results:
-// CHECK:      static void extracted() {
+// CHECK:      static void extracted(XS xs) {
 // CHECK-NEXT: for (int i : xs) {
 // CHECK-NEXT: }{{$}}
 // CHECK-NEXT: }{{[[:space:]].*}}
 // CHECK-NEXT: void extractStatementNotSemiRangedFor(XS xs) {
-// CHECK-NEXT: extracted();{{$}}
+// CHECK-NEXT: extracted(xs);{{$}}
 // CHECK-NEXT: }
 
 void extractStatementNotSemiRangedTryCatch() {
   /*range hextract=->+3:4*/try { int x = 0; }
   catch (const int &i) {
     int y = i;
   }
 }

test/Refactor/Extract/ExtractionSemicolonPolicy.m

 // RUN: clang-refactor extract -selection=test:%s %s -- 2>&1 | grep -v CHECK | FileCheck %s
 
 @interface NSArray
 + (id)arrayWithObjects:(const id [])objects count:(unsigned long)cnt;
 @end
 
 void extractStatementNoSemiObjCFor(NSArray *array) {
   /*range astmt=->+2:4*/for (id i in array) {
     int x = 0;
   }
 }
 // CHECK: 1 'astmt' results:
-// CHECK:      static void extracted() {
+// CHECK:      static void extracted(NSArray *array) {
 // CHECK-NEXT: for (id i in array) {
 // CHECK-NEXT: int x = 0;
 // CHECK-NEXT: }{{$}}
 // CHECK-NEXT: }{{[[:space:]].*}}
 
 void extractStatementNoSemiSync() {
   id lock;
   /*range bstmt=->+2:4*/@synchronized(lock) {
     int x = 0;
   }
 }
 // CHECK: 1 'bstmt' results:
-// CHECK:      static void extracted() {
+// CHECK:      static void extracted(id lock) {
 // CHECK-NEXT: @synchronized(lock) {
 // CHECK-NEXT: int x = 0;
 // CHECK-NEXT: }{{$}}
 // CHECK-NEXT: }{{[[:space:]].*}}
 
 void extractStatementNoSemiAutorel() {
   /*range cstmt=->+2:4*/@autoreleasepool {
     int x = 0;