[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

394–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

394–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

if (!Source || !Source->doesThisDeclarationHaveABody() ||
       return false;
 
     // 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)) {
+      // Bail out in templated classes, as it is hard to spell the class name,
+      // i.e if the template parameter is unnamed.
       if (MD->getParent()->isTemplated())
         return false;
+
+      // The refactoring is meaningless for unnamed classes and definitions
+      // within unnamed namespaces.
+      for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
+        if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
+          if (ND->getDeclName().isEmpty())
+            return false;
+        }
+      }
     }
 
     // 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

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");
+
+  // Not available on definitions within unnamed namespaces
+  EXPECT_UNAVAILABLE(R"cpp(
+    namespace {
+      struct Foo {
+        void f^oo() {}
+      };
+    } // namespace
+  )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);