Bug 1187486 - Update the clang plugin to detect templated implicit constructors; r=ehsan
authorMichael Layzell <michael@thelayzells.com>
Mon, 27 Jul 2015 09:10:30 -0400
changeset 287230 d88da1f3c511ac78aa777f218900ee38bb118b03
parent 287229 3ece3b3dc2f9d9b68b1952e5a3b245865b5e497e
child 287231 639c7d0805bfd1cd079144284b55d480521288d6
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1187486
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1187486 - Update the clang plugin to detect templated implicit constructors; r=ehsan
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestBadImplicitConversionCtor.cpp
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -106,28 +106,34 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class NonMemMovableChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class ExplicitImplicitChecker : public MatchFinder::MatchCallback {
+  public:
+    virtual void run(const MatchFinder::MatchResult &Result);
+  };
+
   ScopeChecker stackClassChecker;
   ScopeChecker globalClassChecker;
   NonHeapClassChecker nonheapClassChecker;
   ArithmeticArgChecker arithmeticArgChecker;
   TrivialCtorDtorChecker trivialCtorDtorChecker;
   NaNExprChecker nanExprChecker;
   NoAddRefReleaseOnReturnChecker noAddRefReleaseOnReturnChecker;
   RefCountedInsideLambdaChecker refCountedInsideLambdaChecker;
   ExplicitOperatorBoolChecker explicitOperatorBoolChecker;
   NoDuplicateRefCntMemberChecker noDuplicateRefCntMemberChecker;
   NeedsNoVTableTypeChecker needsNoVTableTypeChecker;
   NonMemMovableChecker nonMemMovableChecker;
+  ExplicitImplicitChecker explicitImplicitChecker;
   MatchFinder astMatcher;
 };
 
 namespace {
 
 std::string getDeclarationNamespace(const Decl *decl) {
   const DeclContext *DC = decl->getDeclContext()->getEnclosingNamespaceContext();
   const NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC);
@@ -176,17 +182,16 @@ bool isInIgnoredNamespaceForImplicitConv
 
   return name == "std" ||              // standard C++ lib
          name == "__gnu_cxx" ||        // gnu C++ lib
          name == "google_breakpad" ||  // breakpad
          name == "testing";            // gtest
 }
 
 bool isIgnoredPathForImplicitCtor(const Decl *decl) {
-  decl = decl->getCanonicalDecl();
   SourceLocation Loc = decl->getLocation();
   const SourceManager &SM = decl->getASTContext().getSourceManager();
   SmallString<1024> FileName = SM.getFilename(Loc);
   llvm::sys::fs::make_absolute(FileName);
   llvm::sys::path::reverse_iterator begin = llvm::sys::path::rbegin(FileName),
                                     end   = llvm::sys::path::rend(FileName);
   for (; begin != end; ++begin) {
     if (begin->compare_lower(StringRef("skia")) == 0 ||
@@ -217,21 +222,16 @@ bool isIgnoredPathForImplicitConversion(
   for (; begin != end; ++begin) {
     if (begin->compare_lower(StringRef("graphite2")) == 0) {
       return true;
     }
   }
   return false;
 }
 
-bool isInterestingDeclForImplicitCtor(const Decl *decl) {
-  return !isInIgnoredNamespaceForImplicitCtor(decl) &&
-         !isIgnoredPathForImplicitCtor(decl);
-}
-
 bool isInterestingDeclForImplicitConversion(const Decl *decl) {
   return !isInIgnoredNamespaceForImplicitConversion(decl) &&
          !isIgnoredPathForImplicitConversion(decl);
 }
 
 }
 
 class CustomTypeAnnotation {
@@ -368,44 +368,16 @@ public:
         unsigned overrideNote = Diag.getDiagnosticIDs()->getCustomDiagID(
             DiagnosticIDs::Note, "function to override is here");
         Diag.Report(d->getLocation(), overrideID) << d->getDeclName() <<
           (*it)->getDeclName();
         Diag.Report((*it)->getLocation(), overrideNote);
       }
     }
 
-    if (!d->isAbstract() && isInterestingDeclForImplicitCtor(d)) {
-      for (CXXRecordDecl::ctor_iterator ctor = d->ctor_begin(),
-           e = d->ctor_end(); ctor != e; ++ctor) {
-        // Ignore non-converting ctors
-        if (!ctor->isConvertingConstructor(false)) {
-          continue;
-        }
-        // Ignore copy or move constructors
-        if (ctor->isCopyOrMoveConstructor()) {
-          continue;
-        }
-        // Ignore deleted constructors
-        if (ctor->isDeleted()) {
-          continue;
-        }
-        // Ignore whitelisted constructors
-        if (MozChecker::hasCustomAnnotation(*ctor, "moz_implicit")) {
-          continue;
-        }
-        unsigned ctorID = Diag.getDiagnosticIDs()->getCustomDiagID(
-          DiagnosticIDs::Error, "bad implicit conversion constructor for %0");
-        unsigned noteID = Diag.getDiagnosticIDs()->getCustomDiagID(
-          DiagnosticIDs::Note, "consider adding the explicit keyword to the constructor");
-        Diag.Report(ctor->getLocation(), ctorID) << d->getDeclName();
-        Diag.Report(ctor->getLocation(), noteID);
-      }
-    }
-
     return true;
   }
 
   bool VisitSwitchCase(SwitchCase* stmt) {
     HandleUnusedExprResult(stmt->getSubStmt());
     return true;
   }
   bool VisitCompoundStmt(CompoundStmt* stmt) {
@@ -807,16 +779,39 @@ AST_MATCHER(QualType, isNonMemMovable) {
   return isClassNonMemMovable(Node);
 }
 
 /// This matcher will select classes which require a memmovable template arg
 AST_MATCHER(CXXRecordDecl, needsMemMovable) {
   return MozChecker::hasCustomAnnotation(&Node, "moz_needs_memmovable_type");
 }
 
+AST_MATCHER(CXXConstructorDecl, isInterestingImplicitCtor) {
+  const CXXConstructorDecl *decl = Node.getCanonicalDecl();
+  return
+    // Skip ignored namespaces and paths
+    !isInIgnoredNamespaceForImplicitCtor(decl) &&
+    !isIgnoredPathForImplicitCtor(decl) &&
+    // We only want Converting constructors
+    decl->isConvertingConstructor(false) &&
+    // We don't want copy of move constructors, as those are allowed to be implicit
+    !decl->isCopyOrMoveConstructor() &&
+    // We don't want deleted constructors.
+    !decl->isDeleted();
+}
+
+// We can't call this "isImplicit" since it clashes with an existing matcher in clang.
+AST_MATCHER(CXXConstructorDecl, isMarkedImplicit) {
+  return MozChecker::hasCustomAnnotation(&Node, "moz_implicit");
+}
+
+AST_MATCHER(CXXRecordDecl, isConcreteClass) {
+  return !Node.isAbstract();
+}
+
 }
 }
 
 namespace {
 
 void CustomTypeAnnotation::dumpAnnotationReason(DiagnosticsEngine &Diag, QualType T, SourceLocation Loc) {
   unsigned InheritsID = Diag.getDiagnosticIDs()->getCustomDiagID(
     DiagnosticIDs::Note, "%1 is a %0 type because it inherits from a %0 type %2");
@@ -1046,16 +1041,22 @@ DiagnosticsMatcher::DiagnosticsMatcher()
      &needsNoVTableTypeChecker);
 
   // Handle non-mem-movable template specializations
   astMatcher.addMatcher(classTemplateSpecializationDecl(
       allOf(needsMemMovable(),
             hasAnyTemplateArgument(refersToType(isNonMemMovable())))
       ).bind("specialization"),
       &nonMemMovableChecker);
+
+  astMatcher.addMatcher(
+      constructorDecl(isInterestingImplicitCtor(),
+                      ofClass(allOf(isConcreteClass(), decl().bind("class"))),
+                      unless(isMarkedImplicit())).bind("ctor"),
+      &explicitImplicitChecker);
 }
 
 void DiagnosticsMatcher::ScopeChecker::run(
     const MatchFinder::MatchResult &Result) {
   DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
   unsigned stackID = Diag.getDiagnosticIDs()->getCustomDiagID(
     DiagnosticIDs::Error, "variable of type %0 only valid on the stack");
   unsigned globalID = Diag.getDiagnosticIDs()->getCustomDiagID(
@@ -1320,16 +1321,33 @@ void DiagnosticsMatcher::NonMemMovableCh
       Diag.Report(requestLoc, note1ID)
         << specialization;
       Diag.Report(reason->getLocation(), note2ID)
         << argType << reason;
     }
   }
 }
 
+void DiagnosticsMatcher::ExplicitImplicitChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned ErrorID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error, "bad implicit conversion constructor for %0");
+  unsigned NoteID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note, "consider adding the explicit keyword to the constructor");
+
+  // We've already checked everything in the matcher, so we just have to report the error.
+
+  const CXXConstructorDecl *Ctor = Result.Nodes.getNodeAs<CXXConstructorDecl>("ctor");
+  const CXXRecordDecl *Decl = Result.Nodes.getNodeAs<CXXRecordDecl>("class");
+
+  Diag.Report(Ctor->getLocation(), ErrorID) << Decl->getDeclName();
+  Diag.Report(Ctor->getLocation(), NoteID);
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI, StringRef fileName) override {
 #if CLANG_VERSION_FULL >= 306
     std::unique_ptr<MozChecker> checker(llvm::make_unique<MozChecker>(CI));
     ASTConsumerPtr other(checker->getOtherConsumer());
 
     std::vector<ASTConsumerPtr> consumers;
--- a/build/clang-plugin/tests/TestBadImplicitConversionCtor.cpp
+++ b/build/clang-plugin/tests/TestBadImplicitConversionCtor.cpp
@@ -1,14 +1,16 @@
 #define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
 
 struct Foo {
   Foo(int); // expected-error {{bad implicit conversion constructor for 'Foo'}} expected-note {{consider adding the explicit keyword to the constructor}}
   Foo(int, char=0); // expected-error {{bad implicit conversion constructor for 'Foo'}} expected-note {{consider adding the explicit keyword to the constructor}}
   Foo(...); // expected-error {{bad implicit conversion constructor for 'Foo'}} expected-note {{consider adding the explicit keyword to the constructor}}
+  template<class T>
+  Foo(float); // expected-error {{bad implicit conversion constructor for 'Foo'}} expected-note {{consider adding the explicit keyword to the constructor}}
   Foo(int, unsigned);
   Foo(Foo&);
   Foo(const Foo&);
   Foo(volatile Foo&);
   Foo(const volatile Foo&);
   Foo(Foo&&);
   Foo(const Foo&&);
   Foo(volatile Foo&&);
@@ -34,8 +36,15 @@ struct Barn {
 };
 
 struct Abstract {
   Abstract(int);
   Abstract(int, char=0);
   Abstract(...);
   virtual void f() = 0;
 };
+
+template<class T>
+struct Template {
+  Template(int); // expected-error {{bad implicit conversion constructor for 'Template'}} expected-note {{consider adding the explicit keyword to the constructor}}
+  template<class U>
+  Template(float); // expected-error {{bad implicit conversion constructor for 'Template'}} expected-note {{consider adding the explicit keyword to the constructor}}
+};