[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

66–67

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

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?

101

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

120–131

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.

133

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

137

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?

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

1975

can't we merge these into the above ApplyTest?

2007

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

137

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

1975

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

2007

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

137

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

2005

nit: ";"

2007

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);
 }
 
-// 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) {
-  auto &SM = FD->getASTContext().getSourceManager();
-  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
-                                       FD->getSourceRange());
-  if (!CharRange)
+// Synthesize a DeclContext for TargetNS from CurContext. TargetNS must be empty
+// for global namespace, and endwith "::" otherwise.
+// Returns None if TargetNS is not a prefix of CurContext.
+llvm::Optional<const DeclContext *>
+findContextForNS(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 is empty, it means global ns, which is translation unit.
+  if (TargetNS.empty()) {
+    while (!CurContext->isTranslationUnit())
+      CurContext = CurContext->getParent();
+    return CurContext;
+  }
+  // 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;
+}
+
+// Returns source code for FD after applying Replacements.
+// FIXME: Make the function take a parameter to return only the function body,
+// afterwards it can be shared with define-inline code action.
+llvm::Expected<std::string>
+getFunctionSourceAfterReplacements(const FunctionDecl *FD,
+                                   const tooling::Replacements &Replacements) {
+  const auto &SM = FD->getASTContext().getSourceManager();
+  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());
+
+  // Get new begin and end positions for the qualified function definition.
+  unsigned FuncBegin = SM.getFileOffset(OrigFuncRange->getBegin());
+  unsigned FuncEnd = Replacements.getShiftedCodePosition(
+      SM.getFileOffset(OrigFuncRange->getEnd()));
+
+  // Trim the result to function definition.
+  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);
+// 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.
+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.
+  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 = QualifierInsertions.add(
+            tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
+      Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+  });
+
+  if (Errors)
+    return std::move(Errors);
+  return getFunctionSourceAfterReplacements(FD, QualifierInsertions);
 }
 
+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