[clangd] Add support to extract method for ExtractFunction Tweak

[FabioRS]

Summary

I miss more automatically refactoring functions when working with already running code, so I am making some small addition that I hope help more people.

This works by checking if the function is a method (CXXMethodDecl), then collecting information about the function that the code is being extracted, looking for the declaration if it is out-of-line, creating the declaration if it is necessary and putting the extracted function as a class-method.

This is my first code review request, sorry if I did something wrong.

[sammccall]

Thanks for doing this, it looks great!

A few comments on:

• a few on cases that I think aren't handled quite right
• structure of the new code
• structure of the *old* code: names etc that no longer fit after these changes

There's also the ugliness around merging edits/tweak effects - this has come up in a couple of other checks and we should have a nicer way to do it, sorry it's not there yet :-)

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

414

nit: these are *specifiers* rather than attributes

428

and these are *qualifiers* rather than attributes

438

this isn't actually rendering namespaces, right? just classes

442

What if this class is nested?
e.g.

class Foo { class Bar { int baz(); }; };
int Foo::Bar::baz() { ... }

we need to produce void Foo::Bar::extracted() { ... }

I think the easiest thing here is just to save the NestedNameSpecifier* from the original out-of-line definition (getQualifier()), this represents the syntax Foo::Bar:: as written. Then you can just print() it.

464

the naming of this family of functions, and the irregular reuse between them doesn't sit quite right with me, especially the way it suggests "inline definition" is a special case (when for plain functions it's the only case).

What about renderDeclaration(FunctionDeclKind K, DeclContext &Enclosing, const SourceManager &SM);

with enum FunctionDeclKind { InlineDefinition, ForwardDeclaration, OutOfLineDefinition }?,

465

here you're excluding the specifiers, which is correct for static but incorrect for constexpr :-(

473

For out-of-line defined methods, this is the wrong DC, which may lead to the type being incorrectly qualified. The DC should rather be the enclosing class.

Similarly, renderParametersForDefinition uses EnclosingFuncContext too - instead it should take the EnclosingFuncContext as a parameter. (And be renamed renderParametersForDeclaration, since it's no longer just for definitions)

761

Maybe a high-level comment:

For methods defined out-of-line, the extracted method will also be out-of-line. The new declaration/definition will each be next to the existing ones.
Methods defined inline are treated very similarly to plain functions.

768

may want to propagate consteval too
maybe instead of bool Constexpr, store ConstexprSpecKind Constexpr = Method->getConstexprKind()?

769

nit: why is this called ContextConst rather than just const? We can already decide at this point that the new function will be const.

774

I think we should call this EnclosingClass rather than ParentContext, as we only set it/care about it when it's a class, never when it's a namespace

775

There's some redundancy between DeclarationPoint and OutOfLine.

Can we call this one ForwardDeclarationPoint, only set it if the method is out-of-line? Then we can get rid of OutOfLine and just check the presence/absence of ForwardDeclarationPoint.

775

it might be more appropriate to place this elsewhere in the class, e.g. in the private section, among the methods even if we're extracting from a constructor etc. We now have clangd/refactor/InsertionPoint.h to help with this.

I don't think you should do this now (keep the scope small), but might be worth a FIXME

785–786

now that we have multiple insertions, NewFunction::InsertionPoint should be called DefinitionPoint instead

[FabioRS]

I am not sure about the LangOptions object, the NestedNameSpecifier needs it to correctly print the nested classes-name and not sure about the getEnclosing() too.

I can not run the test the consteval, so it is not in the diff.

TestTU failed to build (suppress with /*error-ok*/): 
[1:4-1:13] unknown type name 'consteval'; did you mean 'constexpr'?, fixes: {change 'consteval' to 'constexpr' {1:4-1:13 => "constexpr"}}

For code:

    consteval void SomeClass::f() const {
      int x;
    }

[FabioRS]

Full patch

[sammccall]

Thanks! I'll look at the changes in the morning, just wanted to mention one thing

In D122698#3417946, @FabioRS wrote:

I can not run the test the consteval, so it is not in the diff.

TestTU failed to build (suppress with /*error-ok*/): 
[1:4-1:13] unknown type name 'consteval'; did you mean 'constexpr'?, fixes: {change 'consteval' to 'constexpr' {1:4-1:13 => "constexpr"}}

It's a c++20 feature, so you'll need to add something like ExtraArgs.push_back("-std=c++20");.

Though if there's no real difference between handling constexpr vs consteval, up to you whether it's worth a separate test.

[FabioRS]

In D122698#3418018, @sammccall wrote:

Thanks! I'll look at the changes in the morning, just wanted to mention one thing

In D122698#3417946, @FabioRS wrote:

I can not run the test the consteval, so it is not in the diff.

TestTU failed to build (suppress with /*error-ok*/): 
[1:4-1:13] unknown type name 'consteval'; did you mean 'constexpr'?, fixes: {change 'consteval' to 'constexpr' {1:4-1:13 => "constexpr"}}

It's a c++20 feature, so you'll need to add something like ExtraArgs.push_back("-std=c++20");.

Though if there's no real difference between handling constexpr vs consteval, up to you whether it's worth a separate test.

Thank you!
I think it is interesting to have the test to keep the consteval branch covered. I think maybe there will be more things to change, so I can send the test case for consteval tomorrow with the others changes.

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

473

It is only for out-of-line methods? I put this verification in the getEnclosing() of the new diff.

[sammccall]

OK, I think the main remaining thing is enclosing contexts... (sorry, I wasn't thinking clearly about these in the last round, and I think existing code gets this slightly wrong too).

Each declaration has potentially *two* contexts, semantic and syntactic, and the distinction is relevant here.

namespace ns {
class A {
  void f(); // SemanticDC = A, SyntacticDC = A
};
void A::f() { } // SemanticDC = A, SyntacticDC = ns
}

To determine how to spell different parts of the function, different rules apply to different parts of the declaration:

• return type and function name needs to be relative to the SyntacticDC
• parameters and function body are relative to the SemanticDC

I think renderDeclaration() should take SemanticDC and SyntacticDC as parameters, and NewFunction should have SemanticDC, SyntacticDC, ForwardDeclarationSyntacticDC as fields. (By definition, the forward declaration and the definition have the same semantic DC).

To summarize, something like:

struct NewFunction {
  DeclContext *SemanticDC;
  DeclContext *SyntacticDC;
  DeclContext *ForwardDeclarationSyntacticDC;
}
void renderDeclaration(FunctionDeclKind, DeclContext& SemanticDC, DeclContext &SyntacticDC, ...) {
  ... printType(ReturnType, SyntacticDC), ..., renderParametersForDefinition(SemanticDC)...
}
void createFunctionDefinition(...) {
   renderDeclaration(..., SemanticDC, SyntacticDC, ...);
}
void createForwardDeclaration(...) {
  renderDeclaration(..., SemanticDC, ForwardDeclarationSyntacticDC, ...);
}

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

352

you only ever use the beginning of this range (and it's an insertion point, so having a range doesn't make much sense). Change to SourceLocation?

354

nit: const (the AST isn't great about const-correctness, but should be OK here)

Maybe NestedNameSpec -> DefinitionQualifier?

361

this needs a more specific name - see overall comment on the patch

366

nit: pointer rather than reference, reference members have surprising effects and we don't have any here so far

473

you don't need separate renderDeclaration() and renderDefinition() functions: the FunctionDeclKind tells you whether this is a definition or not.

(And naming-wise, definitions are just a special type of declaration)

473

functions can also be defined in a different syntactic context than they are declared:

namespace ns { void foo(); }
void ns::foo() {}

This is rarer and I suspect we get this case wrong today.

847

nit: createForwardDeclaration

[FabioRS]

Thanks!

namespace ns { int y = 1; void foo(); }
void ns::foo() {[[int x; y++; ]]}

namespace ns { int y = 1; void foo(); }
void extracted() {
int x; y++;
}
void ns::foo() {extracted(); }

This is not working, but I think it's not the scope of this review request, what do you think?
Later today I will take a look at the namespace context problem to verify how much and what code fixes it.

(See the tests line 478)

        class SomeClass {
          static ns::A::C::RType extracted();
static C::RType f();
        };

This is valid code but the syntactic looks strange with all the namespaces/classes, I tested various contexts but was not fixed, do you have some ideia for me to do?

[FabioRS]

I did some simple refactor on the code I did for the function-method and it fixed the namespace syntactic problem too. I am not sure if it expands the pull request scope.

[sammccall]

Thanks, this looks great! Just a couple of nits left.
Appreciate you fixing the out-of-line ns::f() case too. It's easier to understand the fixed logic than the broken logic.

I guess you don't have commit access yet, I can land this for you if you like. Let me know your preferred name/email for attribution.

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

765

You're setting/resetting these in lots of different places, but no need for that:

SyntacticDC = EnclosingFunction->getLexicalDeclContext()
SemanticDC = getDeclContext();

and set ForwardDeclarationSyntacticDC in captureMethodInfo(), leave it null if this isn't a method.

770

this does one check, and is idiomatic in LLVM:

if (const auto *Method = llvm::dyn_cast<...>(...))
  captureMethodInfo(...);

[FabioRS]

In D122698#3426237, @sammccall wrote:

Thanks, this looks great! Just a couple of nits left.
Appreciate you fixing the out-of-line ns::f() case too. It's easier to understand the fixed logic than the broken logic.

I guess you don't have commit access yet, I can land this for you if you like. Let me know your preferred name/email for attribution.

Thanks! I'm glad to help.

I don't have commit access. You can land it for me please, thanks!
Name: Fabio Rossini Sluzala
e-mail: [e-mail]

[FabioRS]

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

765

Thanks!
The out-of-line ns:f() needs the ForwardDeclarationSyntacticDC too, I think inside the branch if (ExtZone.EnclosingFunction->isOutOfLine()) is the place to put it, I will submit a diff.

[FabioRS]

Thanks!

I removed the excess setting/resseting of the SyntacticDC and SemanticDC

ExtractedFunc.SyntacticDC =
    ExtZone.EnclosingFunction->getLexicalDeclContext();
ExtractedFunc.SemanticDC = ExtZone.EnclosingFunction->getDeclContext();

if (const auto *Method =
        llvm::dyn_cast<CXXMethodDecl>(ExtZone.EnclosingFunction))
  captureMethodInfo(ExtractedFunc, Method);

I place the ExtractedFunc.ForwardDeclarationSyntacticDC = ExtractedFunc.SemanticDC inside the if (ExtZone.EnclosingFunction->isOutOfLine()) branch since the out-of-line ns:f() needs it too. What do you think?

[sammccall]

You're right about the out-of-line function case. Current version looks great!
I had to rework the logic around merging the edits a little, it wasn't quite correct and was hitting assertions (not sure if you were seeing these locally, but phabricator is not currently running the clangd tests properly).

Thanks again, landing now.

[FabioRS]

In D122698#3430211, @sammccall wrote:

You're right about the out-of-line function case. Current version looks great!
I had to rework the logic around merging the edits a little, it wasn't quite correct and was hitting assertions (not sure if you were seeing these locally, but phabricator is not currently running the clangd tests properly).

Thanks again, landing now.

Thanks.

Here the tests are passing, maybe something is missing in my compilation setup... There are these unsupported tests, but I was thinking they are for another architeture.

ninja check-clangd
[1/2] Running the Clangd regression tests
llvm-lit: /homessddata/Projects/llvm-project/llvm/utils/lit/lit/llvm/config.py:438: note: using clang: /homessddata/Projects/llvm-project/build/bin/clang

Testing Time: 3.26s
  Unsupported:    6
  Passed     : 1129

You are welcome. Thank you for your patience and time, I am glad to help.

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

 #include "ParsedAST.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "support/Logger.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
+#include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "clang/Tooling/Refactoring/Extract/SourceExtraction.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/raw_os_ostream.h"
 
 namespace clang {
 namespace clangd {
 namespace {
 
 using Node = SelectionTree::Node;
 
 // ExtractionZone is the part of code that is being extracted.
 // EnclosingFunction is the function/method inside which the zone lies.
 // We split the file into 4 parts relative to extraction zone.
 enum class ZoneRelative {
   Before,     // Before Zone and inside EnclosingFunction.
   Inside,     // Inside Zone.
   After,      // After Zone and inside EnclosingFunction.
   OutsideFunc // Outside EnclosingFunction.
 };
 
+enum FunctionDeclKind {
+  InlineDefinition,
+  ForwardDeclaration,
+  OutOfLineDefinition
+};
+
 // A RootStmt is a statement that's fully selected including all it's children
 // and it's parent is unselected.
 // Check if a node is a root statement.
 bool isRootStmt(const Node *N) {
   if (!N->ASTNode.get<Stmt>())
     return false;
   // Root statement cannot be partially selected.
   if (N->Selected == SelectionTree::Partial)

 // Finds the function in which the zone lies.
 const FunctionDecl *findEnclosingFunction(const Node *CommonAnc) {
   // Walk up the SelectionTree until we find a function Decl
   for (const Node *CurNode = CommonAnc; CurNode; CurNode = CurNode->Parent) {
     // Don't extract from lambdas
     if (CurNode->ASTNode.get<LambdaExpr>())
       return nullptr;
     if (const FunctionDecl *Func = CurNode->ASTNode.get<FunctionDecl>()) {
-      // FIXME: Support extraction from methods.
-      if (isa<CXXMethodDecl>(Func))
-        return nullptr;
       // FIXME: Support extraction from templated functions.
       if (Func->isTemplated())
         return nullptr;
       return Func;
     }
   }
   return nullptr;
 }
struct Parameter {
     bool operator<(const Parameter &Other) const {
       return OrderPriority < Other.OrderPriority;
     }
   };
   std::string Name = "extracted";
   QualType ReturnType;
   std::vector<Parameter> Parameters;
   SourceRange BodyRange;
-  SourceLocation InsertionPoint;
-  const DeclContext *EnclosingFuncContext;
+  SourceLocation DefinitionPoint;
+  llvm::Optional<SourceLocation> ForwardDeclarationPoint;
+  const CXXRecordDecl *EnclosingClass = nullptr;
+  const NestedNameSpecifier *DefinitionQualifier = nullptr;
+  const DeclContext *SemanticDC = nullptr;
+  const DeclContext *SyntacticDC = nullptr;
+  const DeclContext *ForwardDeclarationSyntacticDC = nullptr;
   bool CallerReturnsValue = false;
+  bool Static = false;
+  ConstexprSpecKind Constexpr = ConstexprSpecKind::Unspecified;
+  bool Const = false;
+
   // Decides whether the extracted function body and the function call need a
   // semicolon after extraction.
   tooling::ExtractionSemicolonPolicy SemicolonPolicy;
-  NewFunction(tooling::ExtractionSemicolonPolicy SemicolonPolicy)
-      : SemicolonPolicy(SemicolonPolicy) {}
+  const LangOptions *LangOpts;
+  NewFunction(tooling::ExtractionSemicolonPolicy SemicolonPolicy,
+              const LangOptions *LangOpts)
+      : SemicolonPolicy(SemicolonPolicy), LangOpts(LangOpts) {}
   // Render the call for this function.
   std::string renderCall() const;
   // Render the definition for this function.
-  std::string renderDefinition(const SourceManager &SM) const;
+  std::string renderDeclaration(FunctionDeclKind K,
+                                const DeclContext &SemanticDC,
+                                const DeclContext &SyntacticDC,
+                                const SourceManager &SM) const;
 
 private:
-  std::string renderParametersForDefinition() const;
+  std::string
+  renderParametersForDeclaration(const DeclContext &Enclosing) const;
   std::string renderParametersForCall() const;
+  std::string renderSpecifiers(FunctionDeclKind K) const;
+  std::string renderQualifiers() const;
+  std::string renderDeclarationName(FunctionDeclKind K) const;
   // Generate the function body.
   std::string getFuncBody(const SourceManager &SM) const;
 };
 
-std::string NewFunction::renderParametersForDefinition() const {
+std::string NewFunction::renderParametersForDeclaration(
+    const DeclContext &Enclosing) const {
   std::string Result;
   bool NeedCommaBefore = false;
   for (const Parameter &P : Parameters) {
     if (NeedCommaBefore)
       Result += ", ";
     NeedCommaBefore = true;
-    Result += P.render(EnclosingFuncContext);
+    Result += P.render(&Enclosing);
   }
   return Result;
 }
 
 std::string NewFunction::renderParametersForCall() const {
   std::string Result;
   bool NeedCommaBefore = false;
   for (const Parameter &P : Parameters) {
     if (NeedCommaBefore)
       Result += ", ";
     NeedCommaBefore = true;
     Result += P.Name;
   }
   return Result;
 }
 
+std::string NewFunction::renderSpecifiers(FunctionDeclKind K) const {
+  std::string Attributes;
+
+  if (Static && K != FunctionDeclKind::OutOfLineDefinition) {
+    Attributes += "static ";
+  }
+
+  switch (Constexpr) {
+  case ConstexprSpecKind::Unspecified:
+  case ConstexprSpecKind::Constinit:
+    break;
+  case ConstexprSpecKind::Constexpr:
+    Attributes += "constexpr ";
+    break;
+  case ConstexprSpecKind::Consteval:
+    Attributes += "consteval ";
+    break;
+  }
+
+  return Attributes;
+}
+
+std::string NewFunction::renderQualifiers() const {
+  std::string Attributes;
+
+  if (Const) {
+    Attributes += " const";
+  }
+
+  return Attributes;
+}
+
+std::string NewFunction::renderDeclarationName(FunctionDeclKind K) const {
+  if (DefinitionQualifier == nullptr || K != OutOfLineDefinition) {
+    return Name;
+  }
+
+  std::string QualifierName;
+  llvm::raw_string_ostream Oss(QualifierName);
+  DefinitionQualifier->print(Oss, *LangOpts);
+  return llvm::formatv("{0}{1}", QualifierName, Name);
+}
+
 std::string NewFunction::renderCall() const {
   return std::string(
       llvm::formatv("{0}{1}({2}){3}", CallerReturnsValue ? "return " : "", Name,
                     renderParametersForCall(),
                     (SemicolonPolicy.isNeededInOriginalFunction() ? ";" : "")));
 }
 
-std::string NewFunction::renderDefinition(const SourceManager &SM) const {
-  return std::string(llvm::formatv(
-      "{0} {1}({2}) {\n{3}\n}\n", printType(ReturnType, *EnclosingFuncContext),
-      Name, renderParametersForDefinition(), getFuncBody(SM)));
+std::string NewFunction::renderDeclaration(FunctionDeclKind K,
+                                           const DeclContext &SemanticDC,
+                                           const DeclContext &SyntacticDC,
+                                           const SourceManager &SM) const {
+  std::string Declaration = std::string(llvm::formatv(
+      "{0}{1} {2}({3}){4}", renderSpecifiers(K),
+      printType(ReturnType, SyntacticDC), renderDeclarationName(K),
+      renderParametersForDeclaration(SemanticDC), renderQualifiers()));
+
+  switch (K) {
+  case ForwardDeclaration:
+    return std::string(llvm::formatv("{0};\n", Declaration));
+  case OutOfLineDefinition:
+  case InlineDefinition:
+    return std::string(
+        llvm::formatv("{0} {\n{1}\n}\n", Declaration, getFuncBody(SM)));
+    break;
+  }
 }
 
 std::string NewFunction::getFuncBody(const SourceManager &SM) const {
   // FIXME: Generate tooling::Replacements instead of std::string to
   // - hoist decls
   // - add return statement
   // - Add semicolon
   return toSourceCode(SM, BodyRange).str() +
if (CapturedInfo.HasReturnStmt) {
     ExtractedFunc.ReturnType = Ret;
     return true;
   }
   // FIXME: Generate new return statement if needed.
   ExtractedFunc.ReturnType = EnclosingFunc.getParentASTContext().VoidTy;
   return true;
 }
 
+void captureMethodInfo(NewFunction &ExtractedFunc,
+                       const CXXMethodDecl *Method) {
+  ExtractedFunc.Static = Method->isStatic();
+  ExtractedFunc.Const = Method->isConst();
+  ExtractedFunc.EnclosingClass = Method->getParent();
+}
+
 // FIXME: add support for adding other function return types besides void.
 // FIXME: assign the value returned by non void extracted function.
 llvm::Expected<NewFunction> getExtractedFunction(ExtractionZone &ExtZone,
                                                  const SourceManager &SM,
                                                  const LangOptions &LangOpts) {
   CapturedZoneInfo CapturedInfo = captureZoneInfo(ExtZone);
   // Bail out if any break of continue exists
   if (CapturedInfo.BrokenControlFlow)
     return error("Cannot extract break/continue without corresponding "
                  "loop/switch statement.");
-  NewFunction ExtractedFunc(getSemicolonPolicy(ExtZone, SM, LangOpts));
+  NewFunction ExtractedFunc(getSemicolonPolicy(ExtZone, SM, LangOpts),
+                            &LangOpts);
+
+  ExtractedFunc.SyntacticDC =
+      ExtZone.EnclosingFunction->getLexicalDeclContext();
+  ExtractedFunc.SemanticDC = ExtZone.EnclosingFunction->getDeclContext();
+  ExtractedFunc.DefinitionQualifier = ExtZone.EnclosingFunction->getQualifier();
+  ExtractedFunc.Constexpr = ExtZone.EnclosingFunction->getConstexprKind();
+
+  if (const auto *Method =
+          llvm::dyn_cast<CXXMethodDecl>(ExtZone.EnclosingFunction))
+    captureMethodInfo(ExtractedFunc, Method);
+
+  if (ExtZone.EnclosingFunction->isOutOfLine()) {
+    // FIXME: Put the extracted method in a private section if it's a class or
+    // maybe in an anonymous namespace
+    const auto *FirstOriginalDecl =
+        ExtZone.EnclosingFunction->getCanonicalDecl();
+    auto DeclPos =
+        toHalfOpenFileRange(SM, LangOpts, FirstOriginalDecl->getSourceRange());
+    if (!DeclPos)
+      return error("Declaration is inside a macro");
+    ExtractedFunc.ForwardDeclarationPoint = DeclPos.getValue().getBegin();
+    ExtractedFunc.ForwardDeclarationSyntacticDC = ExtractedFunc.SemanticDC;
+  }
+
   ExtractedFunc.BodyRange = ExtZone.ZoneRange;
-  ExtractedFunc.InsertionPoint = ExtZone.getInsertionPoint();
-  ExtractedFunc.EnclosingFuncContext =
-      ExtZone.EnclosingFunction->getDeclContext();
+  ExtractedFunc.DefinitionPoint = ExtZone.getInsertionPoint();
+
   ExtractedFunc.CallerReturnsValue = CapturedInfo.AlwaysReturns;
   if (!createParameters(ExtractedFunc, CapturedInfo) ||
       !generateReturnProperties(ExtractedFunc, *ExtZone.EnclosingFunction,
                                 CapturedInfo))
     return error("Too complex to extract.");
   return ExtractedFunc;
 }
 
tooling::Replacement replaceWithFuncCall(const NewFunction &ExtractedFunc,
                                          const LangOptions &LangOpts) {
   std::string FuncCall = ExtractedFunc.renderCall();
   return tooling::Replacement(
       SM, CharSourceRange(ExtractedFunc.BodyRange, false), FuncCall, LangOpts);
 }
 
 tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc,
                                               const SourceManager &SM) {
-  std::string FunctionDef = ExtractedFunc.renderDefinition(SM);
-  return tooling::Replacement(SM, ExtractedFunc.InsertionPoint, 0, FunctionDef);
+  FunctionDeclKind DeclKind = InlineDefinition;
+  if (ExtractedFunc.ForwardDeclarationPoint.hasValue())
+    DeclKind = OutOfLineDefinition;
+  std::string FunctionDef = ExtractedFunc.renderDeclaration(
+      DeclKind, *ExtractedFunc.SemanticDC, *ExtractedFunc.SyntacticDC, SM);
+
+  return tooling::Replacement(SM, ExtractedFunc.DefinitionPoint, 0,
+                              FunctionDef);
+}
+
+tooling::Replacement createForwardDeclaration(const NewFunction &ExtractedFunc,
+                                              const SourceManager &SM) {
+  std::string FunctionDecl = ExtractedFunc.renderDeclaration(
+      ForwardDeclaration, *ExtractedFunc.SemanticDC,
+      *ExtractedFunc.ForwardDeclarationSyntacticDC, SM);
+  SourceLocation DeclPoint = ExtractedFunc.ForwardDeclarationPoint.getValue();
+
+  return tooling::Replacement(SM, DeclPoint, 0, FunctionDecl);
 }
 
 // Returns true if ExtZone contains any ReturnStmts.
 bool hasReturnStmt(const ExtractionZone &ExtZone) {
   class ReturnStmtVisitor
       : public clang::RecursiveASTVisitor<ReturnStmtVisitor> {
   public:
     bool VisitReturnStmt(ReturnStmt *Return) {
       Found = true;
       return false; // We found the answer, abort the scan.
     }
     bool Found = false;
   };
 
   ReturnStmtVisitor V;
   for (const Stmt *RootStmt : ExtZone.RootStmts) {
     V.TraverseStmt(const_cast<Stmt *>(RootStmt));

 
 Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {
   const SourceManager &SM = Inputs.AST->getSourceManager();
   const LangOptions &LangOpts = Inputs.AST->getLangOpts();
   auto ExtractedFunc = getExtractedFunction(ExtZone, SM, LangOpts);
   // FIXME: Add more types of errors.
   if (!ExtractedFunc)
     return ExtractedFunc.takeError();
-  tooling::Replacements Result;
-  if (auto Err = Result.add(createFunctionDefinition(*ExtractedFunc, SM)))
+  tooling::Replacements Edit;
+  if (auto Err = Edit.add(createFunctionDefinition(*ExtractedFunc, SM)))
     return std::move(Err);
-  if (auto Err = Result.add(replaceWithFuncCall(*ExtractedFunc, SM, LangOpts)))
+  if (auto Err = Edit.add(replaceWithFuncCall(*ExtractedFunc, SM, LangOpts)))
     return std::move(Err);
-  return Effect::mainFileEdit(SM, std::move(Result));
+
+  if (auto FwdLoc = ExtractedFunc->ForwardDeclarationPoint) {
+    // If the fwd-declaration goes in the same file, merge into Replacements.
+    // Otherwise it needs to be a separate file edit.
+    if (SM.isWrittenInSameFile(ExtractedFunc->DefinitionPoint, *FwdLoc)) {
+      if (auto Err = Edit.add(createForwardDeclaration(*ExtractedFunc, SM)))
+        return std::move(Err);
+    } else {
+      auto MultiFileEffect = Effect::mainFileEdit(SM, std::move(Edit));
+      if (!MultiFileEffect)
+        return MultiFileEffect.takeError();
+
+      tooling::Replacements OtherEdit(
+          createForwardDeclaration(*ExtractedFunc, SM));
+      if (auto PathAndEdit = Tweak::Effect::fileEdit(SM, SM.getFileID(*FwdLoc),
+                                                 OtherEdit))
+        MultiFileEffect->ApplyEdits.try_emplace(PathAndEdit->first,
+                                                PathAndEdit->second);
+      else
+        return PathAndEdit.takeError();
+      return MultiFileEffect;
+    }
+  }
+  return Effect::mainFileEdit(SM, std::move(Edit));
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

 int f(const X::Y &y) {
   return extracted(y);
 })cpp";
   EXPECT_EQ(apply(ConstNamespaceCheckInput), ConstNamespaceCheckOutput);
 
   // Don't extract when we need to make a function as a parameter.
   EXPECT_THAT(apply("void f() { [[int a; f();]] }"), StartsWith("fail"));
 
-  // We don't extract from methods for now since they may involve multi-file
-  // edits
-  std::string MethodFailInput = R"cpp(
+  std::string MethodInput = R"cpp(
     class T {
       void f() {
         [[int x;]]
       }
     };
   )cpp";
-  EXPECT_EQ(apply(MethodFailInput), "unavailable");
+  std::string MethodCheckOutput = R"cpp(
+    class T {
+      void extracted() {
+int x;
+}
+void f() {
+        extracted();
+      }
+    };
+  )cpp";
+  EXPECT_EQ(apply(MethodInput), MethodCheckOutput);
+
+  std::string OutOfLineMethodInput = R"cpp(
+    class T {
+      void f();
+    };
+
+    void T::f() {
+      [[int x;]]
+    }
+  )cpp";
+  std::string OutOfLineMethodCheckOutput = R"cpp(
+    class T {
+      void extracted();
+void f();
+    };
+
+    void T::extracted() {
+int x;
+}
+void T::f() {
+      extracted();
+    }
+  )cpp";
+  EXPECT_EQ(apply(OutOfLineMethodInput), OutOfLineMethodCheckOutput);
 
   // We don't extract from templated functions for now as templates are hard
   // to deal with.
   std::string TemplateFailInput = R"cpp(
     template<typename T>
     void f() {
       [[int x;]]
     }
F (extracted();)
   std::string CompoundFailInput = R"cpp(
     void f() [[{
       int a;
     }]]
   )cpp";
   EXPECT_EQ(apply(CompoundFailInput), "unavailable");
 }
 
+TEST_F(ExtractFunctionTest, DifferentHeaderSourceTest) {
+  Header = R"cpp(
+    class SomeClass {
+      void f();
+    };
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+    void SomeClass::f() {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+      extracted();
+    }
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+    class SomeClass {
+      void extracted();
+void f();
+    };
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, DifferentFilesNestedTest) {
+  Header = R"cpp(
+    class T {
+    class SomeClass {
+      void f();
+    };
+    };
+  )cpp";
+
+  std::string NestedOutOfLineSource = R"cpp(
+    void T::SomeClass::f() {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string NestedOutOfLineSourceOutputCheck = R"cpp(
+    void T::SomeClass::extracted() {
+int x;
+}
+void T::SomeClass::f() {
+      extracted();
+    }
+  )cpp";
+
+  std::string NestedHeaderOutputCheck = R"cpp(
+    class T {
+    class SomeClass {
+      void extracted();
+void f();
+    };
+    };
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(NestedOutOfLineSource, &EditedFiles),
+            NestedOutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, NestedHeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, ConstexprDifferentHeaderSourceTest) {
+  Header = R"cpp(
+    class SomeClass {
+      constexpr void f() const;
+    };
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+    constexpr void SomeClass::f() const {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    constexpr void SomeClass::extracted() const {
+int x;
+}
+constexpr void SomeClass::f() const {
+      extracted();
+    }
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+    class SomeClass {
+      constexpr void extracted() const;
+constexpr void f() const;
+    };
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+      << "The header should be edited and receives the declaration of the new "
+         "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+    EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, ConstevalDifferentHeaderSourceTest) {
+  ExtraArgs.push_back("--std=c++20");
+  Header = R"cpp(
+    class SomeClass {
+      consteval void f() const;
+    };
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+    consteval void SomeClass::f() const {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    consteval void SomeClass::extracted() const {
+int x;
+}
+consteval void SomeClass::f() const {
+      extracted();
+    }
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+    class SomeClass {
+      consteval void extracted() const;
+consteval void f() const;
+    };
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+      << "The header should be edited and receives the declaration of the new "
+         "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+    EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, ConstDifferentHeaderSourceTest) {
+  Header = R"cpp(
+    class SomeClass {
+      void f() const;
+    };
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+    void SomeClass::f() const {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    void SomeClass::extracted() const {
+int x;
+}
+void SomeClass::f() const {
+      extracted();
+    }
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+    class SomeClass {
+      void extracted() const;
+void f() const;
+    };
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+      << "The header should be edited and receives the declaration of the new "
+         "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+    EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, StaticDifferentHeaderSourceTest) {
+  Header = R"cpp(
+    class SomeClass {
+      static void f();
+    };
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+    void SomeClass::f() {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    void SomeClass::extracted() {
+int x;
+}
+void SomeClass::f() {
+      extracted();
+    }
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+    class SomeClass {
+      static void extracted();
+static void f();
+    };
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_NE(EditedFiles.begin(), EditedFiles.end())
+      << "The header should be edited and receives the declaration of the new "
+         "function";
+
+  if (EditedFiles.begin() != EditedFiles.end()) {
+    EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+  }
+}
+
+TEST_F(ExtractFunctionTest, DifferentContextHeaderSourceTest) {
+  Header = R"cpp(
+    namespace ns{
+    class A {
+      class C {
+      public:
+        class RType {};
+      };
+
+      class T {
+        class SomeClass {
+          static C::RType f();
+        };
+      };
+    };
+    } // ns
+  )cpp";
+
+  std::string OutOfLineSource = R"cpp(
+    ns::A::C::RType ns::A::T::SomeClass::f() {
+      [[A::C::RType x;
+      return x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    ns::A::C::RType ns::A::T::SomeClass::extracted() {
+A::C::RType x;
+      return x;
+}
+ns::A::C::RType ns::A::T::SomeClass::f() {
+      return extracted();
+    }
+  )cpp";
+
+  std::string HeaderOutputCheck = R"cpp(
+    namespace ns{
+    class A {
+      class C {
+      public:
+        class RType {};
+      };
+
+      class T {
+        class SomeClass {
+          static ns::A::C::RType extracted();
+static C::RType f();
+        };
+      };
+    };
+    } // ns
+  )cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+
+  EXPECT_EQ(apply(OutOfLineSource, &EditedFiles), OutOfLineSourceOutputCheck);
+  EXPECT_EQ(EditedFiles.begin()->second, HeaderOutputCheck);
+}
+
+TEST_F(ExtractFunctionTest, DifferentSyntacticContextNamespace) {
+  std::string OutOfLineSource = R"cpp(
+    namespace ns {
+      void f();
+    }
+
+    void ns::f() {
+      [[int x;]]
+    }
+  )cpp";
+
+  std::string OutOfLineSourceOutputCheck = R"cpp(
+    namespace ns {
+      void extracted();
+void f();
+    }
+
+    void ns::extracted() {
+int x;
+}
+void ns::f() {
+      extracted();
+    }
+  )cpp";
+
+  EXPECT_EQ(apply(OutOfLineSource), OutOfLineSourceOutputCheck);
+}
+
 TEST_F(ExtractFunctionTest, ControlFlow) {
   Context = Function;
   // We should be able to extract break/continue with a parent loop/switch.
   EXPECT_THAT(apply(" [[for(;;) if(1) break;]] "), HasSubstr("extracted"));
   EXPECT_THAT(apply(" for(;;) [[while(1) break;]] "), HasSubstr("extracted"));
   EXPECT_THAT(apply(" [[switch(1) { break; }]]"), HasSubstr("extracted"));
   EXPECT_THAT(apply(" [[while(1) switch(1) { continue; }]]"),
               HasSubstr("extracted"));