[clangd] DefineOutline won't copy virtual specifiers on methods

Summary:
The define out of line refactor tool previously would copy the `virtual`, `override` and `final` specifier into the out of line method definition.
This results in malformed code as those specifiers aren't allowed outside the class definition.

Reviewers: hokein, kadircet

Reviewed By: kadircet

Subscribers: ilya-biryukov, MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang, #clang-tools-extra

Differential Revision: https://reviews.llvm.org/D75429

(cherry picked from commit b2666cc)

Author: njames93

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

@@ -16,6 +16,7 @@
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Attr.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
@@ -155,7 +156,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
         "define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
-  tooling::Replacements QualifierInsertions;
+  tooling::Replacements DeclarationCleanups;
 
   // Finds the first unqualified name in function return type and name, then
   // qualifies those to be valid in TargetContext.
@@ -180,7 +181,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
     const NamedDecl *ND = Ref.Targets.front();
     const std::string Qualifier = getQualification(
         AST, *TargetContext, SM.getLocForStartOfFile(SM.getMainFileID()), ND);
-    if (auto Err = QualifierInsertions.add(
+    if (auto Err = DeclarationCleanups.add(
             tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
       Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
   });
@@ -205,14 +206,72 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
       assert(Tok != Tokens.rend());
       DelRange.setBegin(Tok->location());
       if (auto Err =
-              QualifierInsertions.add(tooling::Replacement(SM, DelRange, "")))
+              DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
         Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
     }
   }
 
+  auto DelAttr = [&](const Attr *A) {
+    if (!A)
+      return;
+    auto AttrTokens =
+        TokBuf.spelledForExpanded(TokBuf.expandedTokens(A->getRange()));
+    assert(A->getLocation().isValid());
+    if (!AttrTokens || AttrTokens->empty()) {
+      Errors = llvm::joinErrors(
+          std::move(Errors),
+          llvm::createStringError(
+              llvm::inconvertibleErrorCode(),
+              llvm::StringRef("define outline: Can't move out of line as "
+                              "function has a macro `") +
+                  A->getSpelling() + "` specifier."));
+      return;
+    }
+    CharSourceRange DelRange =
+        syntax::Token::range(SM, AttrTokens->front(), AttrTokens->back())
+            .toCharRange(SM);
+    if (auto Err =
+            DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
+      Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+  };
+
+  DelAttr(FD->getAttr<OverrideAttr>());
+  DelAttr(FD->getAttr<FinalAttr>());
+
+  if (FD->isVirtualAsWritten()) {
+    SourceRange SpecRange{FD->getBeginLoc(), FD->getLocation()};
+    bool HasErrors = true;
+
+    // Clang allows duplicating virtual specifiers so check for multiple
+    // occurances.
+    for (const auto &Tok : TokBuf.expandedTokens(SpecRange)) {
+      if (Tok.kind() != tok::kw_virtual)
+        continue;
+      auto Spelling = TokBuf.spelledForExpanded(llvm::makeArrayRef(Tok));
+      if (!Spelling) {
+        HasErrors = true;
+        break;
+      }
+      HasErrors = false;
+      CharSourceRange DelRange =
+          syntax::Token::range(SM, Spelling->front(), Spelling->back())
+              .toCharRange(SM);
+      if (auto Err =
+              DeclarationCleanups.add(tooling::Replacement(SM, DelRange, "")))
+        Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+    }
+    if (HasErrors) {
+      Errors = llvm::joinErrors(
+          std::move(Errors),
+          llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                  "define outline: Can't move out of line as "
+                                  "function has a macro `virtual` specifier."));
+    }
+  }
+
   if (Errors)
     return std::move(Errors);
-  return getFunctionSourceAfterReplacements(FD, QualifierInsertions);
+  return getFunctionSourceAfterReplacements(FD, DeclarationCleanups);
 }
 
 struct InsertionPoint {

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

@@ -2065,6 +2065,80 @@ TEST_F(DefineOutlineTest, ApplyTest) {
               };)cpp",
           "Foo::Foo(int z) __attribute__((weak)) : bar(2){}\n",
       },
+      // Virt specifiers.
+      {
+          R"cpp(
+            struct A {
+              virtual void f^oo() {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              virtual void foo() ;
+            };)cpp",
+          " void A::foo() {}\n",
+      },
+      {
+          R"cpp(
+            struct A {
+              virtual virtual void virtual f^oo() {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              virtual virtual void virtual foo() ;
+            };)cpp",
+          "  void  A::foo() {}\n",
+      },
+      {
+          R"cpp(
+            struct A {
+              virtual void foo() = 0;
+            };
+            struct B : A {
+              void fo^o() override {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              virtual void foo() = 0;
+            };
+            struct B : A {
+              void foo() override ;
+            };)cpp",
+          "void B::foo()  {}\n",
+      },
+      {
+          R"cpp(
+            struct A {
+              virtual void foo() = 0;
+            };
+            struct B : A {
+              void fo^o() final {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              virtual void foo() = 0;
+            };
+            struct B : A {
+              void foo() final ;
+            };)cpp",
+          "void B::foo()  {}\n",
+      },
+      {
+          R"cpp(
+            struct A {
+              virtual void foo() = 0;
+            };
+            struct B : A {
+              void fo^o() final override {}
+            };)cpp",
+          R"cpp(
+            struct A {
+              virtual void foo() = 0;
+            };
+            struct B : A {
+              void foo() final override ;
+            };)cpp",
+          "void B::foo()   {}\n",
+      },
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
@@ -2078,6 +2152,8 @@ TEST_F(DefineOutlineTest, HandleMacros) {
   llvm::StringMap<std::string> EditedFiles;
   ExtraFiles["Test.cpp"] = "";
   FileName = "Test.hpp";
+  ExtraArgs.push_back("-DVIRTUAL=virtual");
+  ExtraArgs.push_back("-DOVER=override");
 
   struct {
     llvm::StringRef Test;
@@ -2115,6 +2191,48 @@ TEST_F(DefineOutlineTest, HandleMacros) {
           #define TARGET foo
           void TARGET();)cpp",
        "void TARGET(){ return; }"},
+      {R"cpp(#define VIRT virtual
+          struct A {
+            VIRT void f^oo() {}
+          };)cpp",
+       R"cpp(#define VIRT virtual
+          struct A {
+            VIRT void foo() ;
+          };)cpp",
+       " void A::foo() {}\n"},
+      {R"cpp(
+          struct A {
+            VIRTUAL void f^oo() {}
+          };)cpp",
+       R"cpp(
+          struct A {
+            VIRTUAL void foo() ;
+          };)cpp",
+       " void A::foo() {}\n"},
+      {R"cpp(
+          struct A {
+            virtual void foo() = 0;
+          };
+          struct B : A {
+            void fo^o() OVER {}
+          };)cpp",
+       R"cpp(
+          struct A {
+            virtual void foo() = 0;
+          };
+          struct B : A {
+            void foo() OVER ;
+          };)cpp",
+       "void B::foo()  {}\n"},
+      {R"cpp(#define STUPID_MACRO(X) virtual
+          struct A {
+            STUPID_MACRO(sizeof sizeof int) void f^oo() {}
+          };)cpp",
+       R"cpp(#define STUPID_MACRO(X) virtual
+          struct A {
+            STUPID_MACRO(sizeof sizeof int) void foo() ;
+          };)cpp",
+       " void A::foo() {}\n"},
   };
   for (const auto &Case : Cases) {
     SCOPED_TRACE(Case.Test);
@@ -2226,6 +2344,49 @@ TEST_F(DefineOutlineTest, QualifyFunctionName) {
         << Case.TestHeader;
   }
 }
+
+TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+  ExtraArgs.push_back("-DFINALOVER=final override");
+
+  std::pair<StringRef, StringRef> Cases[] = {
+      {
+          R"cpp(
+          #define VIRT virtual void
+          struct A {
+            VIRT fo^o() {}
+          };)cpp",
+          "fail: define outline: Can't move out of line as function has a "
+          "macro `virtual` specifier."},
+      {
+          R"cpp(
+          #define OVERFINAL final override
+          struct A {
+            virtual void foo() {}
+          };
+          struct B : A {
+            void fo^o() OVERFINAL {}
+          };)cpp",
+          "fail: define outline: Can't move out of line as function has a "
+          "macro `override` specifier.\ndefine outline: Can't move out of line "
+          "as function has a macro `final` specifier."},
+      {
+          R"cpp(
+          struct A {
+            virtual void foo() {}
+          };
+          struct B : A {
+            void fo^o() FINALOVER {}
+          };)cpp",
+          "fail: define outline: Can't move out of line as function has a "
+          "macro `override` specifier.\ndefine outline: Can't move out of line "
+          "as function has a macro `final` specifier."},
+  };
+  for (const auto &Case : Cases) {
+    EXPECT_EQ(apply(Case.first), Case.second);
+  }
+}
 } // namespace
 } // namespace clangd
 } // namespace clang