[clangd] Move function body to out-of-line: unnamed class method incorrect moving

[denis-fatkulin]

Summary

The refactoring Move function body to out-of-line produces incorrect code for methods of unnamed classes.
For this simple example

// foo.h
struct Foo {
  struct {
    void f^oo() {}
  } Bar;
};

the refactoring generates code:

// foo.cpp
void Foo::(unnamed struct at D:\test\foo.h:2:3)foo() {}

Outplace definition for methods of unnamed classes is meaningless. The patch disables it.

[kadircet]

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

395

could you move this comment closer to the isTempated if statement below?

401

not just classes, but also for cases with unnamed namespaces.

could you change this to look like:

for(DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
  if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
     if(ND->getDeclName().isEmpty())
        return false;
  }
}

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

97

can you also have a test inside an unnamed namespace?

[denis-fatkulin]

Review fixes

[denis-fatkulin]

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

395

Fixed

401

It's a useful remark. Tanks!

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

97

Test case added.

[kadircet]

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

89

nit: i think you can group all these 3 tests.

// Not available if any of the parent context is unnamed, as we can't spell it.
EXPECT_UNAVAILABLE(R"cpp(
struct Foo {
  struct { 
     void b^ar() {}
     struct Baz { void b^ar() {} };
  } Bar;
};
namespace {
  struct Foo { void f^oo() {} };
}
)cpp");

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
 //
 //===----------------------------------------------------------------------===//
 
bool prepare(const Selection &Sel) override {
 
     // Bail out if this is a function template or specialization, as their
     // definitions need to be visible in all including translation units.
     if (Source->getDescribedFunctionTemplate())
       return false;
     if (Source->getTemplateSpecializationInfo())
       return false;
 
     // Bail out in templated classes, as it is hard to spell the class name, i.e
     // if the template parameter is unnamed.
     if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) {
       if (MD->getParent()->isTemplated())
         return false;
+
+      // The refactoring is meaningless for unnamed classes.
+      const auto *Parent = MD->getParent();
+      while (Parent) {
+        if (Parent->getName().empty())
+          return false;
+        Parent = llvm::dyn_cast_or_null<CXXRecordDecl>(Parent->getParent());
+      }
     }
 
     // Note that we don't check whether an implementation file exists or not in
     // the prepare, since performing disk IO on each prepare request might be
     // expensive.
     return true;
   }
 

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.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
 //
 //===----------------------------------------------------------------------===//
 
EXPECT_UNAVAILABLE(R"cpp(
     )cpp");
 
   // Not available on function templates and specializations, as definition must
   // be visible to all translation units.
   EXPECT_UNAVAILABLE(R"cpp(
     template <typename> void fo^o() {};
     template <> void fo^o<int>() {};
   )cpp");
+
+  // Not available on methods of unnamed classes.
+  EXPECT_UNAVAILABLE(R"cpp(
+    struct Foo {
+      struct { void b^ar() {} } Bar;
+    };
+  )cpp");
+
+  // Not available on methods of named classes with unnamed parent in parents
+  // nesting.
+  EXPECT_UNAVAILABLE(R"cpp(
+    struct Foo {
+      struct {
+        struct Bar { void b^ar() {} };
+      } Baz;
+    };
+  )cpp");
 }
 
 TEST_F(DefineOutlineTest, FailsWithoutSource) {
   FileName = "Test.hpp";
   llvm::StringRef Test = "void fo^o() { return; }";
   llvm::StringRef Expected =
       "fail: Couldn't find a suitable implementation file.";
   EXPECT_EQ(apply(Test), Expected);