Bug 1153348 - Add an analysis to prohibit operator bools which aren't marked as either explicit or MOZ_IMPLICIT; r=jrmuizel
☠☠ backed out by e01af01b7015 ☠ ☠
authorEhsan Akhgari <ehsan@mozilla.com>
Fri, 10 Apr 2015 23:05:46 -0400
changeset 240403 02e6a50741a95b36444701bf1ed0c9422d987cec
parent 240402 8b99d282cd87173d18f1718ebf0e12ef1e862773
child 240404 940168142fdb7b64c7baa21ef0daf7fb8074313d
push id28636
push userkwierso@gmail.com
push dateThu, 23 Apr 2015 00:16:12 +0000
treeherdermozilla-central@a5af73b32ac8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1153348
milestone40.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 1153348 - Add an analysis to prohibit operator bools which aren't marked as either explicit or MOZ_IMPLICIT; r=jrmuizel This is the counterpart to the existing analysis to catch constructors which aren't marked as either explicit or MOZ_IMPLICIT.
build/clang-plugin/clang-plugin.cpp
build/clang-plugin/tests/TestExplicitOperatorBool.cpp
build/clang-plugin/tests/moz.build
dom/bindings/CallbackObject.h
dom/html/HTMLMediaElement.h
dom/media/SelfRef.h
dom/plugins/base/nsNPAPIPluginInstance.cpp
dom/plugins/ipc/PluginScriptableObjectUtils.h
hal/linux/udev.h
memory/replace/logalloc/replay/Replay.cpp
mfbt/Atomics.h
netwerk/base/AutoClose.h
netwerk/socket/nsSOCKSIOLayer.cpp
xpcom/build/FileLocation.h
xpcom/glue/nsTArray.h
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -79,62 +79,87 @@ private:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
   class RefCountedInsideLambdaChecker : public MatchFinder::MatchCallback {
   public:
     virtual void run(const MatchFinder::MatchResult &Result);
   };
 
+  class ExplicitOperatorBoolChecker : 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;
   MatchFinder astMatcher;
 };
 
 namespace {
 
-bool isInIgnoredNamespace(const Decl *decl) {
+std::string getDeclarationNamespace(const Decl *decl) {
   const DeclContext *DC = decl->getDeclContext()->getEnclosingNamespaceContext();
   const NamespaceDecl *ND = dyn_cast<NamespaceDecl>(DC);
   if (!ND) {
-    return false;
+    return "";
   }
 
   while (const DeclContext *ParentDC = ND->getParent()) {
     if (!isa<NamespaceDecl>(ParentDC)) {
       break;
     }
     ND = cast<NamespaceDecl>(ParentDC);
   }
 
   const auto& name = ND->getName();
+  return name;
+}
 
-  // namespace std and icu are ignored for now
+bool isInIgnoredNamespaceForImplicitCtor(const Decl *decl) {
+  std::string name = getDeclarationNamespace(decl);
+  if (name == "") {
+    return false;
+  }
+
   return name == "std" ||              // standard C++ lib
          name == "__gnu_cxx" ||        // gnu C++ lib
          name == "boost" ||            // boost
          name == "webrtc" ||           // upstream webrtc
          name == "icu_52" ||           // icu
          name == "google" ||           // protobuf
          name == "google_breakpad" ||  // breakpad
          name == "soundtouch" ||       // libsoundtouch
          name == "stagefright" ||      // libstagefright
          name == "MacFileUtilities" || // MacFileUtilities
          name == "dwarf2reader" ||     // dwarf2reader
          name == "arm_ex_to_module" || // arm_ex_to_module
          name == "testing";            // gtest
 }
 
-bool isIgnoredPath(const Decl *decl) {
+bool isInIgnoredNamespaceForImplicitConversion(const Decl *decl) {
+  std::string name = getDeclarationNamespace(decl);
+  if (name == "") {
+    return false;
+  }
+
+  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) {
@@ -145,19 +170,40 @@ bool isIgnoredPath(const Decl *decl) {
         begin->compare_lower(StringRef("scoped_ptr.h")) == 0 ||
         begin->compare_lower(StringRef("graphite2")) == 0) {
       return true;
     }
   }
   return false;
 }
 
-bool isInterestingDecl(const Decl *decl) {
-  return !isInIgnoredNamespace(decl) &&
-         !isIgnoredPath(decl);
+bool isIgnoredPathForImplicitConversion(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("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 MozChecker : public ASTConsumer, public RecursiveASTVisitor<MozChecker> {
   DiagnosticsEngine &Diag;
   const CompilerInstance &CI;
   DiagnosticsMatcher matcher;
@@ -227,17 +273,17 @@ 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() && isInterestingDecl(d)) {
+    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()) {
@@ -411,16 +457,26 @@ bool isClassRefCounted(const CXXRecordDe
 
 bool isClassRefCounted(QualType T) {
   while (const ArrayType *arrTy = T->getAsArrayTypeUnsafe())
     T = arrTy->getElementType();
   CXXRecordDecl *clazz = T->getAsCXXRecordDecl();
   return clazz ? isClassRefCounted(clazz) : RegularClass;
 }
 
+template<class T>
+bool IsInSystemHeader(const ASTContext &AC, const T &D) {
+  auto &SourceManager = AC.getSourceManager();
+  auto ExpansionLoc = SourceManager.getExpansionLoc(D.getLocStart());
+  if (ExpansionLoc.isInvalid()) {
+    return false;
+  }
+  return SourceManager.isInSystemHeader(ExpansionLoc);
+}
+
 }
 
 namespace clang {
 namespace ast_matchers {
 
 /// This matcher will match any class with the stack class assertion or an
 /// array of such classes.
 AST_MATCHER(QualType, stackClassAggregate) {
@@ -510,22 +566,17 @@ AST_MATCHER(BinaryOperator, binaryEquali
 AST_MATCHER(QualType, isFloat) {
   return Node->isRealFloatingType();
 }
 
 /// This matcher will match locations in system headers.  This is adopted from
 /// isExpansionInSystemHeader in newer clangs, but modified in order to work
 /// with old clangs that we use on infra.
 AST_MATCHER(BinaryOperator, isInSystemHeader) {
-  auto &SourceManager = Finder->getASTContext().getSourceManager();
-  auto ExpansionLoc = SourceManager.getExpansionLoc(Node.getLocStart());
-  if (ExpansionLoc.isInvalid()) {
-    return false;
-  }
-  return SourceManager.isInSystemHeader(ExpansionLoc);
+  return IsInSystemHeader(Finder->getASTContext(), Node);
 }
 
 /// This matcher will match locations in SkScalar.h.  This header contains a
 /// known NaN-testing expression which we would like to whitelist.
 AST_MATCHER(BinaryOperator, isInSkScalarDotH) {
   SourceLocation Loc = Node.getOperatorLoc();
   auto &SourceManager = Finder->getASTContext().getSourceManager();
   SmallString<1024> FileName = SourceManager.getFilename(Loc);
@@ -652,16 +703,23 @@ DiagnosticsMatcher::DiagnosticsMatcher()
                                                                          hasParent(callExpr())).bind("member"))))
       ).bind("node"),
     &noAddRefReleaseOnReturnChecker);
 
   astMatcher.addMatcher(lambdaExpr(
             hasDescendant(declRefExpr(hasType(pointerType(pointee(isRefCounted())))).bind("node"))
         ),
     &refCountedInsideLambdaChecker);
+
+  // Older clang versions such as the ones used on the infra recognize these
+  // conversions as 'operator _Bool', but newer clang versions recognize these
+  // as 'operator bool'.
+  astMatcher.addMatcher(methodDecl(anyOf(hasName("operator bool"),
+                                         hasName("operator _Bool"))).bind("node"),
+    &explicitOperatorBoolChecker);
 }
 
 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(
@@ -864,16 +922,35 @@ void DiagnosticsMatcher::RefCountedInsid
       DiagnosticIDs::Note, "Please consider using a smart pointer");
   const DeclRefExpr *node = Result.Nodes.getNodeAs<DeclRefExpr>("node");
 
   Diag.Report(node->getLocStart(), errorID) << node->getFoundDecl() <<
     node->getType()->getPointeeType();
   Diag.Report(node->getLocStart(), noteID);
 }
 
+void DiagnosticsMatcher::ExplicitOperatorBoolChecker::run(
+    const MatchFinder::MatchResult &Result) {
+  DiagnosticsEngine &Diag = Result.Context->getDiagnostics();
+  unsigned errorID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Error, "bad implicit conversion operator for %0");
+  unsigned noteID = Diag.getDiagnosticIDs()->getCustomDiagID(
+      DiagnosticIDs::Note, "consider adding the explicit keyword to %0");
+  const CXXConversionDecl *method = Result.Nodes.getNodeAs<CXXConversionDecl>("node");
+  const CXXRecordDecl *clazz = method->getParent();
+
+  if (!method->isExplicitSpecified() &&
+      !MozChecker::hasCustomAnnotation(method, "moz_implicit") &&
+      !IsInSystemHeader(method->getASTContext(), *method) &&
+      isInterestingDeclForImplicitConversion(method)) {
+    Diag.Report(method->getLocStart(), errorID) << clazz;
+    Diag.Report(method->getLocStart(), noteID) << "'operator bool'";
+  }
+}
+
 class MozCheckAction : public PluginASTAction {
 public:
   ASTConsumerPtr CreateASTConsumer(CompilerInstance &CI, StringRef fileName) override {
 #if CLANG_VERSION_FULL >= 306
     std::unique_ptr<MozChecker> checker(make_unique<MozChecker>(CI));
 
     std::vector<std::unique_ptr<ASTConsumer>> consumers;
     consumers.push_back(std::move(checker));
new file mode 100644
--- /dev/null
+++ b/build/clang-plugin/tests/TestExplicitOperatorBool.cpp
@@ -0,0 +1,11 @@
+#define MOZ_IMPLICIT __attribute__((annotate("moz_implicit")))
+
+struct Bad {
+  operator bool(); // expected-error {{bad implicit conversion operator for 'Bad'}} expected-note {{consider adding the explicit keyword to 'operator bool'}}
+};
+struct Good {
+  explicit operator bool();
+};
+struct Okay {
+  MOZ_IMPLICIT operator bool();
+};
--- a/build/clang-plugin/tests/moz.build
+++ b/build/clang-plugin/tests/moz.build
@@ -2,16 +2,17 @@
 # vim: set filetype=python:
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 SOURCES += [
     'TestBadImplicitConversionCtor.cpp',
     'TestCustomHeap.cpp',
+    'TestExplicitOperatorBool.cpp',
     'TestGlobalClass.cpp',
     'TestMustOverride.cpp',
     'TestNANTestingExpr.cpp',
     'TestNANTestingExprC.c',
     'TestNoAddRefReleaseOnReturn.cpp',
     'TestNoArithmeticExprInArgument.cpp',
     'TestNonHeapClass.cpp',
     'TestNoRefcountedInsideLambdas.cpp',
--- a/dom/bindings/CallbackObject.h
+++ b/dom/bindings/CallbackObject.h
@@ -300,17 +300,17 @@ public:
   }
 
   nsISupports* GetISupports() const
   {
     return reinterpret_cast<nsISupports*>(mPtrBits & ~XPCOMCallbackFlag);
   }
 
   // Boolean conversion operator so people can use this in boolean tests
-  operator bool() const
+  explicit operator bool() const
   {
     return GetISupports();
   }
 
   // Even if HasWebIDLCallback returns true, GetWebIDLCallback() might still
   // return null.
   bool HasWebIDLCallback() const
   {
--- a/dom/html/HTMLMediaElement.h
+++ b/dom/html/HTMLMediaElement.h
@@ -655,17 +655,17 @@ protected:
     explicit WakeLockBoolWrapper(bool val = false)
       : mValue(val), mCanPlay(true), mOuter(nullptr) {}
 
     ~WakeLockBoolWrapper();
 
     void SetOuter(HTMLMediaElement* outer) { mOuter = outer; }
     void SetCanPlay(bool aCanPlay);
 
-    operator bool() const { return mValue; }
+    MOZ_IMPLICIT operator bool() const { return mValue; }
 
     WakeLockBoolWrapper& operator=(bool val);
 
     bool operator !() const { return !mValue; }
 
     static void TimerCallback(nsITimer* aTimer, void* aClosure);
 
   private:
--- a/dom/media/SelfRef.h
+++ b/dom/media/SelfRef.h
@@ -30,17 +30,17 @@ public:
   void Drop(T* t)
   {
     if (mHeld) {
       mHeld = false;
       t->Release();
     }
   }
 
-  operator bool() const { return mHeld; }
+  MOZ_IMPLICIT operator bool() const { return mHeld; }
 
   SelfReference(const SelfReference& aOther) = delete;
   void operator=(const SelfReference& aOther) = delete;
 private:
   bool mHeld;
 };
 } // mozilla
 
--- a/dom/plugins/base/nsNPAPIPluginInstance.cpp
+++ b/dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ -1162,17 +1162,17 @@ class MOZ_STACK_CLASS AutoPluginLibraryC
 public:
   explicit AutoPluginLibraryCall(nsNPAPIPluginInstance* aThis)
     : mThis(aThis), mGuard(aThis), mLibrary(nullptr)
   {
     nsNPAPIPlugin* plugin = mThis->GetPlugin();
     if (plugin)
       mLibrary = plugin->GetLibrary();
   }
-  operator bool() { return !!mLibrary; }
+  explicit operator bool() { return !!mLibrary; }
   PluginLibrary* operator->() { return mLibrary; }
 
 private:
   nsNPAPIPluginInstance* mThis;
   PluginDestructionGuard mGuard;
   PluginLibrary* mLibrary;
 };
 
--- a/dom/plugins/ipc/PluginScriptableObjectUtils.h
+++ b/dom/plugins/ipc/PluginScriptableObjectUtils.h
@@ -272,17 +272,17 @@ public:
     mActor->Unprotect();
   }
 
   ActorType* operator->()
   {
     return mActor;
   }
 
-  operator bool()
+  explicit operator bool()
   {
     return !!mActor;
   }
 
 private:
   ActorType* mActor;
 };
 
--- a/hal/linux/udev.h
+++ b/hal/linux/udev.h
@@ -58,17 +58,17 @@ class udev_lib {
       udev_unref(udev);
     }
 
     if (lib) {
       dlclose(lib);
     }
   }
 
-  operator bool() {
+  explicit operator bool() {
     return udev;
   }
 
  private:
   bool LoadSymbols() {
 #define DLSYM(s) \
   do { \
     s = (typeof(s))dlsym(lib, #s); \
--- a/memory/replace/logalloc/replay/Replay.cpp
+++ b/memory/replace/logalloc/replay/Replay.cpp
@@ -171,17 +171,17 @@ public:
   /* Returns whether the two involved buffers have the same content. */
   bool operator ==(Buffer aOther)
   {
     return mLength == aOther.mLength && (mBuf == aOther.mBuf ||
                                          !strncmp(mBuf, aOther.mBuf, mLength));
   }
 
   /* Returns whether the buffer is empty. */
-  operator bool() { return mLength; }
+  explicit operator bool() { return mLength; }
 
   /* Returns the memory location of the buffer. */
   const char* get() { return mBuf; }
 
   /* Returns the memory location of the end of the buffer (technically, the
    * first byte after the buffer). */
   const char* GetEnd() { return mBuf + mLength; }
 
--- a/mfbt/Atomics.h
+++ b/mfbt/Atomics.h
@@ -737,17 +737,17 @@ class Atomic<bool, Order>
 {
   typedef typename detail::AtomicBase<uint32_t, Order> Base;
 
 public:
   MOZ_CONSTEXPR Atomic() : Base() {}
   explicit MOZ_CONSTEXPR Atomic(bool aInit) : Base(aInit) {}
 
   // We provide boolean wrappers for the underlying AtomicBase methods.
-  operator bool() const
+  MOZ_IMPLICIT operator bool() const
   {
     return Base::Intrinsics::load(Base::mValue);
   }
 
   bool operator=(bool aVal)
   {
     return Base::operator=(aVal);
   }
--- a/netwerk/base/AutoClose.h
+++ b/netwerk/base/AutoClose.h
@@ -18,17 +18,17 @@ template <typename T>
 class AutoClose
 {
 public:
   AutoClose() { } 
   ~AutoClose(){
     Close();
   }
 
-  operator bool() const
+  explicit operator bool() const
   {
     return mPtr;
   }
 
   already_AddRefed<T> forget()
   {
     return mPtr.forget();
   }
--- a/netwerk/socket/nsSOCKSIOLayer.cpp
+++ b/netwerk/socket/nsSOCKSIOLayer.cpp
@@ -237,17 +237,17 @@ public:
       return WritePtr<char, MaxLength>(aStr.Data(), aStr.Length());
   }
 
   size_t Written() {
       MOZ_ASSERT(mBuf);
       return mLength;
   }
 
-  operator bool() { return !!mBuf; }
+  explicit operator bool() { return !!mBuf; }
 private:
   template <size_t Size2>
   friend class Buffer;
 
   template <typename T>
   Buffer<Size - sizeof(T)> Write(T& aValue) {
       return WritePtr<T, sizeof(T)>(&aValue, sizeof(T));
   }
--- a/xpcom/build/FileLocation.h
+++ b/xpcom/build/FileLocation.h
@@ -85,19 +85,19 @@ public:
    */
   void GetPath(nsACString& aResult) const { aResult = mPath; }
 
   /**
    * Boolean value corresponding to whether the file location is initialized
    * or not.
    */
 #if defined(MOZILLA_XPCOMRT_API)
-  operator bool() const { return mBaseFile; }
+  explicit operator bool() const { return mBaseFile; }
 #else
-  operator bool() const { return mBaseFile || mBaseZip; }
+  explicit operator bool() const { return mBaseFile || mBaseZip; }
 #endif // defined(MOZILLA_XPCOMRT_API)
 
   /**
    * Returns whether another FileLocation points to the same resource
    */
   bool Equals(const FileLocation& aFile) const;
 
   /**
--- a/xpcom/glue/nsTArray.h
+++ b/xpcom/glue/nsTArray.h
@@ -107,17 +107,17 @@ struct TileClient;
 // Public methods returning non-proxy types cannot be called from other
 // nsTArray members.
 //
 struct nsTArrayFallibleResult
 {
   // Note: allows implicit conversions from and to bool
   MOZ_IMPLICIT nsTArrayFallibleResult(bool aResult) : mResult(aResult) {}
 
-  operator bool() { return mResult; }
+  MOZ_IMPLICIT operator bool() { return mResult; }
 
 private:
   bool mResult;
 };
 
 struct nsTArrayInfallibleResult
 {
 };