[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

164

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

77

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

177

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?)

183

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.

184

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.

295

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?

376

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

177

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

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

77

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

77

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.

167

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

173

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

180

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

181

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?

194

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?

197

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.

221

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

224

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

194

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

224

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

224

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?

180

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

207

NIT: braces are redundant

361–412

NIT: use auto

366

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

894

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

911

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

950

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

953

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

978

Maybe move this matcher to the start of the file?

1018

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

1065

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?

1156

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

1204

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"?

207

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

911

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

1065

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");

1204

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

207

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

911

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.

1204

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

364

NIT: s/SemiColon/Semicolon

375

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

71

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

989

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

1029

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

1095

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?

1137

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

375

see macro tests for the behavior in such cases.

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

1095

yes that's the point, adding a comment.

1137

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

364

This one is still there.

375

s/SemiColon/Semicolon

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

1095

Thanks!

1137

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 {
 
+// Lexes from end of \p FD until it finds a semicolon.
+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))
+      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>() &&
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;
+  std::string Failure;
+  findExplicitReferences(FD->getBody(), [&](ReferenceLoc Ref) {
+    if (!Failure.empty())
+      return;
+    // Since we want to qualify only the first qualifier, skip names with a
+    // qualifier.
+    if (Ref.Qualifier.hasQualifier())
+      return;
+    // There might be no decl in dependent contexts, there's nothing much we can
+    // do in such cases.
+    if (Ref.Targets.empty())
+      return;
+
+    // All Targets should be in the same nested name scope, so we can safely
+    // chose any one of them.
+    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))) {
+      Failure = llvm::formatv("Failed to add qualifier: {0}",
+                              llvm::fmt_consume(std::move(Err)));
+      elog(Failure.c_str());
+    }
+  });
+
+  if (!Failure.empty())
+    return llvm::createStringError(llvm::inconvertibleErrorCode(), Failure);
+
+  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);
+}
+
+/// Generates Replacements for changing parameter names in \p Dest to be the
+/// same as in \p Source.
+llvm::Expected<tooling::Replacements>
+renameParameters(const FunctionDecl *Dest, const FunctionDecl *Source) {
+  const SourceManager &SM = Dest->getASTContext().getSourceManager();
+  tooling::Replacements Replacements;
+  auto GenerateReplacements = [&](const NamedDecl *Dest,
+                                  const NamedDecl *Source) -> llvm::Error {
+    llvm::StringRef DestName = Dest->getName();
+    llvm::StringRef SourceName = Source->getName();
+    if (DestName == SourceName)
+      return llvm::Error::success();
+    if (auto Err = Replacements.add(tooling::Replacement(
+            SM, Dest->getLocation(), DestName.size(),
+            ((DestName.empty() ? " " : "") + SourceName).str()))) {
+      return Err;
+    }
+    return llvm::Error::success();
+  };
+
+  // Fix function parameter names.
+  assert(Dest->param_size() == Source->param_size());
+  for (size_t I = 0, E = Dest->param_size(); I != E; ++I) {
+    if (auto Err = GenerateReplacements(Dest->getParamDecl(I),
+                                        Source->getParamDecl(I)))
+      return std::move(Err);
+  }
+
+  // Fix function template parameter list.
+  auto *DestTempl = Dest->getDescribedFunctionTemplate();
+  auto *SourceTempl = Source->getDescribedFunctionTemplate();
+  assert(bool(DestTempl) == bool(SourceTempl));
+
+  // Nothing to do if function is not a template.
+  if (!DestTempl)
+    return Replacements;
+
+  const auto *DestTPL = DestTempl->getTemplateParameters();
+  const auto *SourceTPL = SourceTempl->getTemplateParameters();
+  assert(DestTPL->size() == SourceTPL->size());
+
+  for (size_t I = 0, EP = DestTPL->size(); I != EP; ++I) {
+    if (auto Err =
+            GenerateReplacements(DestTPL->getParam(I), SourceTPL->getParam(I)))
+      return std::move(Err);
+  }
+  return Replacements;
+}
+
 // 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 ASTContext &AST = Sel.AST.getASTContext();
+    const SourceManager &SM = AST.getSourceManager();
+
+    auto SemiColon = getSemiColonForDecl(Target);
+    if (!SemiColon) {
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Couldn't find semicolon for target declaration");
+    }
+
+    auto ParamReplacements = renameParameters(Target, Source);
+    if (!ParamReplacements)
+      return ParamReplacements.takeError();
+
+    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).merge(*ParamReplacements));
+    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

enum CodeContext {
     // Snippet will appear within a function body. e.g. a statement.
     Function,
     // Snippet is an expression.
     Expression,
   };
 
   // Mapping from file name to contents.
   llvm::StringMap<std::string> ExtraFiles;
+  llvm::StringMap<std::string> EditedFiles;
 
 protected:
   TweakTest(const char *TweakID) : TweakID(TweakID) {}
 
   // Contents of a header file to be implicitly included.
   // This typically contains declarations that will be used for a set of related
   // testcases.
   std::string Header;
 
   // 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 any other
+  //    changes.
   //    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);
 
   // 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) {
   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();
   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
+      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"

 TEST_F(RawStringLiteralTest, Test) {
   Context = Expression;
   EXPECT_AVAILABLE(R"cpp(^"^f^o^o^\^n^")cpp");
   EXPECT_AVAILABLE(R"cpp(R"(multi )" ^"token " "str\ning")cpp");
   EXPECT_UNAVAILABLE(R"cpp(^"f^o^o^o")cpp"); // no chars need escaping
   EXPECT_UNAVAILABLE(R"cpp(R"(multi )" ^"token " u8"str\ning")cpp"); // nonascii
   EXPECT_UNAVAILABLE(R"cpp(^R^"^(^multi )" "token " "str\ning")cpp"); // raw
   EXPECT_UNAVAILABLE(R"cpp(^"token\n" __FILE__)cpp"); // chunk is macro
-  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp"); // forbidden escape char
+  EXPECT_UNAVAILABLE(R"cpp(^"a\r\n";)cpp");           // forbidden escape char
 
   const char *Input = R"cpp(R"(multi
 token)" "\nst^ring\n" "literal")cpp";
   const char *Output = R"cpp(R"(multi
 token
 string
 literal)")cpp";
   EXPECT_EQ(apply(Input), Output);
TEST_F(DefineInlineTest, UsingShadowDecls) {
   using ns2::foo;
 
   template <typename T>
   void b^ar() {
     foo(T());
   })cpp");
 }
 
+TEST_F(DefineInlineTest, TransformUsings) {
+  EXPECT_EQ(apply(R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo();
+  void f^oo() {
+    using namespace a;
+
+    using namespace b;
+    using namespace a::b;
+
+    using namespace c;
+    using namespace b::c;
+    using namespace a::b::c;
+
+    using a::bar;
+
+    using b::baz;
+    using a::b::baz;
+
+    using c::aux;
+    using b::c::aux;
+    using a::b::c::aux;
+
+    namespace d = c;
+    namespace d = b::c;
+  }
+  )cpp"),
+            R"cpp(
+  namespace a {
+  void bar();
+  namespace b {
+  void baz();
+  namespace c {
+  void aux();
+  }
+  }
+  }
+
+  void foo(){
+    using namespace a;
+
+    using namespace a::b;
+    using namespace a::b;
+
+    using namespace a::b::c;
+    using namespace a::b::c;
+    using namespace a::b::c;
+
+    using a::bar;
+
+    using a::b::baz;
+    using a::b::baz;
+
+    using a::b::c::aux;
+    using a::b::c::aux;
+    using a::b::c::aux;
+
+    namespace d = a::b::c;
+    namespace d = a::b::c;
+  }
+  
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformDecls) {
+  EXPECT_EQ(apply(R"cpp(
+  void foo()    /*Comment -_-*/  ;
+
+  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;
+
+    template <typename T> class Bar {};
+    Bar<int> z;
+  }
+  )cpp"),
+            R"cpp(
+  void foo()    /*Comment -_-*/  {
+    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;
+
+    template <typename T> class Bar {};
+    Bar<int> z;
+  }
+
+  
+  )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()    /*Comment -_-*/  ;
+
+  void f^oo() {
+    using namespace a;
+    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()    /*Comment -_-*/  {
+    using namespace a;
+    a::bar<a::Bar<int>>.bar();
+    a::aux<a::Bar<int>>();
+  }
+
+  
+  )cpp");
+}
+
+MATCHER_P2(FileWithContents, FileName, Contents, "") {
+  return arg.first() == FileName && arg.second == Contents;
+}
+
+TEST_F(DefineInlineTest, TransformMembers) {
+  EXPECT_EQ(apply(R"cpp(
+  class Foo {
+    void foo()    /*Comment -_-*/  ;
+  };
+
+  void Foo::f^oo() {
+    return;
+  }
+  )cpp"),
+            R"cpp(
+  class Foo {
+    void foo()    /*Comment -_-*/  {
+    return;
+  }
+  };
+
+  
+  )cpp");
+
+  ExtraFiles["a.h"] = R"cpp(
+  class Foo {
+    void foo()    /*Comment -_-*/  ;
+  };)cpp";
+  EXPECT_EQ(apply(R"cpp(
+  #include "a.h"
+  void Foo::f^oo() {
+    return;
+  })cpp"),
+            R"cpp(
+  #include "a.h"
+  )cpp");
+  EXPECT_THAT(EditedFiles,
+              testing::ElementsAre(FileWithContents(testPath("a.h"),
+                                                    R"cpp(
+  class Foo {
+    void foo()    /*Comment -_-*/  {
+    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()    /*Comment -_-*/  ;
+
+  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()    /*Comment -_-*/  {
+    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 {};
+    };
+    namespace b{
+    class Foo{};
+    };
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+
+  void foo()    /*Comment -_-*/  ;
+
+  void f^oo() {
+    Bar<int> B;
+    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 {};
+    };
+    namespace b{
+    class Foo{};
+    };
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+
+  void foo()    /*Comment -_-*/  {
+    a::Bar<int> B;
+    a::b::Foo foo;
+    a::Bar<a::Bar<int>>::Baz<a::Bar<int>> q;
+  }
+
+  
+  )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();
+    namespace b {
+    class Foo{};
+    namespace c {
+    void test();
+    }
+    }
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+
+  void foo()    /*Comment -_-*/  ;
+
+  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();
+    c::test();
+  }
+  )cpp"),
+            R"cpp(
+  namespace a {
+    template <typename T> class Bar {
+    public:
+      void foo();
+      static void bar();
+      int x;
+      static int y;
+    };
+    void bar();
+    namespace b {
+    class Foo{};
+    namespace c {
+    void test();
+    }
+    }
+  }
+  using namespace a;
+  using namespace b;
+  using namespace c;
+
+  void foo()    /*Comment -_-*/  {
+    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::b::c::test();
+  }
+
+  
+  )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, TransformParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+    void foo(int, bool b);
+
+    void ^foo(int f, bool x) {})cpp"), R"cpp(
+    void foo(int f, bool x){}
+
+    )cpp");
+}
+
+TEST_F(DefineInlineTest, TransformTemplParamNames) {
+  EXPECT_EQ(apply(R"cpp(
+    struct Foo {
+      struct Bar {
+        template <class, class X,
+                  template<typename> class, template<typename> class Y,
+                  int, int Z>
+        void foo();
+      };
+    };
+
+    template <class T, class U,
+              template<typename> class V, template<typename> class W,
+              int X, int Y>
+    void Foo::Bar::f^oo() {})cpp"), R"cpp(
+    struct Foo {
+      struct Bar {
+        template <class T, class U,
+                  template<typename> class V, template<typename> class W,
+                  int X, int Y>
+        void foo(){}
+      };
+    };
+
+    )cpp");
+}
 } // namespace
 } // namespace clangd
 } // namespace clang