Commit 3d21a059 authored by Ehsan Akhgari's avatar Ehsan Akhgari
Browse files

Bug 1153348 - Add an analysis to prohibit operator bools which aren't marked... 

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.
parent 3f4737e4

build/clang-plugin/clang-plugin.cpp

+91 −14
Original line number Diff line number Diff line
@@ -84,6 +84,11 @@ private: 
    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;
@@ -92,16 +97,17 @@ private: 
  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()) {
@@ -112,8 +118,15 @@ bool isInIgnoredNamespace(const Decl *decl) { 
  }



  const auto& name = ND->getName();

  return name;

}



bool isInIgnoredNamespaceForImplicitCtor(const Decl *decl) {

  std::string name = getDeclarationNamespace(decl);

  if (name == "") {

    return false;

  }



  // namespace std and icu are ignored for now

  return name == "std" ||              // standard C++ lib

         name == "__gnu_cxx" ||        // gnu C++ lib

         name == "boost" ||            // boost
@@ -129,7 +142,19 @@ bool isInIgnoredNamespace(const Decl *decl) { 
         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();
@@ -150,9 +175,30 @@ bool isIgnoredPath(const Decl *decl) { 
  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);

}



}
@@ -232,7 +278,7 @@ public: 
      }

    }



    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
@@ -416,6 +462,16 @@ bool isClassRefCounted(QualType T) { 
  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 {
@@ -515,12 +571,7 @@ AST_MATCHER(QualType, isFloat) { 
/// 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
@@ -657,6 +708,13 @@ DiagnosticsMatcher::DiagnosticsMatcher() 
            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(
@@ -869,6 +927,25 @@ void DiagnosticsMatcher::RefCountedInsideLambdaChecker::run( 
  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 {

build/clang-plugin/tests/TestExplicitOperatorBool.cpp

0 → 100644
+11 −0
Original line number Diff line number Diff line 
#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();

};

build/clang-plugin/tests/moz.build

+1 −0
Original line number Diff line number Diff line
@@ -7,6 +7,7 @@ 
SOURCES += [

    'TestBadImplicitConversionCtor.cpp',

    'TestCustomHeap.cpp',

    'TestExplicitOperatorBool.cpp',

    'TestGlobalClass.cpp',

    'TestMustOverride.cpp',

    'TestNANTestingExpr.cpp',

dom/bindings/CallbackObject.h

+1 −1
Original line number Diff line number Diff line
@@ -305,7 +305,7 @@ public: 
  }



  // Boolean conversion operator so people can use this in boolean tests

  operator bool() const

  explicit operator bool() const

  {

    return GetISupports();

  }

dom/html/HTMLMediaElement.h

+1 −1
Original line number Diff line number Diff line
@@ -660,7 +660,7 @@ protected: 
    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);