Make Clang plugin check WeakPtrFactory member order

tools/clang/plugins/FindBadConstructs.cpp

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // This file defines a bunch of recurring problems in the Chromium C++ code.
 //
 // Checks that are implemented:
 // - Constructors/Destructors should not be inlined if they are of a complex
 //   class type.
 // - Missing "virtual" keywords on methods that should be virtual.
 // - Non-annotated overriding virtual methods.
 // - Virtual methods with nonempty implementations in their headers.
 // - Classes that derive from base::RefCounted / base::RefCountedThreadSafe
 //   should have protected or private destructors.
+// - WeakPtrFactory members that refer to their outer class should be the last
+//   member.
 
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/AST.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/CXXInheritance.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/Lex/Lexer.h"
@@ skipping 11 matching lines @@
     "[chromium-style] Overriding method must have \"virtual\" keyword.";
 const char kNoExplicitDtor[] =
     "[chromium-style] Classes that are ref-counted should have explicit "
     "destructors that are declared protected or private.";
 const char kPublicDtor[] =
     "[chromium-style] Classes that are ref-counted should have "
     "destructors that are declared protected or private.";
 const char kProtectedNonVirtualDtor[] =
     "[chromium-style] Classes that are ref-counted and have non-private "
     "destructors should declare their destructor virtual.";
+const char kWeakPtrFactoryOrder[] =
+    "[chromium-style] WeakPtrFactory members which refer to their outer class "
+    "must be the last member in the outer class definition.";
 const char kNoteInheritance[] =
     "[chromium-style] %0 inherits from %1 here";
 const char kNoteImplicitDtor[] =
     "[chromium-style] No explicit destructor for %0 defined";
 const char kNotePublicDtor[] =
     "[chromium-style] Public destructor declared here";
 const char kNoteProtectedNonVirtualDtor[] =
     "[chromium-style] Protected non-virtual destructor declared here";
 
 bool TypeHasNonTrivialDtor(const Type* type) {
   if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl())
     return cxx_r->hasTrivialDestructor();
 
   return false;
 }
 
 // Returns the underlying Type for |type| by expanding typedefs and removing
 // any namespace qualifiers. This is similar to desugaring, except that for
 // ElaboratedTypes, desugar will unwrap too much.
 const Type* UnwrapType(const Type* type) {
   if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
     return UnwrapType(elaborated->getNamedType().getTypePtr());
   if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
     return UnwrapType(typedefed->desugar().getTypePtr());
   return type;
 }
 
+struct FindBadConstructsOptions {
+  FindBadConstructsOptions() : check_base_classes(false),
+                               check_virtuals_in_implementations(true),
+                               check_url_directory(false),
+                               check_weak_ptr_factory_order(false) {
+  }
+  bool check_base_classes;
+  bool check_virtuals_in_implementations;
+  bool check_url_directory;
+  bool check_weak_ptr_factory_order;
+};
+
 // Searches for constructs that we know we don't want in the Chromium code base.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
   FindBadConstructsConsumer(CompilerInstance& instance,
-                            bool check_base_classes,
-                            bool check_virtuals_in_implementations,
-                            bool check_url_directory)
-      : ChromeClassTester(instance, check_url_directory),
-        check_base_classes_(check_base_classes),
-        check_virtuals_in_implementations_(check_virtuals_in_implementations) {
+                            const FindBadConstructsOptions& options)
+      : ChromeClassTester(instance, options.check_url_directory),
+        options_(options) {
     // Register warning/error messages.
     diag_method_requires_override_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kMethodRequiresOverride);
     diag_method_requires_virtual_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kMethodRequiresVirtual);
     diag_no_explicit_dtor_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kNoExplicitDtor);
     diag_public_dtor_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kPublicDtor);
     diag_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kProtectedNonVirtualDtor);
+    diag_weak_ptr_factory_order_ = diagnostic().getCustomDiagID(
+        getErrorLevel(), kWeakPtrFactoryOrder);
 
     // Registers notes to make it easier to interpret warnings.
     diag_note_inheritance_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNoteInheritance);
     diag_note_implicit_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNoteImplicitDtor);
     diag_note_public_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNotePublicDtor);
     diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor);
   }
 
   virtual void CheckChromeClass(SourceLocation record_location,
                                 CXXRecordDecl* record) {
     bool implementation_file = InImplementationFile(record_location);
 
     if (!implementation_file) {
       // Only check for "heavy" constructors/destructors in header files;
       // within implementation files, there is no performance cost.
       CheckCtorDtorWeight(record_location, record);
     }
 
-    if (!implementation_file || check_virtuals_in_implementations_) {
+    if (!implementation_file || options_.check_virtuals_in_implementations) {
       bool warn_on_inline_bodies = !implementation_file;
 
       // Check that all virtual methods are marked accordingly with both
       // virtual and OVERRIDE.
       CheckVirtualMethods(record_location, record, warn_on_inline_bodies);
     }
 
     CheckRefCountedDtors(record_location, record);
+
+    if (options_.check_weak_ptr_factory_order)
+      CheckWeakPtrFactoryMembers(record_location, record);
   }
 
  private:
   // The type of problematic ref-counting pattern that was encountered.
   enum RefcountIssue {
     None,
     ImplicitDestructor,
     PublicDestructor
   };
 
-  bool check_base_classes_;
-  bool check_virtuals_in_implementations_;
+  FindBadConstructsOptions options_;
 
   unsigned diag_method_requires_override_;
   unsigned diag_method_requires_virtual_;
   unsigned diag_no_explicit_dtor_;
   unsigned diag_public_dtor_;
   unsigned diag_protected_non_virtual_dtor_;
+  unsigned diag_weak_ptr_factory_order_;
   unsigned diag_note_inheritance_;
   unsigned diag_note_implicit_dtor_;
   unsigned diag_note_public_dtor_;
   unsigned diag_note_protected_non_virtual_dtor_;
 
   // Prints errors if the constructor/destructor weight is too heavy.
   void CheckCtorDtorWeight(SourceLocation record_location,
                            CXXRecordDecl* record) {
     // We don't handle anonymous structs. If this record doesn't have a
     // name, it's of the form:
@@ skipping 354 matching lines @@
   unsigned DiagnosticForIssue(RefcountIssue issue) {
     switch (issue) {
       case ImplicitDestructor:
         return diag_no_explicit_dtor_;
       case PublicDestructor:
         return diag_public_dtor_;
       case None:
         assert(false && "Do not call DiagnosticForIssue with issue None");
         return 0;
     }
+    assert(false);
+    return 0;
   }
 
   // Check |record| to determine if it has any problematic refcounting
   // issues and, if so, print them as warnings/errors based on the current
   // value of getErrorLevel().
   //
   // If |record| is a C++ class, and if it inherits from one of the Chromium
   // ref-counting classes (base::RefCounted / base::RefCountedThreadSafe),
   // ensure that there are no public destructors in the class hierarchy. This
   // is to guard against accidentally stack-allocating a RefCounted class or
@@ skipping 46 matching lines @@
     //   friend class base::Refcounted<RefCountedInterface>;
     //   virtual ~RefCountedInterface() {}
     // };
     //
     // While RefCountedInterface is "safe", in that its destructor is
     // private, it's possible to do the following "unsafe" code:
     //   scoped_refptr<RefCountedInterface> some_class(
     //       new RefCountedInterface);
     //   // Calls SomeInterface::~SomeInterface(), which is unsafe.
     //   delete static_cast<SomeInterface*>(some_class.get());
-    if (!check_base_classes_)
+    if (!options_.check_base_classes)
       return;
 
     // Find all public destructors. This will record the class hierarchy
     // that leads to the public destructor in |dtor_paths|.
     CXXBasePaths dtor_paths;
     if (!record->lookupInBases(
             &FindBadConstructsConsumer::HasPublicDtorCallback, this,
             dtor_paths)) {
       return;
     }
@@ skipping 13 matching lines @@
         diagnostic().Report(loc, diag_note_implicit_dtor_) << problem_record;
         PrintInheritanceChain(*it);
       } else if (issue == PublicDestructor) {
         diagnostic().Report(record_location, diag_public_dtor_);
         PrintInheritanceChain(refcounted_path.front());
         diagnostic().Report(loc, diag_note_public_dtor_);
         PrintInheritanceChain(*it);
       }
     }
   }
+
+  // Check for any problems with WeakPtrFactory class members. This currently
+  // only checks that any WeakPtrFactory<T> member of T appears as the last
+  // data member in T. We could consider checking for bad uses of
+  // WeakPtrFactory to refer to other data members, but that would require
+  // looking at the initializer list in constructors to see what the factory
+  // points to.
+  // Note, if we later add other unrelated checks of data members, we should
+  // consider collapsing them in to one loop to avoid iterating over the data
+  // members more than once.
+  void CheckWeakPtrFactoryMembers(SourceLocation record_location,
+                                  CXXRecordDecl* record) {
+    // Skip anonymous structs.
+    if (record->getIdentifier() == NULL)
+      return;
+
+    // Iterate through members of the class.
+    RecordDecl::field_iterator iter(record->field_begin()),
+                               the_end(record->field_end());
+    SourceLocation weak_ptr_factory_location;  // Invalid initially.
+    for (; iter != the_end; ++iter) {
+      // If we enter the loop but have already seen a matching WeakPtrFactory,
+      // it means there is at least one member after the factory.
+      if (weak_ptr_factory_location.isValid()) {
+        diagnostic().Report(weak_ptr_factory_location,
+                            diag_weak_ptr_factory_order_);
+      }
+      const TemplateSpecializationType* template_spec_type =
+          iter->getType().getTypePtr()->getAs<TemplateSpecializationType>();
+      if (template_spec_type) {
+        const TemplateDecl* template_decl =
+            template_spec_type->getTemplateName().getAsTemplateDecl();
+        if (template_decl && template_spec_type->getNumArgs() >= 1) {
+          if (template_decl->getNameAsString().compare("WeakPtrFactory") == 0 &&
+              GetNamespace(template_decl) == "base") {
+            const TemplateArgument& arg = template_spec_type->getArg(0);
+            if (arg.getAsType().getTypePtr()->getAsCXXRecordDecl() ==
+                record->getTypeForDecl()->getAsCXXRecordDecl()) {
+              weak_ptr_factory_location = iter->getLocation();
+            }
+          }
+        }
+      }
+    }
+  }
 };
 
+
 class FindBadConstructsAction : public PluginASTAction {
  public:
-  FindBadConstructsAction()
-      : check_base_classes_(false),
-        check_virtuals_in_implementations_(true),
-        check_url_directory_(false) {
+  FindBadConstructsAction() {
   }
 
  protected:
   // Overridden from PluginASTAction:
   virtual ASTConsumer* CreateASTConsumer(CompilerInstance& instance,
                                          llvm::StringRef ref) {
-    return new FindBadConstructsConsumer(
-        instance, check_base_classes_, check_virtuals_in_implementations_,
-        check_url_directory_);
+    return new FindBadConstructsConsumer(instance, options_);
   }
 
   virtual bool ParseArgs(const CompilerInstance& instance,
                          const std::vector<std::string>& args) {
     bool parsed = true;
 
     for (size_t i = 0; i < args.size() && parsed; ++i) {
       if (args[i] == "skip-virtuals-in-implementations") {
         // TODO(rsleevi): Remove this once http://crbug.com/115047 is fixed.
-        check_virtuals_in_implementations_ = false;
+        options_.check_virtuals_in_implementations = false;
       } else if (args[i] == "check-base-classes") {
         // TODO(rsleevi): Remove this once http://crbug.com/123295 is fixed.
-        check_base_classes_ = true;
+        options_.check_base_classes = true;
       } else if (args[i] == "check-url-directory") {
         // TODO(tfarina): Remove this once http://crbug.com/229660 is fixed.
-        check_url_directory_ = true;
+        options_.check_url_directory = true;
+      } else if (args[i] == "check-weak-ptr-factory-order") {
+        // TODO(dmichael): Remove this once http://crbug.com/303818 is fixed.
+        options_.check_weak_ptr_factory_order = true;
       } else {
         parsed = false;
         llvm::errs() << "Unknown clang plugin argument: " << args[i] << "\n";
       }
     }
 
     return parsed;
   }
 
  private:
-  bool check_base_classes_;
-  bool check_virtuals_in_implementations_;
-  bool check_url_directory_;
+  FindBadConstructsOptions options_;
 };
 
 }  // namespace
 
 static FrontendPluginRegistry::Add<FindBadConstructsAction>
 X("find-bad-constructs", "Finds bad C++ constructs");