[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

2028

maybe name them TestHeader, and TestSource.

2051

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

getFunctionSourceAfterReplacements(const FunctionDecl *FD,
   return QualifiedFunc->substr(FuncBegin, FuncEnd - FuncBegin + 1);
 }
 
 // Creates a modified version of function definition that can be inserted at a
 // different location, qualifies return value and function name to achieve that.
 // Contains function signature, body and template parameters if applicable.
 // No need to qualify parameters, as they are looked up in the context
 // containing the function/method.
-// FIXME: Qualify function name depending on the target context.
 llvm::Expected<std::string>
 getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   auto &SM = FD->getASTContext().getSourceManager();
   auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
   if (!TargetContext)
     return llvm::createStringError(
         llvm::inconvertibleErrorCode(),
         "define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements QualifierInsertions;
 
-  // Finds the first unqualified name in function return type and qualifies it
-  // to be valid in TargetContext.
+  // Finds the first unqualified name in function return type and name, then
+  // qualifies those to be valid in TargetContext.
   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;
       }
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 TestHeader;
+    llvm::StringRef TestSource;
+    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.TestSource;
+    EXPECT_EQ(apply(Case.TestHeader, &EditedFiles), Case.ExpectedHeader);
+    EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+                                 testPath("Test.cpp"), Case.ExpectedSource)))
+        << Case.TestHeader;
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang