[clangd] Define out-of-line qualify return value

[kadircet]

Summary

Return type might need qualification if insertion context doesn't have
the same decls visible as the source context.

This patch adds qualification for return value to cover such cases.

[kadircet]

• Rebase

[kadircet]

• Move TargetContext generation logic to a separate function

[merge_guards_bot]

Build result: FAILURE - Could not check out parent git hash "627221d6588eb650759f39e80858abb2a3848ccb". 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

[kadircet]

• Rebase

[hokein]

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

69

could you add some documentations, e.g. what's the requirement for the input TargetNS?

I'm not sure Synthesize is clear here, maybe lookupTargetContext or findTargetContext?

77

nit: maybe use early return?

105

could you also update the comments here, mentioning the function handles the return type qualifiers properly?

117

could we use a more meaningful name, maybe AddQualifierEdit? Would be nice to have some comments explaining what the following code does.

149

nit: I think this error message is exposed to users, I'm not sure "see logs for details" is friendly to them.

153

we have similar code in define-inline as well, would be nice to have them in a single place in the long term. probably a FIXME?

173

btw, do we need to qualify the function parameters. maybe it is not needed, but would be nice to have some brief comments explaining it.

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

1977

can't we merge these into the above ApplyTest?

2009

oh, this is very tricky case (I think you meant to test the public members), note that Bar and foo are private member of Foo, we can't move the body out of the class Foo, for this case I think we should disallow the tweak.

No need to do it in this patch, but please update the test here (to test public members).

[kadircet]

• Address comments

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

153

they're quite similar but, different in nature. one of them returns the full function definition, including template parameter lists, whereas the other only operates on function body.

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

1977

I would rather keep these separate, as these tests tends to get out of control otherwise, e.g. Hover.All

2009

I don't follow, the following compiles nicely:

namespace a {
class Foo {
  class Bar {};
  Bar foo();
};
}  // namespace a

a::Foo::Bar a::Foo::foo() { return {}; }

the problem here is we are not qualifying the function name, which is handled in the follow up patch D70656

[hokein]

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

153

yes, the only difference is the range, right? the logic of applying replacements, getting correct begin/end offset, and getting the interesting content is the same. we could abstract a function that taking the range into the parameter.

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

2007

nit: ";"

2009

ah, you are right. I think I was somehow confused with the a::Foo::Bar foo().

[kadircet]

• Extract replacement applying part to a new function and add a fixme for sharing it with define inline code-action.

[merge_guards_bot]

Build result: fail - 60408 tests passed, 1 failed and 726 were skipped.

failed: Clangd Unit Tests._/ClangdTests/DefineOutlineTest.QualifyReturnValue

Log files: console-log.txt, CMakeCache.txt

[kadircet]

• Fix tests

[merge_guards_bot]

Build result: pass - 60447 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

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

 //===--- DefineOutline.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 "HeaderSourceSwitch.h"
+#include "Logger.h"
 #include "Path.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #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 <cstddef>
+#include <string>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // 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.
llvm::Optional<Path> getSourceFile(llvm::StringRef FileName,
                                    const Tweak::Selection &Sel) {
   if (auto Source = getCorrespondingHeaderOrSource(
           FileName,
           &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
     return *Source;
   return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
 }
 
+// Synthesize a DeclContext for TargetNS from CurContext.
+llvm::Optional<const DeclContext *>
+synthesizeContextForNS(llvm::StringRef TargetNS,
+                       const DeclContext *CurContext) {
+  assert(TargetNS.empty() || TargetNS.endswith("::"));
+  // Skip any non-namespace contexts, e.g. TagDecls, functions/methods.
+  CurContext = CurContext->getEnclosingNamespaceContext();
+  if (TargetNS.empty()) {
+    // If TargetNS is empty, it means global ns, which is translation unit.
+    while (!CurContext->isTranslationUnit())
+      CurContext = CurContext->getParent();
+  } else {
+    // Otherwise we need to drop any trailing namespaces from CurContext until
+    // we reach TargetNS.
+    std::string TargetContextNS =
+        CurContext->isNamespace()
+            ? llvm::cast<NamespaceDecl>(CurContext)->getQualifiedNameAsString()
+            : "";
+    TargetContextNS.append("::");
+
+    llvm::StringRef CurrentContextNS(TargetContextNS);
+    // If TargetNS is not a prefix of CurrentContext, there's no way to reach
+    // it.
+    if (!CurrentContextNS.startswith(TargetNS))
+      return llvm::None;
+
+    while (CurrentContextNS != TargetNS) {
+      CurContext = CurContext->getParent();
+      // These colons always exists since TargetNS is a prefix of
+      // CurrentContextNS, it ends with "::" and they are not equal.
+      CurrentContextNS = CurrentContextNS.take_front(
+          CurrentContextNS.drop_back(2).rfind("::") + 2);
+    }
+  }
+  return CurContext;
+}
+
 // Creates a modified version of function definition that can be inserted at a
 // different location. Contains both function signature and body.
-llvm::Optional<llvm::StringRef> getFunctionSourceCode(const FunctionDecl *FD) {
+llvm::Expected<std::string>
+getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace) {
   auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-                                       FD->getSourceRange());
-  if (!CharRange)
-    return llvm::None;
+  auto TargetContext =
+      synthesizeContextForNS(TargetNamespace, FD->getDeclContext());
+  if (!TargetContext)
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "define outline: couldn't synthesize a context for target");
+
+  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())
+      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;
+      }
+    }
+    const NamedDecl *ND = Ref.Targets.front();
+    const std::string Qualifier =
+        getQualification(FD->getASTContext(), *TargetContext,
+                         SM.getLocForStartOfFile(SM.getMainFileID()), ND);
+    if (auto Err = Replacements.add(
+            tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier))) {
+      elog("define outline: Failed to add quals: {0}", std::move(Err));
+      HadErrors = true;
+    }
+  });
+
+  if (HadErrors) {
+    return llvm::createStringError(
+        llvm::inconvertibleErrorCode(),
+        "define outline: Failed to compute qualifiers see logs for details.");
+  }
+
+  // Get new begin and end positions for the qualified body.
+  auto OrigFuncRange = toHalfOpenFileRange(
+      SM, FD->getASTContext().getLangOpts(), FD->getSourceRange());
+  if (!OrigFuncRange)
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Couldn't get range for function.");
   // Include template parameter list.
   if (auto *FTD = FD->getDescribedFunctionTemplate())
-    CharRange->setBegin(FTD->getBeginLoc());
+    OrigFuncRange->setBegin(FTD->getBeginLoc());
+
+  unsigned FuncBegin = SM.getFileOffset(OrigFuncRange->getBegin());
+  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 return type.
   // FIXME: Qualify function name depending on the target context.
-  return toSourceCode(SM, *CharRange);
 }
 
+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
 // should also try to follow ordering of declarations. For example, if decls
 // come in order `foo, bar, baz` then this function should return some point
 // between foo and baz for inserting bar.
-llvm::Expected<size_t> getInsertionOffset(llvm::StringRef Contents,
-                                          llvm::StringRef QualifiedName,
-                                          const format::FormatStyle &Style) {
+llvm::Expected<InsertionPoint>
+getInsertionPoint(llvm::StringRef Contents, llvm::StringRef QualifiedName,
   auto Region = getEligiblePoints(Contents, QualifiedName, Style);
 
   assert(!Region.EligiblePoints.empty());
   // FIXME: This selection can be made smarter by looking at the definition
   // locations for adjacent decls to Source. Unfortunately psudeo parsing in
   // getEligibleRegions only knows about namespace begin/end events so we
   // can't match function start/end positions yet.
-  auto InsertionPoint = Region.EligiblePoints.back();
-  return positionToOffset(Contents, InsertionPoint);
+  auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
+  if (!Offset)
+    return Offset.takeError();
+  return InsertionPoint{Region.EnclosingNamespace, *Offset};
 }
 
 /// Moves definition of a function/method to an appropriate implementation file.
 ///
 /// Before:
 /// a.h
 ///   void foo() { return; }
 /// a.cc
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();
-    auto InsertionOffset =
-        getInsertionOffset(Contents, Source->getQualifiedNameAsString(),
-                           getFormatStyleForFile(*CCFile, Contents, &FS));
-    if (!InsertionOffset)
-      return InsertionOffset.takeError();
+    auto InsertionPoint =
+        getInsertionPoint(Contents, Source->getQualifiedNameAsString(),
+    if (!InsertionPoint)
+      return InsertionPoint.takeError();
 
-    auto FuncDef = getFunctionSourceCode(Source);
+    auto FuncDef =
+        getFunctionSourceCode(Source, InsertionPoint->EnclosingNamespace);
     if (!FuncDef)
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           "Couldn't get full source for function definition.");
 
     SourceManagerForFile SMFF(*CCFile, Contents);
-    const tooling::Replacement InsertFunctionDef(*CCFile, *InsertionOffset, 0,
-                                                 *FuncDef);
+    const tooling::Replacement InsertFunctionDef(
+        *CCFile, InsertionPoint->Offset, 0, *FuncDef);
     auto Effect = Effect::mainFileEdit(
         SMFF.get(), tooling::Replacements(InsertFunctionDef));
     if (!Effect)
       return Effect.takeError();
 
     // FIXME: We should also get rid of inline qualifier.
     const tooling::Replacement DeleteFuncBody(
         Sel.AST.getSourceManager(),

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

TEST_F(DefineOutlineTest, HandleMacros) {
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)));
   }
 }
 
+TEST_F(DefineOutlineTest, QualifyReturnValue) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  struct {
+    llvm::StringRef Test;
+    llvm::StringRef ExpectedHeader;
+    llvm::StringRef ExpectedSource;
+  } Cases[] = {
+      {R"cpp(
+        namespace a { class Foo; }
+        using namespace a;
+        Foo fo^o() { return; })cpp",
+       R"cpp(
+        namespace a { class Foo; }
+        using namespace a;
+        Foo foo() ;)cpp",
+       "a::Foo foo() { return; }"},
+      {R"cpp(
+        namespace a {
+          class Foo {
+            class Bar {};
+            Bar fo^o() { return {}; }
+          }
+        })cpp",
+       R"cpp(
+        namespace a {
+          class Foo {
+            class Bar {};
+            Bar foo() ;
+          }
+        })cpp",
+       "a::Foo::Bar 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)));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang