[clangd] DefineInline action apply logic with fully qualified names

[kadircet]

Summary

Initial version of DefineInline action that will fully qualify every
name inside function body. It has some problems while qualyfying dependent
template arguments, see FIXME in the unittests.

[ilya-biryukov]

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

165

Are we trying to replace the whole name?

We can avoid this problem by not qualifying template arguments in the first place.
I.e. instead of replacing the whole name, including template arguments:

[[baz<Bar>]] -> [[::ns::baz<Bar>]]

We could simply replace the part before template arguments:

[[baz]]<Bar> -> [[::ns::baz]]<Bar>

[kadircet]

• Rebase and update testhelpers

[kadircet]

• Mark tweak as visible

[ilya-biryukov]

Currently we add too many qualifiers in some cases, e.g. here's a common pattern in clangd:

// foo.h
#include "Decl.h"
namespace clang::clangd { std::string printName(Decl*D); }

// foo.cpp
#include "foo.h"
namespace clang::clangd { 
  std::string printName(Decl *D) {
    NamedDecl* ND = ...;
  } 
...
}

this will result in:

// foo.h
namespace clang::clangd {
std::string printName(Decl*D) {
  clang::NamedDecl *ND = ...; // <-- (!) we did not even had any 'using' declarations or 'using namespace's 
}
}

It's ok to leave this out of the initial change, but could we describe our strategy to tackle this somewhere in the comments - how we want to fix this and when.

clang-tools-extra/clangd/unittests/TweakTesting.h

81–82

It seems weird that we have this inconsistency between the contents for current file (passed through the return value) and contents for other files (pass through the fields).

A few better alternatives that come to mind:

1. add an out parameter, could be optional in case when all changes are in the main file.
2. this function will fail when there are changes outside main file, but we can add a new function to return changes in all modified files, e.g. as StringMap<std::string> or vector<pair<Path, string/*Content*/>> (the latter allows to use standard matchers)

[ilya-biryukov]

We also need to rename parameters sometimes, right?

// Sometimes we need to rename parameters.
void usages(int decl_param, int);

void usages(int def_param, int now_named) {
  llvm::errs() << def_param + now_named;
}

// And template parameters! (these are even more interesting)
template <class T>
struct Foo {
  template <class U, class>
  void usages();
};
template <class L>
template <class R, class NowNamed>
void Foo<L>::usages() {
  llvm::errs() << L() + R() + NowNamed();
}

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

178

I don't think it's true in presence of using declarations:

namespace ns {
  void foo(int*);
}
namespace other_ns {
  void foo(char*);
}

using ns::foo;
using other_ns::foo;

template <class T>
void usages() {
  foo(T()); // <-- would we add `ns::foo` or `other_ns::foo`?
}

Not qualifying in this case is probably ok, but adding any of the qualifiers is probably wrong.
Could you check what we do in this case and either add to tests or put into a list of known issues (e.g. file a bug or at least add a FIXME?)

184

NIT: could you add an example of a member expression here?

Note that sometimes there is no left side (it's implicit), but in that case we shouldn't qualify either.

185

I believe we could also avoid qualifying anything from non-namespace scope.
This follows from the fact that one cannot move stuff from class to namespace with using declarations:

namespace ns {
class X {
  static void foo();
  void usages();
};
}

using ns::X::foo; // <- not allowed!
void ns::X::usages() {
  foo(); // <-- so no need to qualify this!
}

If I'm reading the code correctly, we will get ns::foo() in the current version.

254

Not sure if this covers these two cases (could you please add them to the tests?):

template <class T>
struct vector {
  void foo();
};

template <class T>
void vector<T>::foo() {}

template <class T>
struct list {
  template <class U>
  void foo();
}

template <class T>
template <class U>
void list<T>::foo() {}

Or are we disabled in those cases?

335

This never fails since there are no replacements to conflict with, right?
Maybe we could store a single tooling::Replacement here instead? That should simplify rest of the code (e.g. no need to clear() replacements and re-use them for different file)

[hokein]

In D66647#1684046, @ilya-biryukov wrote:

We also need to rename parameters sometimes, right?

// Sometimes we need to rename parameters.
void usages(int decl_param, int);

void usages(int def_param, int now_named) {
  llvm::errs() << def_param + now_named;
}

// And template parameters! (these are even more interesting)
template <class T>
struct Foo {
  template <class U, class>
  void usages();
};
template <class L>
template <class R, class NowNamed>
void Foo<L>::usages() {
  llvm::errs() << L() + R() + NowNamed();
}

This is tricky, and there are also cases where the declaration doesn't provide the name of parameter, e.g. llvm::json::Value toJSON(const CodeAction &);. Maybe we can overwrite the declaration signature with the definition's, or just disable the tweak on these cases.

[kadircet]

• Address comments

[kadircet]

In D66647#1684012, @ilya-biryukov wrote:

It's ok to leave this out of the initial change, but could we describe our strategy to tackle this somewhere in the comments - how we want to fix this and when.

I actually have a fixme saying:

// FIXME: Instead of fully qualifying we should try deducing visible scopes at
// target location and generate minimal edits.

Elaborating on it by saying "we can start by using the namespaces in targetcontext".

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

178

as discussed offline;
action should not be available in that case and we need underlying

clang-tools-extra/clangd/unittests/TweakTesting.h

81–82

It is left-out this way since most of the checks only care about a single file, and there are lots of them so changing would definitely require some plumbing.

I am not sure having the inconsistency between main file and others matters so much though; we have similar discrimination towards non-main files in a bunch of other places in testing infrastructure e.g TestTU.

As for the suggestions, adding another apply function for multifile case doesn't seem so nice, so I would rather move forward with first one, if you believe this is a strong concern.

[ilya-biryukov]

In D66647#1685506, @kadircet wrote:

I actually have a fixme saying:

// FIXME: Instead of fully qualifying we should try deducing visible scopes at
// target location and generate minimal edits.

Elaborating on it by saying "we can start by using the namespaces in targetcontext".

Are we planning to fix this right away or should we keep this indefinitely?
If latter, I believe we should think about some heuristics to avoid qualification in the short term instead.
If we want to do it right away, that's great! (albeit more complicated, I guess)

[kadircet]

In D66647#1685554, @ilya-biryukov wrote:

Are we planning to fix this right away or should we keep this indefinitely?
If latter, I believe we should think about some heuristics to avoid qualification in the short term instead.
If we want to do it right away, that's great! (albeit more complicated, I guess)

That's the end goal, but I am currently working on a patch that will at least strip away the common namespaces
of function declaration and symbols used inside.

[ilya-biryukov]

clang-tools-extra/clangd/unittests/TweakTesting.h

81–82

It is left-out this way since most of the checks only care about a single file, and there are lots of them so changing would definitely require some plumbing.

Agree that no matter what we do, we should let the current usages stay the same.

I am not sure having the inconsistency between main file and others matters so much though; we have similar discrimination towards non-main files in a bunch of other places in testing infrastructure e.g TestTU.

What kind of inconsistency are you referring to? Both main file and extra files are fields inside TestTU? It outputs ParsedAST and it's a return value of the build() function.
There are no multiple ways to return results.

As for the suggestions, adding another apply function for multifile case doesn't seem so nice, so I would rather move forward with first one, if you believe this is a strong concern.

Yeah, totally ok with this. I'm not a big fan of out parameters myself and prefer putting stuff into the return value instead.
But they're still widely used, so I view this as my preference rather than a common convention (sigh...)

[kadircet]

• Add renaming of template and function parameters

[kadircet]

In D66647#1684046, @ilya-biryukov wrote:

We also need to rename parameters sometimes, right?

// Sometimes we need to rename parameters.
void usages(int decl_param, int);

void usages(int def_param, int now_named) {
  llvm::errs() << def_param + now_named;
}

// And template parameters! (these are even more interesting)
template <class T>
struct Foo {
  template <class U, class>
  void usages();
};
template <class L>
template <class R, class NowNamed>
void Foo<L>::usages() {
  llvm::errs() << L() + R() + NowNamed();
}

So currently AST doesn't store any information regarding template parameter locations except the deepest one.
Therefore I've changed the availability to discard any methods inside templated classes, since there is no way to
validate the template parameter names. Hopefully this should be a rare use-case, and I believe most of the times
people have same template paramater names on declaration and definition. Let me know what you think about
it.

In addition to that implemented renaming for function parameters and template parameters in case of templated
functions.

[ilya-biryukov]

In D66647#1686021, @kadircet wrote:

So currently AST doesn't store any information regarding template parameter locations except the deepest one.
Therefore I've changed the availability to discard any methods inside templated classes, since there is no way to
validate the template parameter names. Hopefully this should be a rare use-case, and I believe most of the times
people have same template paramater names on declaration and definition. Let me know what you think about
it.

Thanks! LG, I also think this should be a rare case. The deepest template parameters are typically the only ones anyway.

In addition to that implemented renaming for function parameters and template parameters in case of templated
functions.

Thanks, will take a look!

[kadircet]

• Fix getSemiColon to handle semicolons at the end of file

[kadircet]

• Update test helper to take an out parameter

[ilya-biryukov]

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

68

"Lexes" is probably a not very relevant implementation detail.
I'd say this function probably doesn't need a comment, it's clear what it does from its name.

168

NIT: Maybe store Error directly?
It would communicate the intent of the variable more clearly.

174

NIT: IIUC, this could be simply if (Ref.Qualifier)

181

As discussed before, Targets can actually come from different scopes, e.g. from ADL.

182

It just occurred to me that this is a really bad idea. I think the order of items in Targets is non-deterministic.
Could we instead try to check if all scopes are the same and only qualify in that case?

195

You would want to use ND->printNestedNameSpecifier() instead to avoid printing inline namespaces:

namespace std { inline namespace v1 { 
 struct vector {};
}}

^-- I believe the current code would print std::v1:: for vector and we want std::.
Could you add this example to tests too?

198

Maybe return a generic error in the end instead of the last error?
Something like createStringError(..., "Failed to compute qualifiers, see errors in the logs for more details").

Should be a better UX for anyone who tries to explore what went wrong.

222

NIT: mention that both function parameters and template parameters are updated here.

225

This does not rename any references to those parameters, right?
E.g.

template <class T> void foo(T x);

template <class U> void foo(U x) {}

would turn into

template <class U> void foo(T x);

right?

[kadircet]

• Address comments

[kadircet]

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

195

it is the opposite, printNamespaceScope skips anonymous and inline namespaces, whereas printNestedNameSpecifier also prints those. adding a test.

225

yes that's right, just adding a unittest will address this later on

[kadircet]

• Move parameter renaming logic to a separate patch.

[kadircet]

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

225

moving into D68937, since it got bigger than expected.

[ilya-biryukov]

Mostly comments about tests, the implementation itself LG.

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

68

NIT: s/SemiColon/Semicolon

79

What are the tokens we expect to see before the semicolon here?
It feels safer to just skip comments and whitespace and check the next token is a semicolon.

Any reason to have an actual loop here?

181

NIT: could we mention the tweak name here? to make sure it's easy to detect those lines in the logs:
define inline: targets from multiple contexts

208

NIT: braces are redundant

320–366

NIT: use auto

325

NIT: braces are redundant

clang-tools-extra/clangd/unittests/TweakTesting.cpp

85

Could you document EditedFiles contains all other edited files and the original std::string only contains results for the main file?
Would also be useful to document what is the key in the StringMap: absolute/relative paths or URI?

clang-tools-extra/clangd/unittests/TweakTests.cpp

1131

Why have a comment in this test?
If we need to test we handle comments before the semicolon, could we move to a separate focused test?
If we want to test something else, let's leave out the comment. The test is hard enough to read on its own

1148

Template declarations inside function bodies are not allowed in C++.
Are we getting compiler errors here and not actually testing anything?

1187

Same here: we don't need the comment here.

1190

Either remove using namespace a from the function body or put a FIXME this shouldn't actually qualify?
I believe the first option is what we want, if we're trying to check template arguments are getting transformed properly

1215

Maybe move this matcher to the start of the file?

1255

We prefer to have using ::testing::ElementsAre at the start of the file and not qualify the usage everywhere in our tests.

1302

This is really uncommon, I'm not even sure we should test this.
The other case is much more common and interesting:

template <class T>
void foo(T p);

template <>
void foo<int>(int p) { /* do something */ }

Could we add a test that we don't suggest this action in that case?

1393

Can we test with a single namespace?
Having 3 layers of nested namespace in every test makes reading them very complicated.

Instead, we could validate with separate tests:

• that the qualification logic works with multiple nested namespaces,
• that various kinds of references are properly qualified with a single namespace

I believe that would keep both tests more focused

1441

Could you indent here and in other tests? it's hard to follow the fact that Foo is inside b without indentation.
Same for other tests.

Alternatively, put on one line if there's just a single declaration inside the namespace

[kadircet]

• Address comments

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

79

it has been a long time since our offline discussions around this one. but the idea was could have any attributes before semicolon, these could be macros that will expand to one thing in some platforms and another in a different platform. Therefore we've decided to skip anything until we've found a semicolon.

I don't think I follow the second part of the question though? What do you mean by "having an actual loop"?

208

I've thought you were a fan of braces when the inner statement spans multiple lines, even if it is a single statement :D

clang-tools-extra/clangd/unittests/TweakTests.cpp

1148

well, it was still working and inlining the function definition, apparently despite the diagnostic, ranges were correct.

1302

yes there is a test for this in availability checks patch:

EXPECT_UNAVAILABLE(R"cpp(
    template <typename T> void foo();

    template<> void f^oo<int>() {
    })cpp");

1441

no more nested namespaces

[ilya-biryukov]

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

79

The loop I mentioned is

while (!CurToken.is(tok::semi))

I believe this could be replaced with

auto NextTok = Lexer::findNextToken(...);
if (NextTok && NextTok->is(tok::semi))
  ...

There are two common attribute applications we should mostly care about:

// Attribute in declaration specifiers.
[[noreturn]] int foo(); 

// Attribute after declarator name
int foo [[noreturn]]();

Both work with a simple if statement, rather than the attributes.

The case we're covering with the loop is super uncommon and I don't think we should even bother to support it:

int foo() [[some::attribute]];

[ilya-biryukov]

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

208

It's more complicated than that... Let me explain...
I'm a fan of braces when:

1. There are line comments (I perceive those as an extra statement):

if (something) {
  // Here comes a comment...
  return 10;
}

2. There are composite statements inside the if body:

if (something) {
  for (;;)
    do_something_else();
}

In your case, I'd not use braces...
It's not important, though, pick the style you like most.

clang-tools-extra/clangd/unittests/TweakTests.cpp

1148

I guess we didn't need to do any edits there, so we just carried the text unchanged.
Had we actually needed to change something inside the template, we'd fail to do so.

Range of the function body is correct, since the parser can easily recover in those cases.

1441

Yay! Thanks!

[ilya-biryukov]

A few last NITs and one important comment about handling the case when function definition come from macro expansions

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

323

NIT: s/SemiColon/Semicolon

334

Could we add a test when the semicolon comes from a macro expansion? I believe the action won't be available in that case, which is ok. Just to make sure we cover this corner-case.

#define SEMI ;
void foo() SEMI

Also interesting cases:

• source function is in a macro expansion
• target function is in a macro expansion.

I believe they might catch a few bugs here, as we seem to assume all locations are file locations...

clang-tools-extra/clangd/unittests/TweakTesting.h

73

Hm, maybe even call ADD_FAILURE() when there are changes to other files and tests pass null to EditedFiles?
Likely an error in the test.

clang-tools-extra/clangd/unittests/TweakTests.cpp

1226

Argh... Without clang-format that looks SO weird :-)

1266

Maybe put it before using namespace a;?
To make sure the test does not need to change if we actually support not qualifying when not neccessary

1332

How is this test different from the previous one?
Is it just trying to make sure we don't always move to the first function body?

1374

Do we test the following case anywhere?

template <class T> void foo();
template <> void ^foo<int>() { // should not be available
  return;
}

[kadircet]

• Improve macro handling

[kadircet]

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

334

see macro tests for the behavior in such cases.

clang-tools-extra/clangd/unittests/TweakTests.cpp

1332

yes that's the point, adding a comment.

1374

Yes, there is once check in availability tests for:

EXPECT_UNAVAILABLE(R"cpp(
  template <typename T> void foo();

  template<> void f^oo<int>() {
  })cpp");

[ilya-biryukov]

LGTM

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

323

This one is still there.

334

s/SemiColon/Semicolon

clang-tools-extra/clangd/unittests/TweakTests.cpp

1332

Thanks!

1374

Ah, nice, I missed it. Thanks for pointing it out.

[kadircet]

• s/SemiColon/Semicolon/

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

 //===--- DefineInline.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 "FindTarget.h"
 #include "Logger.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "XRefs.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"

 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/Preprocessor.h"
 #include "clang/Lex/Token.h"
 #include "clang/Sema/Lookup.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/FormatAdapters.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstddef>
 #include <set>
 #include <string>
 #include <unordered_map>
+#include <utility>
 #include <vector>
 
 namespace clang {
 namespace clangd {
 namespace {
 
+llvm::Optional<SourceLocation> getSemicolonForDecl(const FunctionDecl *FD) {
+  const SourceManager &SM = FD->getASTContext().getSourceManager();
+  const LangOptions &LangOpts = FD->getASTContext().getLangOpts();
+
+  SourceLocation SemiColon =
+      Lexer::getLocForEndOfToken(FD->getEndLoc(), 0, SM, LangOpts);
+  llvm::StringRef BufData = SM.getBufferData(SM.getFileID(SemiColon));
+  Lexer RawLexer(SM.getLocForStartOfFile(SM.getFileID(SemiColon)), LangOpts,
+                 BufData.begin(), SM.getCharacterData(SemiColon),
+                 BufData.end());
+  Token CurToken;
+  while (!CurToken.is(tok::semi)) {
+    if (RawLexer.LexFromRawLexer(CurToken))
+      break;
+  }
+  if (!CurToken.is(tok::semi))
+    return llvm::None;
+  SemiColon = CurToken.getLocation();
+  return SemiColon;
+}
+
 // Deduces the FunctionDecl from a selection. Requires either the function body
 // or the function decl to be selected. Returns null if none of the above
 // criteria is met.
 const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
   const ast_type_traits::DynTypedNode &AstNode = SelNode->ASTNode;
   if (const FunctionDecl *FD = AstNode.get<FunctionDecl>())
     return FD;
   if (AstNode.get<CompoundStmt>() &&
       SelNode->Selected == SelectionTree::Complete) {
     if (const SelectionTree::Node *P = SelNode->Parent)
       return P->ASTNode.get<FunctionDecl>();
   }
   return nullptr;
 }
 
 // Checks the decls mentioned in Source are visible in the context of Target.
-// Achives that by checking declarations occur before target location in
+// Achives that by checking declaraions occur before target location in
 // translation unit or declared in the same class.
 bool checkDeclsAreVisible(const llvm::DenseSet<const Decl *> &DeclRefs,
                           const FunctionDecl *Target, const SourceManager &SM) {
   SourceLocation TargetLoc = Target->getLocation();
   // To be used in visibility check below, decls in a class are visible
   // independent of order.
   const RecordDecl *Class = nullptr;
   if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(Target))
for (const auto *DR : DeclRefs) {
     else if (const auto *FD = llvm::dyn_cast<FieldDecl>(D))
       Parent = FD->getParent();
     if (Parent != Class)
       return false;
   }
   return true;
 }
 
+// Rewrites body of FD to fully-qualify all of the decls inside.
+llvm::Expected<std::string> qualifyAllDecls(const FunctionDecl *FD) {
+  // There are three types of spellings that needs to be qualified in a function
+  // body:
+  // - Types:       Foo                 -> ns::Foo
+  // - DeclRefExpr: ns2::foo()          -> ns1::ns2::foo();
+  // - UsingDecls:
+  //    using ns2::foo      -> using ns1::ns2::foo
+  //    using namespace ns2 -> using namespace ns1::ns2
+  //    using ns3 = ns2     -> using ns3 = ns1::ns2
+  //
+  // Go over all references inside a function body to generate replacements that
+  // will fully qualify those. So that body can be moved into an arbitrary file.
+  // We perform the qualification by qualyfying the first type/decl in a
+  // (un)qualified name. e.g:
+  //    namespace a { namespace b { class Bar{}; void foo(); } }
+  //    b::Bar x; -> a::b::Bar x;
+  //    foo(); -> a::b::foo();
+  // FIXME: Instead of fully qualyfying we should try deducing visible scopes at
+  // target location and generate minimal edits.
+
+  const SourceManager &SM = FD->getASTContext().getSourceManager();
+  tooling::Replacements Replacements;
+  bool HadErrors = false;
+  findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) {
+    // Since we want to qualify only the first qualifier, skip names with a
+    // qualifier.
+    if (Ref.Qualifier)
+      return;
+    // There might be no decl in dependent contexts, there's nothing much we can
+    // do in such cases.
+    if (Ref.Targets.empty())
+      return;
+
+    for (const NamedDecl *ND : Ref.Targets) {
+      if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
+        elog("define inline: Targets from multiple contexts: {0}, {1}",
+             printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND));
+        HadErrors = true;
+        return;
+      }
+    }
+    // All Targets are in the same scope, so we can safely chose first one.
+    const NamedDecl *ND = Ref.Targets.front();
+    // Skip anything from a non-namespace scope, these can be:
+    // - Function or Method scopes, which means decl is local and doesn't need
+    //   qualification.
+    // - From Class/Struct/Union scope, which again doesn't need any qualifiers,
+    //   rather the left side of it requires qualification, like:
+    //   namespace a { class Bar { public: static int x; } }
+    //   void foo() { Bar::x; }
+    //                ~~~~~ -> we need to qualify Bar not x.
+    if (!ND->getDeclContext()->isNamespace())
+      return;
+
+    std::string Qualifier = printNamespaceScope(*ND->getDeclContext());
+    if (auto Err = Replacements.add(
+            tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
+      HadErrors = true;
+      elog("define inline: Failed to add quals: {0}", std::move(Err));
+    }
+  });
+
+  if (HadErrors) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "define inline: Failed to compute qualifiers see logs for details.");
+  }
+
+  auto QualifiedFunc = tooling::applyAllReplacements(
+      SM.getBufferData(SM.getFileID(FD->getLocation())), Replacements);
+  if (!QualifiedFunc)
+    return QualifiedFunc.takeError();
+
+  // Get new begin and end positions for the qualified body.
+  SourceRange OrigFuncRange = FD->getBody()->getSourceRange();
+  unsigned BodyBegin = SM.getFileOffset(OrigFuncRange.getBegin());
+  unsigned BodyEnd = Replacements.getShiftedCodePosition(
+      SM.getFileOffset(OrigFuncRange.getEnd()));
+
+  // Trim the result to function body.
+  return QualifiedFunc->substr(BodyBegin, BodyEnd - BodyBegin + 1);
+}
+
 // Returns the canonical declaration for the given FunctionDecl. This will
 // usually be the first declaration in current translation unit with the
 // exception of template specialization.
 // For those we return first declaration different than the canonical one.
 // Because canonical declaration points to template decl instead of
 // specialization.
 const FunctionDecl *findTarget(const FunctionDecl *FD) {
   auto CanonDecl = FD->getCanonicalDecl();
   if (!FD->isFunctionTemplateSpecialization())
     return CanonDecl;
   // For specializations CanonicalDecl is the TemplatedDecl, which is not the
   // target we want to inline into. Instead we traverse previous decls to find
   // the first forward decl for this specialization.
   auto PrevDecl = FD;
   while (PrevDecl->getPreviousDecl() != CanonDecl) {
     PrevDecl = PrevDecl->getPreviousDecl();
     assert(PrevDecl && "Found specialization without template decl");
   }
   return PrevDecl;
 }
 
+// Returns the begining location for a FunctionDecl. Returns location of
+// template keyword for templated functions.
+const SourceLocation getBeginLoc(const FunctionDecl *FD) {
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+    return FTD->getBeginLoc();
+  return FD->getBeginLoc();
+}
+
 /// Moves definition of a function/method to its declaration location.
 /// Before:
 /// a.h:
 ///   void foo();
 ///
 /// a.cc:
 ///   void foo() { return; }
 ///
 /// ------------------------
 /// After:
 /// a.h:
 ///   void foo() { return; }
 ///
 /// a.cc:
 ///
 class DefineInline : public Tweak {
 public:
   const char *id() const override final;
 
   Intent intent() const override { return Intent::Refactor; }
   std::string title() const override {
     return "Move function body to declaration";
   }
-  bool hidden() const override { return true; }
 
   // Returns true when selection is on a function definition that does not
   // make use of any internal symbols.
   bool prepare(const Selection &Sel) override {
     const SelectionTree::Node *SelNode = Sel.ASTSelection.commonAncestor();
     if (!SelNode)
       return false;
     Source = getSelectedFunction(SelNode);
bool prepare(const Selection &Sel) override {
     if (!checkDeclsAreVisible(getNonLocalDeclRefs(Sel.AST, Source), Target,
                               Sel.AST.getSourceManager()))
       return false;
 
     return true;
   }
 
   Expected<Effect> apply(const Selection &Sel) override {
-    return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                   "Not implemented yet");
+    const auto &AST = Sel.AST.getASTContext();
+    const auto &SM = AST.getSourceManager();
+
+    auto SemiColon = getSemicolonForDecl(Target);
+    if (!SemiColon) {
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Couldn't find semicolon for target declaration");
+    }
+
+    auto QualifiedBody = qualifyAllDecls(Source);
+    if (!QualifiedBody)
+      return QualifiedBody.takeError();
+
+    const tooling::Replacement SemiColonToFuncBody(SM, *SemiColon, 1,
+                                                   *QualifiedBody);
+    SourceLocation BeginLoc = getBeginLoc(Source);
+    unsigned int SourceLen =
+        SM.getFileOffset(Source->getEndLoc()) - SM.getFileOffset(BeginLoc) + 1;
+    const tooling::Replacement DeleteFuncBody(SM, BeginLoc, SourceLen, "");
+
+    llvm::SmallVector<std::pair<std::string, Edit>, 2> Edits;
+    // Edit for Target.
+    auto FE = Effect::fileEdit(SM, SM.getFileID(*SemiColon),
+                               tooling::Replacements(SemiColonToFuncBody));
+    if (!FE)
+      return FE.takeError();
+    Edits.push_back(std::move(*FE));
+
+    // Edit for Source.
+    if (!SM.isWrittenInSameFile(Source->getBeginLoc(), Target->getBeginLoc())) {
+      // Generate a new edit if the Source and Target are in different files.
+      auto FE = Effect::fileEdit(SM, SM.getFileID(Sel.Cursor),
+                                 tooling::Replacements(DeleteFuncBody));
+      if (!FE)
+        return FE.takeError();
+      Edits.push_back(std::move(*FE));
+    } else {
+      // Merge with previous edit if they are in the same file.
+      if (auto Err = Edits.front().second.Replacements.add(DeleteFuncBody))
+        return std::move(Err);
+    }
+
+    Effect E;
+    for (auto &Pair : Edits)
+      E.ApplyEdits.try_emplace(std::move(Pair.first), std::move(Pair.second));
+    return E;
   }
 
 private:
   const FunctionDecl *Source = nullptr;
   const FunctionDecl *Target = nullptr;
 };
 
 REGISTER_TWEAK(DefineInline);
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/TweakTesting.h

protected:
   // Extra flags passed to the compilation in apply().
   std::vector<const char *> ExtraArgs;
 
   // Context in which snippets of code should be placed to run tweaks.
   CodeContext Context = File;
 
   // Apply the current tweak to the range (or point) in MarkedCode.
   // MarkedCode will be wrapped according to the Context.
-  //  - if the tweak produces edits, returns the edited code (without markings).
+  //  - if the tweak produces edits, returns the edited code (without markings)
+  //    for the main file.
+  //    Populates \p EditedFiles if there were changes to other files whenever
+  //    it is non-null. It is a mapping from absolute path of the edited file to
+  //    its new contents.
   //    The context added to MarkedCode will be stripped away before returning,
   //    unless the tweak edited it.
   //  - if the tweak produces a message, returns "message:\n<message>"
   //  - if prepare() returns false, returns "unavailable"
   //  - if apply() returns an error, returns "fail: <message>"
-  std::string apply(llvm::StringRef MarkedCode) const;
+  std::string apply(llvm::StringRef MarkedCode,
+                    llvm::StringMap<std::string> *EditedFiles = nullptr) const;
 
   // Accepts a code snippet with many ranges (or points) marked, and returns a
   // list of snippets with one range marked each.
   // Primarily used from EXPECT_AVAILABLE/EXPECT_UNAVAILABLE macro.
   static std::vector<std::string> expandCases(llvm::StringRef MarkedCode);
 
   // Returns a matcher that accepts marked code snippets where the tweak is
   // available at the marked range.

clang-tools-extra/clangd/unittests/TweakTesting.cpp

 //===-- TweakTesting.cpp ------------------------------------------------*-===//
 //
 // 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 "TweakTesting.h"
 
 #include "Annotations.h"
 #include "SourceCode.h"
+#include "TestFS.h"
 #include "refactor/Tweak.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/Support/Error.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 using Context = TweakTest::CodeContext;
 
 std::pair<llvm::StringRef, llvm::StringRef> wrapping(Context Ctx) {
   switch (Ctx) {
MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles,
   if (PrepareResult)
     return true;
   llvm::consumeError(PrepareResult.takeError());
   return false;
 }
 
 } // namespace
 
-std::string TweakTest::apply(llvm::StringRef MarkedCode) const {
+std::string TweakTest::apply(llvm::StringRef MarkedCode,
+                             llvm::StringMap<std::string> *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
   TU.HeaderCode = Header;
+  TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
   Tweak::Selection S(AST, Selection.first, Selection.second);
 
   auto T = prepareTweak(TweakID, S);
   if (!T) {
     llvm::consumeError(T.takeError());
     return "unavailable";
   }
   llvm::Expected<Tweak::Effect> Result = (*T)->apply(S);
   if (!Result)
     return "fail: " + llvm::toString(Result.takeError());
   if (Result->ShowMessage)
     return "message:\n" + *Result->ShowMessage;
   if (Result->ApplyEdits.empty())
     return "no effect";
-  if (Result->ApplyEdits.size() > 1)
-    return "received multi-file edits";
 
-  auto ApplyEdit = Result->ApplyEdits.begin()->second;
-  if (auto NewText = ApplyEdit.apply())
-    return unwrap(Context, *NewText);
-  else
+  std::string EditedMainFile;
+  for (auto &It : Result->ApplyEdits) {
+    auto NewText = It.second.apply();
+    if (!NewText)
       return "bad edits: " + llvm::toString(NewText.takeError());
+    llvm::StringRef Unwrapped = unwrap(Context, *NewText);
+    if (It.first() == testPath(TU.Filename))
+      EditedMainFile = Unwrapped;
+    else if (EditedFiles)
+      EditedFiles->try_emplace(It.first(), Unwrapped.str());
+  }
+  return EditedMainFile;
 }
 
 ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header,
                           ExtraFiles);
 }
 
 std::vector<std::string> TweakTest::expandCases(llvm::StringRef MarkedCode) {

clang-tools-extra/clangd/unittests/TweakTests.cpp

 #include "clang/Basic/FileSystemOptions.h"
 #include "clang/Basic/LLVM.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
 #include "llvm/ADT/StringExtras.h"
+#include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock-matchers.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cassert>
 #include <string>
 #include <vector>
 
 using ::testing::AllOf;
 using ::testing::HasSubstr;
 using ::testing::StartsWith;
+using ::testing::ElementsAre;
 
 namespace clang {
 namespace clangd {
 namespace {
 
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
 TEST(FileEdits, AbsolutePath) {
   auto RelPaths = {"a.h", "foo.cpp", "test/test.cpp"};
 
   llvm::IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> MemFS(
       new llvm::vfs::InMemoryFileSystem);
   MemFS->setCurrentWorkingDirectory(testRoot());
   for (auto Path : RelPaths)
     MemFS->addFile(Path, 0, llvm::MemoryBuffer::getMemBuffer("", Path));
TEST_F(DefineInlineTest, UsingShadowDecls) {
   using ns2::foo;
 
   template <typename T>
   void b^ar() {
     foo(T());
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformNestedNamespaces) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+    void bar();
+    namespace b {
+      void baz();
+      namespace c {
+        void aux();
+      }
+    }
+  }
+
+  void foo();
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  void f^oo() {
+    bar();
+    a::bar();
+
+    baz();
+    b::baz();
+    a::b::baz();
+
+    aux();
+    c::aux();
+    b::c::aux();
+    a::b::c::aux();
+  })cpp"), R"cpp(
+  namespace a {
+    void bar();
+    namespace b {
+      void baz();
+      namespace c {
+        void aux();
+      }
+    }
+  }
+
+  void foo(){
+    a::bar();
+    a::bar();
+
+    a::b::baz();
+    a::b::baz();
+    a::b::baz();
+
+    a::b::c::aux();
+    a::b::c::aux();
+    a::b::c::aux();
+    a::b::c::aux();
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo();
+  void f^oo() {
+    using namespace a;
+    using namespace b;
+    using namespace c;
+    using c::aux;
+    namespace d = c;
+  })cpp"),
+            R"cpp(
+  namespace a { namespace b { namespace c { void aux(); } } }
+
+  void foo(){
+    using namespace a;
+    using namespace a::b;
+    using namespace a::b::c;
+    using a::b::c::aux;
+    namespace d = a::b::c;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo();
+  void f^oo() {
+    class Foo {
+    public:
+      void foo();
+      int x;
+      static int y;
+    };
+    Foo::y = 0;
+
+    enum En { Zero, One };
+    En x = Zero;
+
+    enum class EnClass { Zero, One };
+    EnClass y = EnClass::Zero;
+  })cpp"),
+            R"cpp(
+  void foo(){
+    class Foo {
+    public:
+      void foo();
+      int x;
+      static int y;
+    };
+    Foo::y = 0;
+
+    enum En { Zero, One };
+    En x = Zero;
+
+    enum class EnClass { Zero, One };
+    EnClass y = EnClass::Zero;
+  }
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      void bar();
+    };
+    template <typename T> T bar;
+    template <typename T> void aux() {}
+  }
+
+  void foo();
+
+  using namespace a;
+  void f^oo() {
+    bar<Bar<int>>.bar();
+    aux<Bar<int>>();
+  })cpp"),
+            R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      void bar();
+    };
+    template <typename T> T bar;
+    template <typename T> void aux() {}
+  }
+
+  void foo(){
+    a::bar<a::Bar<int>>.bar();
+    a::aux<a::Bar<int>>();
+  }
+
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+    void foo();
+  };
+
+  void Foo::f^oo() {
+    return;
+  }
+  )cpp"),
+            R"cpp(
+  class Foo {
+    void foo(){
+    return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+    void foo();
+  };)cpp";
+
+  llvm::StringMap<std::string> EditedFiles;
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+    return;
+  })cpp",
+                  &EditedFiles),
+            R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+              ElementsAre(FileWithContents(testPath("a.h"),
+                                                    R"cpp(
+  class Foo {
+    void foo(){
+    return;
+  }
+  };)cpp")));
+}
+
+TEST_F(DefineInlineTest, TransformDependentTypes) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+    template <typename T> class Bar {};
+  }
+  using namespace a;
+
+  template <typename T>
+  void foo();
+
+  template <typename T>
+  void f^oo() {
+    Bar<T> B;
+    Bar<Bar<T>> q;
+  }
+  )cpp"),
+            R"cpp(
+  namespace a {
+    template <typename T> class Bar {};
+  }
+  using namespace a;
+
+  template <typename T>
+  void foo(){
+    a::Bar<T> B;
+    a::Bar<a::Bar<T>> q;
+  }
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformFunctionTempls) {
+  EXPECT_EQ(apply(R"cpp(
+  template <typename T>
+  void foo(T p);
+
+  template <>
+  void foo<int>(int p);
+
+  template <>
+  void foo<char>(char p);
+
+  template <>
+  void fo^o<int>(int p) {
+    return;
+  }
+  )cpp"),
+            R"cpp(
+  template <typename T>
+  void foo(T p);
+
+  template <>
+  void foo<int>(int p){
+    return;
+  }
+
+  template <>
+  void foo<char>(char p);
+
+  
+  )cpp");
+
+  EXPECT_EQ(apply(R"cpp(
+  template <typename T>
+  void foo(T p);
+
+  template <>
+  void foo<int>(int p);
+
+  template <>
+  void foo<char>(char p);
+
+  template <>
+  void fo^o<char>(char p) {
+    return;
+  }
+  )cpp"),
+            R"cpp(
+  template <typename T>
+  void foo(T p);
+
+  template <>
+  void foo<int>(int p);
+
+  template <>
+  void foo<char>(char p){
+    return;
+  }
+
+  
+  )cpp");
+
+  EXPECT_EQ(apply(R"cpp(
+  template <typename T>
+  void foo(T p);
+
+  template <>
+  void foo<int>(int p);
+
+  template <typename T>
+  void fo^o(T p) {
+    return;
+  }
+  )cpp"),
+            R"cpp(
+  template <typename T>
+  void foo(T p){
+    return;
+  }
+
+  template <>
+  void foo<int>(int p);
+
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTypeLocs) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      template <typename Q> class Baz {};
+    };
+    class Foo{};
+  }
+
+  void foo();
+
+  using namespace a;
+  void f^oo() {
+    Bar<int> B;
+    Foo foo;
+    a::Bar<Bar<int>>::Baz<Bar<int>> q;
+  })cpp"),
+            R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      template <typename Q> class Baz {};
+    };
+    class Foo{};
+  }
+
+  void foo(){
+    a::Bar<int> B;
+    a::Foo foo;
+    a::Bar<a::Bar<int>>::Baz<a::Bar<int>> q;
+  }
+
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDeclRefs) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      void foo();
+      static void bar();
+      int x;
+      static int y;
+    };
+    void bar();
+    void test();
+  }
+
+  void foo();
+  using namespace a;
+  void f^oo() {
+    a::Bar<int> B;
+    B.foo();
+    a::bar();
+    Bar<Bar<int>>::bar();
+    a::Bar<int>::bar();
+    B.x = Bar<int>::y;
+    Bar<int>::y = 3;
+    bar();
+    a::test();
+  })cpp"),
+            R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      void foo();
+      static void bar();
+      int x;
+      static int y;
+    };
+    void bar();
+    void test();
+  }
+
+  void foo(){
+    a::Bar<int> B;
+    B.foo();
+    a::bar();
+    a::Bar<a::Bar<int>>::bar();
+    a::Bar<int>::bar();
+    B.x = a::Bar<int>::y;
+    a::Bar<int>::y = 3;
+    a::bar();
+    a::test();
+  }
+  using namespace a;
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, StaticMembers) {
+  EXPECT_EQ(apply(R"cpp(
+    namespace ns { class X { static void foo(); void bar(); }; }
+    void ns::X::b^ar() {
+      foo();
+    })cpp"), R"cpp(
+    namespace ns { class X { static void foo(); void bar(){
+      foo();
+    } }; }
+    )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformInlineNamespaces) {
+  EXPECT_EQ(apply(R"cpp(
+    namespace a { inline namespace b { namespace { struct Foo{}; } } }
+    void foo();
+
+    using namespace a;
+    void ^foo() {Foo foo;})cpp"), R"cpp(
+    namespace a { inline namespace b { namespace { struct Foo{}; } } }
+    void foo(){a::Foo foo;}
+
+    using namespace a;
+    )cpp");
+}
+
+TEST_F(DefineInlineTest, TokensBeforeSemicolon) {
+  EXPECT_EQ(apply(R"cpp(
+  #define MACRO
+  void foo()    MACRO /*Comment -_-*/ MACRO ;
+  void fo^o() { return ; })cpp"),
+            R"cpp(
+  #define MACRO
+  void foo()    MACRO /*Comment -_-*/ MACRO { return ; }
+  )cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang