[clangd] Define out-of-line qualify function name

[kadircet]

Summary

When moving function definitions to a different context, the function
name might need a different spelling, for example in the header it might be:

namespace a {
  void foo() {}
}

And we might want to move it into a context which doesn't have namespace a as
a parent, then we must re-spell the function name, i.e:

void a::foo() {}

This patch implements a version of this which ignores using namespace
declarations in the source file.

[kadircet]

• Get rid of debug output

[merge_guards_bot]

Build result: FAILURE - Could not check out parent git hash "0a2c8c14d3aa16ca964a8e643f9a5bfdd5459de3". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

[merge_guards_bot]

Build result: FAILURE - Could not check out parent git hash "0a2c8c14d3aa16ca964a8e643f9a5bfdd5459de3". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

[hokein]

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

1831

maybe name them TestHeader, and TestSource.

1854

nit: could we keep each string per line? It would improve the code readability here.

[kadircet]

• Address comments

[kadircet]

• Update comments after rebase

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

 #include "clang/Driver/Types.h"
 #include "clang/Format/Format.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/raw_ostream.h"
 #include <cstddef>
 #include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Deduces the FunctionDecl from a selection. Requires either the function body
getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   bool HadErrors = false;
   tooling::Replacements Replacements;
   findExplicitReferences(FD, [&](ReferenceLoc Ref) {
     // It is enough to qualify the first qualifier, so skip references with a
     // qualifier. Also we can't do much if there are no targets or name is
     // inside a macro body.
     if (Ref.Qualifier || Ref.Targets.empty() || Ref.NameLoc.isMacroID())
       return;
-    // Qualify return type
-    if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin())
+    // Only qualify return type and function name.
+    if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
+        Ref.NameLoc != FD->getLocation())
       return;
 
     for (const NamedDecl *ND : Ref.Targets) {
       if (ND->getDeclContext() != Ref.Targets.front()->getDeclContext()) {
         elog("Targets from multiple contexts: {0}, {1}",
              printQualifiedName(*Ref.Targets.front()), printQualifiedName(*ND));
         return;
       }
unsigned FuncEnd = Replacements.getShiftedCodePosition(
       SM.getFileOffset(OrigFuncRange->getEnd()));
 
   // Trim the result to function body.
   auto QualifiedFunc = tooling::applyAllReplacements(
       SM.getBufferData(SM.getMainFileID()), Replacements);
   if (!QualifiedFunc)
     return QualifiedFunc.takeError();
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
-
-  // FIXME: Qualify function name depending on the target context.
 }
 
 struct InsertionPoint {
   std::string EnclosingNamespace;
   size_t Offset;
 };
 // Returns the most natural insertion point for \p QualifiedName in \p Contents.
 // This currently cares about only the namespace proximity, but in feature it
auto &FS =
         Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem();
     auto Buffer = FS.getBufferForFile(*CCFile);
     // FIXME: Maybe we should consider creating the implementation file if it
     // doesn't exist?
     if (!Buffer)
       return llvm::createStringError(Buffer.getError(),
                                      Buffer.getError().message());
     auto Contents = Buffer->get()->getBuffer();
+    llvm::errs() << "Got file: " << Contents << '\n';
     auto InsertionPoint =
         getInsertionPoint(Contents, Source->getQualifiedNameAsString(),
                           getFormatStyleForFile(*CCFile, Contents, &FS));
     if (!InsertionPoint)
       return InsertionPoint.takeError();
 
     auto FuncDef =
         getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
     if (!FuncDef)
-      return llvm::createStringError(
-          llvm::inconvertibleErrorCode(),
-          "Couldn't get full source for function definition.");
+      return FuncDef.takeError();
 
     SourceManagerForFile SMFF(*CCFile, Contents);
     const tooling::Replacement InsertFunctionDef(
         *CCFile, InsertionPoint->Offset, 0, *FuncDef);
     auto Effect = Effect::mainFileEdit(
         SMFF.get(), tooling::Replacements(InsertFunctionDef));
     if (!Effect)
       return Effect.takeError();

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

llvm::StringRef ExpectedSource;
         })cpp",
        R"cpp(
         namespace a {
           class Foo {
             class Bar {};
             Bar foo() ;
           }
         })cpp",
-       "a::Foo::Bar foo() { return {}; }\n"},
+       "a::Foo::Bar a::Foo::foo() { return {}; }\n"},
       {R"cpp(
         class Foo;
         Foo fo^o() { return; })cpp",
        R"cpp(
         class Foo;
         Foo foo() ;)cpp",
        "Foo foo() { return; }"},
   };
   llvm::StringMap<std::string> EditedFiles;
   for (auto &Case : Cases) {
     apply(Case.Test, &EditedFiles);
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)));
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyFunctionName) {
+  FileName = "Test.hpp";
+  struct {
+    llvm::StringRef Test;
+    llvm::StringRef SourceFile;
+    llvm::StringRef ExpectedHeader;
+    llvm::StringRef ExpectedSource;
+  } Cases[] = {
+      {R"cpp(
+        namespace a {
+          namespace b {
+            class Foo {
+              void fo^o() {}
+            };
+          }
+        })cpp",
+       "",
+       R"cpp(
+        namespace a {
+          namespace b {
+            class Foo {
+              void foo() ;
+            };
+          }
+        })cpp",
+       "void a::b::Foo::foo() {}\n"},
+      {"namespace a { namespace b { void f^oo() {} } }", "namespace a{}",
+       "namespace a { namespace b { void foo() ; } }",
+       "namespace a{void b::foo() {} }"},
+      {"namespace a { namespace b { void f^oo() {} } }", "using namespace a;",
+       "namespace a { namespace b { void foo() ; } }",
+       // FIXME: Take using namespace directives in the source file into
+       // account. This can be spelled as b::foo instead.
+       "using namespace a;void a::b::foo() {} "},
+  };
+  llvm::StringMap<std::string> EditedFiles;
+  for (auto &Case : Cases) {
+    ExtraFiles["Test.cpp"] = Case.SourceFile;
+    EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+    EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+                                 testPath("Test.cpp"), Case.ExpectedSource)))
+        << Case.Test;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang