[clang] Warning for non-final classes with final destructors

[logan-5]

Summary

Marking a class' destructor final prevents the class from being inherited from. However, it is a subtle and awkward way to express that at best, and unintended at worst. It may also generate worse code (in other compilers) than marking the class itself final. For these reasons, this revision adds a warning for nonfinal classes with final destructors, with a note to suggest marking the class final to silence the warning.

See https://reviews.llvm.org/D66621 for more background.

Patch by logan-5 (Logan Smith)

[xbolva00]

Looks fine

[• Quuxplusone]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2204

Is there any good reason to spell it dtor in the preceding file but destructor in this one? I think the spelling should be consistent one way or the other. Helps with greppability/maintainability.

clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp

12

Should there be a test for the sealed spelling?

[logan-5]

Addressed some feedback.

[hjyamauchi]

LGTM.

[logan-5]

Thanks. I'll need someone with commit access to help me out with this.

[thakis]

This is a cool warning. Could the note have a fixit attached to it, so that it's easy to mass-clean-up the warning automatically?

[xbolva00]

Done.

rL370737/rL370740

clang/include/clang/Basic/DiagnosticGroups.td

 
 def DeleteIncomplete : DiagGroup<"delete-incomplete">;
 def DeleteNonAbstractNonVirtualDtor : DiagGroup<"delete-non-abstract-non-virtual-dtor">;
 def DeleteAbstractNonVirtualDtor : DiagGroup<"delete-abstract-non-virtual-dtor">;
 def DeleteNonVirtualDtor : DiagGroup<"delete-non-virtual-dtor",
                                      [DeleteNonAbstractNonVirtualDtor,
                                       DeleteAbstractNonVirtualDtor]>;
 def AbstractFinalClass : DiagGroup<"abstract-final-class">;
+def FinalDtorNonFinalClass : DiagGroup<"final-dtor-non-final-class">;
 
 def CXX11CompatDeprecatedWritableStr :
   DiagGroup<"c++11-compat-deprecated-writable-strings">;
 
 def DeprecatedAttributes : DiagGroup<"deprecated-attributes">;
 def DeprecatedCommaSubscript : DiagGroup<"deprecated-comma-subscript">;
 def DeprecatedDeclarations : DiagGroup<"deprecated-declarations">;
 def UnavailableDeclarations : DiagGroup<"unavailable-declarations">;

clang/include/clang/Basic/DiagnosticSemaKinds.td

def warn_destructor_marked_not_override_overriding : Warning <
   InGroup<CXX11WarnOverrideDestructor>, DefaultIgnore;
 def warn_function_marked_not_override_overriding : Warning <
   "%0 overrides a member function but is not marked 'override'">,
   InGroup<CXX11WarnOverrideMethod>;
 def err_class_marked_final_used_as_base : Error<
   "base %0 is marked '%select{final|sealed}1'">;
 def warn_abstract_final_class : Warning<
   "abstract class is marked '%select{final|sealed}0'">, InGroup<AbstractFinalClass>;
+def warn_final_destructor_nonfinal_class : Warning<
+  "class with destructor marked '%select{final|sealed}0' cannot be inherited from">,
+  InGroup<FinalDtorNonFinalClass>;
+def note_final_destructor_nonfinal_class_silence : Note<
+  "mark %0 as '%select{final|sealed}1' to silence this warning">;
 
 // C++11 attributes
 def err_repeat_attribute : Error<"%0 attribute cannot be repeated">;
 
 // C++11 final
 def err_final_function_overridden : Error<
   "declaration of %0 overrides a '%select{final|sealed}1' function">;
 

clang/lib/Sema/SemaDeclCXX.cpp

void Sema::CheckCompletedCXXClass(CXXRecordDecl *Record) {
   if (Record->isAbstract()) {
     if (FinalAttr *FA = Record->getAttr<FinalAttr>()) {
       Diag(Record->getLocation(), diag::warn_abstract_final_class)
         << FA->isSpelledAsSealed();
       DiagnoseAbstractType(Record);
     }
   }
 
+  // Warn if the class has a final destructor but is not itself marked final.
+  if (!Record->hasAttr<FinalAttr>()) {
+    if (CXXDestructorDecl *dtor = Record->getDestructor()) {
+      if (FinalAttr *FA = dtor->getAttr<FinalAttr>()) {
+        Diag(FA->getLocation(), diag::warn_final_destructor_nonfinal_class)
+          << FA->isSpelledAsSealed();
+        Diag(Record->getLocation(), diag::note_final_destructor_nonfinal_class_silence)
+          << Context.getRecordType(Record)
+          << FA->isSpelledAsSealed();
+      }
+    }
+  }
+
   // See if trivial_abi has to be dropped.
   if (Record->hasAttr<TrivialABIAttr>())
     checkIllFormedTrivialABIStruct(*Record);
 
   // Set HasTrivialSpecialMemberForCall if the record has attribute
   // "trivial_abi".
   bool HasTrivialABI = Record->hasAttr<TrivialABIAttr>();
 

clang/test/SemaCXX/warn-final-dtor-nonfinal-class.cpp

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 -verify %s -Wfinal-dtor-non-final-class
+
+class A {
+    ~A();
+};
+
+class B { // expected-note {{mark 'B' as 'final' to silence this warning}}
+    virtual ~B() final; // expected-warning {{class with destructor marked 'final' cannot be inherited from}}
+};
+
+class C final {
+    virtual ~C() final;
+};