New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor

[ArnaudBienner]

Summary

Adding you, Nico, as a reviewer, since you worked on a similar warning, and reviewed my recent changes.
Feel free to add/delegate to someone else.

[rjmccall]

lib/Sema/SemaOverload.cpp

13075

Intentionally suppressed in lambdas? I think that might be reasonable, but also worth a comment.

13080

This should be folded into the same checks that lead into that warning, since most of the conditions are identical and it's just a few final decisions (and the purity of the function) that changes whether to emit a diagnostic and which one.

13087

Use cast<> instead of dyn_cast+assert.

13091

ContextRD must be polymorphic, it provides a virtual method.

13099

As a matter of general principle, these diagnostics should be at TheCall->getExprLoc() instead of MemExpr->getBeginLoc(). As a matter of correctness, the insertion must start at MemExpr->getMemberLoc() instead of MemExpr->getBeginLoc() because the base in the member expression might be explicit.

Please add tests for the fix-its.

[ArnaudBienner]

Thanks @xbolva00 for adding reviewers and @rjmccall for reviewing!

@rjmccall as you might remember, we had a discussion on the mailing list ("Warning when calling virtual functions from constructor/desctructor?") and from what I understand the overall feeling was that this patch/warning won't be accepted until we move forward with your proposal of having a "-Wversion=" flag to allow deactivate new warnings when upgrading clang, but enable them by default otherwise.
Have you made any progress on that? Or do you think the warning can be implemented anyway? (maybe off by default?)

Just want to avoid spending our time reviewing/doing changes on a patch that won't be accepted in the end :)

[rjmccall]

Oh, sorry, I had completely forgotten about that.

My contributions to Clang are primarily code review these days; I am not working on that idea.

include/clang/Basic/DiagnosticGroups.td

def Dangling : DiagGroup<"dangling", [DanglingField,
                                       DanglingInitializerList,
                                       ReturnStackAddress]>;
 def DistributedObjectModifiers : DiagGroup<"distributed-object-modifiers">;
 def ExpansionToDefined : DiagGroup<"expansion-to-defined">;
 def FlagEnum : DiagGroup<"flag-enum">;
 def IncrementBool : DiagGroup<"increment-bool", [DeprecatedIncrementBool]>;
 def InfiniteRecursion : DiagGroup<"infinite-recursion">;
 def PureVirtualCallFromCtorDtor: DiagGroup<"call-to-pure-virtual-from-ctor-dtor">;
+def VirtualCallFromCtorDtor: DiagGroup<"call-to-virtual-from-ctor-dtor">;
 def GNUImaginaryConstant : DiagGroup<"gnu-imaginary-constant">;
 def IgnoredQualifiers : DiagGroup<"ignored-qualifiers">;
 def : DiagGroup<"import">;
 def GNUIncludeNext : DiagGroup<"gnu-include-next">;
 def IncompatibleMSStruct : DiagGroup<"incompatible-ms-struct">;
 def IncompatiblePointerTypesDiscardsQualifiers
   : DiagGroup<"incompatible-pointer-types-discards-qualifiers">;
 def IncompatibleFunctionPointerTypes

include/clang/Basic/DiagnosticSemaKinds.td

def err_member_function_call_bad_type : Error<
   "cannot initialize object parameter of type %0 with an expression "
   "of type %1">;
 
 def warn_call_to_pure_virtual_member_function_from_ctor_dtor : Warning<
   "call to pure virtual member function %0 has undefined behavior; "
   "overrides of %0 in subclasses are not available in the "
   "%select{constructor|destructor}1 of %2">, InGroup<PureVirtualCallFromCtorDtor>;
 
+def warn_call_to_virtual_member_function_from_ctor_dtor : Warning<
+  "call to virtual member function %0 from %select{constructor|destructor}1 of %2 "
+  "will not call overridden implementations">, InGroup<VirtualCallFromCtorDtor>;
+def note_call_to_virtual_member_function_from_ctor_dtor_silence : Note<
+  "qualify the call to silence this warning">;
+
 def select_special_member_kind : TextSubstitution<
   "%select{default constructor|copy constructor|move constructor|"
   "copy assignment operator|move assignment operator|destructor}0">;
 
 def note_member_declared_at : Note<"member is declared here">;
 def note_ivar_decl : Note<"instance variable is declared here">;
 def note_bitfield_decl : Note<"bit-field is declared here">;
 def note_implicit_param_decl : Note<"%0 is an implicit parameter">;

lib/Sema/SemaOverload.cpp

if (isa<CXXThisExpr>(MemExpr->getBase()->IgnoreParenCasts()) &&
 
       Diag(MD->getBeginLoc(), diag::note_previous_decl) << MD->getDeclName();
       if (getLangOpts().AppleKext)
         Diag(MemExpr->getBeginLoc(), diag::note_pure_qualified_call_kext)
             << MD->getParent()->getDeclName() << MD->getDeclName();
     }
   }
 
+  if ((isa<CXXConstructorDecl>(CurContext) ||
+       isa<CXXDestructorDecl>(CurContext)) &&
+      isa<CXXThisExpr>(MemExpr->getBase()->IgnoreParenCasts()) &&
+      TheCall->getMethodDecl()->isVirtual() &&
+      !MemExpr->hasQualifier() &&
+      // We already have a warning for pure virtual functions above, so don't
+      // consider them here
+      !TheCall->getMethodDecl()->isPure()) {
+    const CXXMethodDecl *CallMD = TheCall->getMethodDecl();
+    // dynamic casting CurnContext to CXXMethodDecl here is fine since we know
+    // from the check above that CurnContext is either CXXConstructorDecl or
+    // CXXDestructorDecl, which both inherit from CXXMethodDecl.
+    const CXXMethodDecl *ContextMD = dyn_cast<CXXMethodDecl>(CurContext);
+    assert(ContextMD && "CurContext was expected be a method declaration");
+    const CXXRecordDecl *ContextRD = ContextMD->getParent();
+    // A final class cannot be derived from, so issue a warning only if not
+    // final
+    if (ContextRD->isPolymorphic() && !ContextRD->hasAttr<FinalAttr>()) {
+      Diag(MemExpr->getBeginLoc(),
+           diag::warn_call_to_virtual_member_function_from_ctor_dtor)
+          << CallMD->getDeclName() << isa<CXXDestructorDecl>(CurContext)
+          << ContextMD->getParent()->getDeclName();
+      Diag(MemExpr->getBeginLoc(),
+           diag::note_call_to_virtual_member_function_from_ctor_dtor_silence)
+          << FixItHint::CreateInsertion(
+                 MemExpr->getBeginLoc(),
+                 ContextMD->getParent()->getDeclName().getAsString() + "::");
+    }
+  }
+
   if (CXXDestructorDecl *DD =
           dyn_cast<CXXDestructorDecl>(TheCall->getMethodDecl())) {
     // a->A::f() doesn't go through the vtable, except in AppleKext mode.
     bool CallCanBeVirtual = !MemExpr->hasQualifier() || getLangOpts().AppleKext;
     CheckVirtualDtorCall(DD, MemExpr->getBeginLoc(), /*IsDelete=*/false,
                          CallCanBeVirtual, /*WarnOnNonAbstractTypes=*/true,
                          MemExpr->getMemberLoc());
   }

test/Analysis/ctor.mm

-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DI386 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DI386 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify -DTEST_INLINABLE_ALLOCATORS -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin12 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify -analyzer-config eagerly-assume=false %s
-// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin12 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -std=c++11 -verify -DTEST_INLINABLE_ALLOCATORS -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DI386 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -Wno-call-to-virtual-from-ctor-dtor -std=c++11 -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple i386-apple-darwin10 -DI386 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -Wno-call-to-virtual-from-ctor-dtor -std=c++11 -verify -DTEST_INLINABLE_ALLOCATORS -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin12 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -Wno-call-to-virtual-from-ctor-dtor -std=c++11 -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin12 -analyzer-checker=core,debug.ExprInspection -fobjc-arc -analyzer-config c++-inlining=constructors -Wno-null-dereference -Wno-call-to-virtual-from-ctor-dtor -std=c++11 -verify -DTEST_INLINABLE_ALLOCATORS -analyzer-config eagerly-assume=false %s
 
 #include "Inputs/system-header-simulator-cxx.h"
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);
 
 // A simplified version of std::move.
 template <typename T>

test/Analysis/dtor.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection,cplusplus -analyzer-config c++-inlining=destructors -Wno-null-dereference -Wno-inaccessible-base -verify -analyzer-config eagerly-assume=false %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=core,unix.Malloc,debug.ExprInspection,cplusplus -analyzer-config c++-inlining=destructors -Wno-null-dereference -Wno-inaccessible-base -Wno-call-to-virtual-from-ctor-dtor -verify -analyzer-config eagerly-assume=false %s
 
 void clang_analyzer_eval(bool);
 void clang_analyzer_checkInlined(bool);
 
 class A {
 public:
   ~A() { 
     int *x = 0;

test/Analysis/virtualcall.cpp

-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-output=text -verify -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-output=text -Wno-call-to-virtual-from-ctor-dtor -verify -std=c++11 %s
 
-// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -analyzer-output=text -verify -std=c++11 %s
+// RUN: %clang_analyze_cc1 -analyzer-checker=optin.cplusplus.VirtualCall -analyzer-store region -analyzer-config optin.cplusplus.VirtualCall:PureOnly=true -DPUREONLY=1 -analyzer-output=text -Wno-call-to-virtual-from-ctor-dtor -verify -std=c++11 %s
 
 #include "virtualcall.h"
 
 class A {
 public:
   A();
 
   ~A(){};

test/SemaCXX/warn-virtual-call-from-ctor-dtor.cpp

+// RUN: %clang_cc1 %s -fsyntax-only -verify -Wcall-to-virtual-from-ctor-dtor
+struct A {
+  A() { f(); } // expected-warning {{call to virtual member function 'f' from constructor of 'A' will not call overridden implementations}} expected-note {{qualify the call to silence this warning}}
+  ~A() { f(); } // expected-warning {{call to virtual member function 'f' from destructor of 'A' will not call overridden implementations}} expected-note {{qualify the call to silence this warning}}
+
+  virtual void f() {}
+};
+
+// Warns in classes inheriting from A too
+struct B : A {
+  B() { f(); } // expected-warning {{call to virtual member function 'f' from constructor of 'B' will not call overridden implementations}} expected-note {{qualify the call to silence this warning}}
+  ~B() { f(); } // expected-warning {{call to virtual member function 'f' from destructor of 'B' will not call overridden implementations}} expected-note {{qualify the call to silence this warning}}
+  void f() {}
+};
+
+// Don't warn if the class if final
+struct C final : A {
+  C() { f(); }
+  ~C() { f(); }
+};
+
+// Don't warn (or note) when calling the function on a pointer.
+struct D : A {
+  A *a;
+  D() { a->f(); };
+  ~D() { a->f(); };
+};
+
+// Don't warn if the call is fully qualified.
+struct E {
+  virtual void f() = 0;
+  E() {
+    E::f();
+  }
+};