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

[njames93]

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.

[njames93]

• Extend to add virtual and final support

[njames93]

• Made the replacements macro safe

[njames93]

• Fix assertion when no Attrs are present

[kadircet]

Thanks for working on this!

A few comments on macro handling and coding style. Apart from that mostly needs more testing.

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

216

nit:

auto DelAttr = [&](const Attr* A) { /* do magic */};
if(auto *OA = FD->getAttr<OverrideAttr>())
  DelAttr(OA);
if(auto *FA = ...)

219

can you rather use auto AttrToken = TB.getSpelledTokens(TB.getExpandedTokens(Attr.getRange())); throughout this part.
It can provide you with token range as well.

220

can you add some test cases for this branch ? In theory it should be ok to drop this if full expansion is just the attr, i.e.

#define FNL final
struct A { virtual void foo() FNL {} };

but should possibly fail in :

#define MACRO foo() final
struct A { virtual void MACRO {} };

236

nit: I believe QualifierInsertions needs to be renamed now, maybe something like DeclarationCleanups ?

244

again could you please add tests checking this case?

246

you would rather want to go over spelled tokens, as expandedtokens might not exist in the source code. (it looks like the usage above, not related to this patch, is also broken. No need to fix that one I'll try to prepare a fix, but patches welcome.)

TokBuf.spelledForExpanded(TokBuf.expandedTokens(SpecRange))

247

nit: use early exits, i.e:

if(Tok.kind() != tok::kw_virtual)
  continue;
``

249

same argument as above.

260

you can use Tok.range(SM) instead

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

2113

can you also add a test case for final override/override final

[njames93]

• Refactor and extra test cases

[njames93]

I'm not entirely sure how to get the spelledTokens working in a good macro safe way?

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

220

it's not a great idea refactoring functions with MACRO attributes, we can never know if there are side effects based on different definitions due to things like build configs.

[kadircet]

In D75429#1900709, @njames93 wrote:

I'm not entirely sure how to get the spelledTokens working in a good macro safe way?

I don't really follow this comment, could you elaborate? TB.expandedTokens always refer to a subset of the pre-processed token stream, so they can contain non-spelled tokens therefore clangd should never try to operate on those tokens. For example:

#define FOO(X) int X;
FOO(y);

pre-processed token stream will only contain int y which doesn't exist in the source code at all. TB.spelledForExpanded tries to map those expanded range back to spelling (to FOO(y) for example if you've got the full range int y), which is safe to operate on.
if there's no direct mapping between selected range and source code then it returns None, so you should bail out in such cases.

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

220

well that's definitely one way to go, but while designing this code action we decided user should know better and clangd currently provides this code action in cases involving macros, see DefineOutlineTest.HandleMacros and it would be inconsistent with rest of the behavior if it bailed out in half of the cases and kept working for the rest.

Also this case is even safer than the rest, since it is only trying to drop those qualifiers and not duplicate them in the definition side.

244

sorry, i still don't see any cases with multiple virtual keywords.

[njames93]

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

220

Thanks for explaining that for me, I'll give it a go and see how the test cases are handled

244

Ah I thought you meant a test case just containing virtual which there is, I'll get the second one added.
Should I even support multiple virtual decls, https://godbolt.org/z/kGbF22 clang treats this as a warning, but for gcc its an error.

[kadircet]

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

244

If compiler has an error parsing those, we'll be on the safe side since AST will be broken for that node and we'll most likely bail out long before we reach here.

If compiler is happy with those, and produces just warnings, deleting one vs deleting multiple shouldn't make much difference in the code path(just a matter of breaking or not), but would make action more resilient so I would go for deleting multiple, since it won't have any side effects the user will still see the compiler warning in the header(declaration location).

[njames93]

• Macro handling more in line with rest of clangd

[njames93]

• added virtual virtual

[njames93]

• Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

[kadircet]

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

231

let's use syntax::Token::range(SM, AttrTokens->front(), AttrTokens->back())... instead

253

what about setting Any to true below this check and just breaking here instead? e.g.

bool Any = false;
for(...) {
  ....
  if (!Spelling) break;
  Any = true;
}
if (!Any) appendToErrors

261

why assert on this and not delete the whole Spelling that produced the virtual keyword ?

e.g.

#define STUPID_MACRO(X) virtual
struct F {
STUPID_MACRO(BLA BLA) void foo();
}

[kadircet]

In D75429#1901018, @njames93 wrote:

• Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

I believe the formatting linter messages results from the fact that you are finishing the structs with a } on a newline, it should be right next to expected output, without any newlines in between.

[njames93]

In D75429#1901073, @kadircet wrote:

In D75429#1901018, @njames93 wrote:

• Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

I believe the formatting linter messages results from the fact that you are finishing the structs with a } on a newline, it should be right next to expected output, without any newlines in between.

The linter messages are actually to do with the no new line after the { and bad indentation inside the braced init list, however that's how the rest look.

[kadircet]

In D75429#1901120, @njames93 wrote:

In D75429#1901073, @kadircet wrote:

In D75429#1901018, @njames93 wrote:

• Fixed clang tidy warning, wont fix format as it's contradicting with the style of the rest of the function

I believe the formatting linter messages results from the fact that you are finishing the structs with a } on a newline, it should be right next to expected output, without any newlines in between.

The linter messages are actually to do with the no new line after the { and bad indentation inside the braced init list, however that's how the rest look.

yes it just suggests a formatting, but the formatting you want is dropping the new line just before } and hopefully that should also make linter happy.

[njames93]

• Tweaked format

[njames93]

• Addressed comments

[kadircet]

Thanks, LGTM with a few small comments, please address those before landing.

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

244

double negation is bad(this already looks confusing, sounds like we start in an errorful state?) I think we should rather have HadErrors

261

sorry for not explicitly asking in the previous one, could you also add a test for this case?

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

2192

you also need to delete the comma just before the }

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

 #include "HeaderSourceSwitch.h"
 #include "Logger.h"
 #include "ParsedAST.h"
 #include "Path.h"
 #include "Selection.h"
 #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"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
 #include "clang/Basic/SourceLocation.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Basic/TokenKinds.h"
getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
   auto &SM = AST.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;
+  tooling::Replacements DeclarationCleanups;
 
   // Finds the first unqualified name in function return type and name, then
   // qualifies those 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;
     // Only qualify return type and function name.
     if (Ref.NameLoc != FD->getReturnTypeSourceRange().getBegin() &&
         Ref.NameLoc != FD->getLocation())
       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(
         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));
   });
 
   // Get rid of default arguments, since they should not be specified in
   // out-of-line definition.
   for (const auto *PVD : FD->parameters()) {
     if (PVD->hasDefaultArg()) {
       // Deletion range initially spans the initializer, excluding the `=`.
       auto DelRange = CharSourceRange::getTokenRange(PVD->getDefaultArgRange());
       // Get all tokens before the default argument.
       auto Tokens = TokBuf.expandedTokens(PVD->getSourceRange())
                         .take_while([&SM, &DelRange](const syntax::Token &Tok) {
                           return SM.isBeforeInTranslationUnit(
                               Tok.location(), DelRange.getBegin());
                         });
       // Find the last `=` before the default arg.
       auto Tok =
           llvm::find_if(llvm::reverse(Tokens), [](const syntax::Token &Tok) {
             return Tok.kind() == tok::equal;
           });
       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;
+    assert(A->getLocation().isValid());
+    if (A->getLocation().isMacroID()) {
+      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;
+    }
+    if (auto Err = DeclarationCleanups.add(tooling::Replacement(
+            SM, CharSourceRange::getTokenRange(A->getRange()), "")))
+      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 Any = false;
+    // Clang allows duplicating virtual specifiers so check for multiple
+    // occurances.
+    for (const syntax::Token &Tok : TokBuf.expandedTokens(SpecRange)) {
+      if (Tok.kind() != tok::kw_virtual)
+        continue;
+      assert(Tok.location().isValid());
+      if (Tok.location().isMacroID()) {
+        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."));
+        continue;
+      }
+      Any = true;
+      if (auto Err = DeclarationCleanups.add(
+              tooling::Replacement(SM, Tok.range(SM).toCharRange(SM), "")))
+        Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
+    }
+    assert(Any); // Ensure at least one virtual was found
+  }
+
   if (Errors)
     return std::move(Errors);
-  return getFunctionSourceAfterReplacements(FD, QualifierInsertions);
+  return getFunctionSourceAfterReplacements(FD, DeclarationCleanups);
 }
 
 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

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

llvm::StringRef ExpectedSource;
               class Foo {
                 int y = 2;
                 Foo(int z) __attribute__((weak)) ;
                 int bar;
                 int z = 2;
               };)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 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);
     EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)));
   }
 }
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;
TEST_F(DefineOutlineTest, QualifyFunctionName) {
   for (auto &Case : Cases) {
     ExtraFiles["Test.cpp"] = std::string(Case.TestSource);
     EXPECT_EQ(apply(Case.TestHeader, &EditedFiles), Case.ExpectedHeader);
     EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
                                  testPath("Test.cpp"), Case.ExpectedSource)))
         << Case.TestHeader;
   }
 }
+
+TEST_F(DefineOutlineTest, FailsMacroSpecifier) {
+  FileName = "Test.hpp";
+  ExtraFiles["Test.cpp"] = "";
+
+  std::pair<StringRef, StringRef> Cases[] = {
+      {
+          R"cpp(
+          #define VIRT virtual
+          struct A {
+            VIRT void fo^o() {}
+          };)cpp",
+          "fail: define outline: Can't move out of line as function has a "
+          "macro `virtual` specifier."},
+      {
+          R"cpp(
+          #define OVER override
+          struct A {
+            virtual void foo() {}
+          };
+          struct B : A {
+            void fo^o() OVER {}
+          };)cpp",
+          "fail: define outline: Can't move out of line as function has a "
+          "macro `override` specifier."},
+      {
+          R"cpp(
+          #define FINAL final
+          #define OVER override
+          struct A {
+            virtual void foo() {}
+          };
+          struct B : A {
+            void fo^o() FINAL OVER {}
+          };)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(
+          #define FINALOVER final override
+          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