[clang-tidy] Fix cppcoreguidelines-virtual-base-class-destructor in macros

[steakhal]

Summary

The cppcoreguidelines-virtual-base-class-destructor checker crashed on this example:

#define DECLARE(CLASS) \
class CLASS {          \
protected:             \
  virtual ~CLASS();    \
}
DECLARE(Foo); // no-crash

The checker will hit the following assertion:

clang-tidy: llvm/include/llvm/ADT/Optional.h:196: T &llvm::optional_detail::OptionalStorage<clang::Token, true>::getValue() & [T = clang::Token]: Assertion `hasVal' failed."

It turns out, Lexer::findNextToken() returned llvm::None within the getVirtualKeywordRange() function when the VirtualEndLoc SourceLocation represents a macro expansion.
To prevent this from happening, I decided to propagate the llvm::None further up and only create the removal of virtual if the getVirtualKeywordRange() succeeds.

I considered an alternative fix for this issue:
I could have checked the Destructor.getLocation().isMacroID() before doing any Fixit calculation inside the check() function.
In contrast to this approach my patch will preserve the diagnostics and drop the fixits only if it would have crashed.

[whisperity]

The fix is concise, thank you for observing and untangling the crash!

As the check was originally introduced in the ongoing release, we can go without a release notes entry.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

288

It is strange that there is no fix-it here even though the keyword appears as a single token ...[1]

288

Maybe a FIXME could be added for this case, just to register that we indeed realised something is strange here, but I'm not convinced neither for nor against. The purpose of the patch is to get rid of the crash, after all.

299

[1]... whereas here the check generates the fixit which removes the entire macro substitution!

[steakhal]

Add fixme comment for:

class FooBar2 {
protected:
  virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this.
};

[steakhal]

ping

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

288

Definitely deserves a FIXME. Yes.

[whisperity]

I'm not sure if it is a good idea to merge things on a Friday, but I am comfortable with this, let's get the check crash less.

[steakhal]

In D113558#3154967, @whisperity wrote:

I'm not sure if it is a good idea to merge things on a Friday, but I am comfortable with this, let's get the check crash less.

I've run this on a few projects and it did not introduce new crashes. I plan to commit this on Monday.

clang-tools-extra/clang-tidy/cppcoreguidelines/VirtualClassDestructorCheck.cpp

void VirtualClassDestructorCheck::registerMatchers(MatchFinder *Finder) {
   Finder->addMatcher(
       cxxRecordDecl(
           anyOf(has(cxxMethodDecl(isVirtual())), InheritsVirtualMethod),
           unless(hasPublicVirtualOrProtectedNonVirtualDestructor()))
           .bind("ProblematicClassOrStruct"),
       this);
 }
 
-static CharSourceRange
+static Optional<CharSourceRange>
 getVirtualKeywordRange(const CXXDestructorDecl &Destructor,
                        const SourceManager &SM, const LangOptions &LangOpts) {
+  if (Destructor.getLocation().isMacroID())
+    return None;
+
   SourceLocation VirtualBeginLoc = Destructor.getBeginLoc();
   SourceLocation VirtualEndLoc = VirtualBeginLoc.getLocWithOffset(
       Lexer::MeasureTokenLength(VirtualBeginLoc, SM, LangOpts));
 
   /// Range ends with \c StartOfNextToken so that any whitespace after \c
   /// virtual is included.
   SourceLocation StartOfNextToken =
       Lexer::findNextToken(VirtualEndLoc, SM, LangOpts)
void VirtualClassDestructorCheck::check(
   bool ProtectedAndVirtual = false;
   FixItHint Fix;
 
   if (MatchedClassOrStruct->hasUserDeclaredDestructor()) {
     if (Destructor->getAccess() == AccessSpecifier::AS_public) {
       Fix = FixItHint::CreateInsertion(Destructor->getLocation(), "virtual ");
     } else if (Destructor->getAccess() == AccessSpecifier::AS_protected) {
       ProtectedAndVirtual = true;
-      Fix = FixItHint::CreateRemoval(getVirtualKeywordRange(
-          *Destructor, *Result.SourceManager, Result.Context->getLangOpts()));
+      if (const auto MaybeRange =
+              getVirtualKeywordRange(*Destructor, *Result.SourceManager,
+                                     Result.Context->getLangOpts()))
+        Fix = FixItHint::CreateRemoval(*MaybeRange);
     }
   } else {
     Fix = generateUserDeclaredDestructor(*MatchedClassOrStruct,
                                          *Result.SourceManager);
   }
 
   diag(MatchedClassOrStruct->getLocation(),
        "destructor of %0 is %select{public and non-virtual|protected and "

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-virtual-class-destructor.cpp

 struct DerivedFromTemplateNonVirtualBaseStruct2 : T {
   virtual void foo();
 };
 
 using DerivedFromTemplateNonVirtualBaseStruct2Typedef = DerivedFromTemplateNonVirtualBaseStruct2<PublicNonVirtualBaseStruct>;
 DerivedFromTemplateNonVirtualBaseStruct2Typedef InstantiationWithPublicNonVirtualBaseStruct2;
 
 } // namespace Bugzilla_51912
+
+namespace macro_tests {
+#define CONCAT(x, y) x##y
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar1' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar1 {
+protected:
+  CONCAT(vir, tual) CONCAT(~Foo, Bar1()); // no-fixit
+};
+
+// CHECK-MESSAGES: :[[@LINE+2]]:7: warning: destructor of 'FooBar2' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+1]]:7: note: make it protected and non-virtual
+class FooBar2 {
+protected:
+  virtual CONCAT(~Foo, Bar2()); // FIXME: We should have a fixit for this.
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'FooBar3' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it protected and non-virtual
+// CHECK-FIXES:      class FooBar3 {
+// CHECK-FIXES-NEXT: protected:
+// CHECK-FIXES-NEXT:   ~FooBar3();
+// CHECK-FIXES-NEXT: };
+class FooBar3 {
+protected:
+  CONCAT(vir, tual) ~FooBar3();
+};
+
+// CHECK-MESSAGES: :[[@LINE+6]]:7: warning: destructor of 'FooBar4' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+5]]:7: note: make it protected and non-virtual
+// CHECK-FIXES:      class FooBar4 {
+// CHECK-FIXES-NEXT: protected:
+// CHECK-FIXES-NEXT:   ~CONCAT(Foo, Bar4());
+// CHECK-FIXES-NEXT: };
+class FooBar4 {
+protected:
+  CONCAT(vir, tual) ~CONCAT(Foo, Bar4());
+};
+
+// CHECK-MESSAGES: :[[@LINE+3]]:7: warning: destructor of 'FooBar5' is protected and virtual [cppcoreguidelines-virtual-class-destructor]
+// CHECK-MESSAGES: :[[@LINE+2]]:7: note: make it protected and non-virtual
+#define XMACRO(COLUMN1, COLUMN2) COLUMN1 COLUMN2
+class FooBar5 {
+protected:
+  XMACRO(CONCAT(vir, tual), ~CONCAT(Foo, Bar5());) // no-crash, no-fixit
+};
+#undef XMACRO
+#undef CONCAT
+} // namespace macro_tests