[analyzer] A checker for macOS-specific bool- and number-like objects.

[NoQ]

Summary

If you store a boolean value as an Objective-C object NSNumber *X, and want to see if it's true or false, than it's not a good idea to write this check as 'X == YES'. Because X is a pointer, and it is unlikely that it's equal to 1. You perhaps wanted to say [X boolValue] instead. Similarly, instead of X == 3, you'd certainly want to write something like [X intValue] == 3.

Similarly, in XNU/driver code, if you have a C++ object OSBoolean *X, which points to one of the two immutable objects - kOSBooleanTrue or kOSBooleanFalse , it's not a good idea to convert X to a C++ bool directly (eg. bool B = X). You want to compare it to kOSBooleanTrue or call the ->isTrue() method instead.

Bugs of both kinds have been noticed in real-world code, so i'm attempting to make a simple checker for that. It is purely AST-based and moreover uses only ASTMatchers for everything (which makes it the first analyzer checker to use AST matchers). In fact i'm not sure if this checker should be in the analyzer or in clang-tidy; there should be no problem to move it around.

I'm still going to tweak these matchers a bit, as i'm in the middle of gathering statistics.

[dcoughlin]

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp

62 ↗

(On Diff #66157)

• The '[boolValue]' thing here is weird. Maybe use '-boolValue' instead?
• How about "Comparison between 'NSNumber *' and 'BOOL'; use -boolValue instead."
• You probably want to remove lifetime qualifiers from the type with .getUnqualifiedType(). before printing (i.e., don't mention '__strong') in the diagnostic.

80 ↗

(On Diff #66157)

The AST matchers seem pretty convenient!

test/Analysis/bool-conversion.cpp

18 ↗

(On Diff #66157)

It is generally good to include the diagnostic text in the test for the warning. This way we make sure we get the warning we expected.

test/Analysis/bool-conversion.m

2 ↗

(On Diff #66157)

You should add a test invocation here where -fobjc-arc is set as well. This adds a bunch of implicit goop that it would be good to test with.

6 ↗

(On Diff #66157)

These 'const's are not idiomatic. It is probably better to remove them.

10 ↗

(On Diff #66157)

There is a Sema warning for p == YES already, right? ("comparison between pointer and integer ('NSNumber *' and 'int')")

11 ↗

(On Diff #66157)

Should p == NO be on in non-pedantic mode, as well? It also seems to me that you could warn on comparison of *any* ObjCObjectPointerType type to NO. At a minimum it would probably be good to check for comparisons of id to NO.

[zaks.anna]

Let's test it on more real word bugs.

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp

11 ↗

(On Diff #66157)

Should we warn on comparisons of NSNumber to '0'? Comparisons to 'nil' would be considered a proper pointer comparison, while comparisons to literal '0' would be considered a faulty comparison of the numbers.

49 ↗

(On Diff #66157)

"throws" -> "generates"?

[zaks.anna]

Here are more comments. Could you address/answer these and upload the latest patch that compares NSNumber to other numbers?

Thanks!

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp

88 ↗

(On Diff #66157)

Can you test if matching for NSNumber works when they come from ivars, properties, and method returns, works?

117 ↗

(On Diff #66157)

Can we use any binary operator here without any restrictions?

test/Analysis/bool-conversion.cpp

18 ↗

(On Diff #66157)

+1

Are these pedantic because we assume these are comparing the pointer and not the value? Have you checked that this is a common idiom?

Same comment applies to NSNumber. If it is common practice to compare against nil..

21 ↗

(On Diff #66157)

Why is this OK?

test/Analysis/bool-conversion.m

2 ↗

(On Diff #66157)

+ 1!!!
Especially, since we are matching the AST.

10 ↗

(On Diff #66157)

These tests seem to be even more relevant to OSBoolean.

[NoQ]

The checker now checks conversions of NSNumber to integer types, not just boolean.

To do now - a bit more time needed to test various tweaks. The current version of the non-pedantic checker has found a couple of good new true positives, being relatively silent in general. I'm keeping positives of form x == 0 (where x is a number pointer) in non-pedantic checker, even though they have been reporting non-bugs, because bugs of this form have been found, and it seems very desirable to make the code more specific in this particular case.

To do later - consider conversions by passing NSNumber to a function that expects an integer, consider OSNumber and maybe CFNumber(?).

[NoQ]

Ouch, seems inline answers don't automatically post themselves when you update the diff :o

lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp

62 ↗

(On Diff #66157)

Yeah, noticed the lifetime qualifiers. Changed messages significantly.

117 ↗

(On Diff #66157)

Comparisons are handled here. Assignments are handled in a different matcher. Arithmetic operators are either a valid pointer arithmetic (such as + or -) or cause compile errors (such as * or /). Bitwise ops (&, >>, etc.) - perhaps somebody is computing shadow memory. Logical ops - also too commonly intentional (typically: if (p && [p boolValue])). Comma operator - ouch, we should probably warn about the very fact that somebody uses comma operators.

test/Analysis/bool-conversion.cpp

18 ↗

(On Diff #66157)

Are these pedantic because we assume these are comparing the pointer and not the value? Have you checked that this is a common idiom?

I haven't checked if this is more popular or less popular than if (p == nil), but it's clearly too popular and too often correct for a warning enabled by default.

21 ↗

(On Diff #66157)

Will test more closely; i think this gives a lot of intentionals, but a cast to a non-boolean integer is clearly an error that must be reported.

test/Analysis/bool-conversion.m

10 ↗

(On Diff #66157)

There is a Sema warning

Yep, but it is disabled with -w. I think it's a good idea to add -w to the run line in all analyzer tests - it often makes tests easier to read, and in particular we're sure that all warnings in the test are coming from the enabled checkers, not from other sources.

11 ↗

(On Diff #66157)

Whoops, accidental.

[NoQ]

• Rename test files for consistency.
• Suppress false positives on intptr_t.
• Add a triple to the tests to in order to stop worrying about integer types.

[zaks.anna]

include/clang/StaticAnalyzer/Checkers/Checkers.td

475

"number pointers" -> "objects representing numbers"

[NoQ]

• Support conversion though function calls.
• Move "if (x == 0)" to pedantic for now (too loud).

include/clang/StaticAnalyzer/Checkers/Checkers.td

 }
 
 //===----------------------------------------------------------------------===//
 // Mac OS X, Cocoa, and Core Foundation checkers.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = OSX in {
 
+def NumberObjectConversionChecker : Checker<"NumberObjectConversion">,
+  InPackage<OSX>,
+  HelpText<"Check for erroneous conversions of number pointers into numbers">,
+  DescFile<"NumberObjectConversionChecker">;
+
 def MacOSXAPIChecker : Checker<"API">,
   InPackage<OSX>,
   HelpText<"Check for proper uses of various Apple APIs">,
   DescFile<"MacOSXAPIChecker.cpp">;
 
 def MacOSKeychainAPIChecker : Checker<"SecKeychainAPI">,
   InPackage<OSX>,
   HelpText<"Check for proper uses of Secure Keychain APIs">,

lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   MPI-Checker/MPIBugReporter.cpp
   MPI-Checker/MPIChecker.cpp
   MPI-Checker/MPIFunctionClassifier.cpp
   NSAutoreleasePoolChecker.cpp
   NSErrorChecker.cpp
   NoReturnFunctionChecker.cpp
   NonNullParamChecker.cpp
   NullabilityChecker.cpp
+  NumberObjectConversionChecker.cpp
   ObjCAtSyncChecker.cpp
   ObjCContainersASTChecker.cpp
   ObjCContainersChecker.cpp
   ObjCMissingSuperCallChecker.cpp
   ObjCSelfInitChecker.cpp
   ObjCSuperDeallocChecker.cpp
   ObjCUnusedIVarsChecker.cpp
   PaddingChecker.cpp
add_clang_library(clangStaticAnalyzerCheckers
   ValistChecker.cpp
   VirtualCallChecker.cpp
 
   DEPENDS
   ClangSACheckers
 
   LINK_LIBS
   clangAST
+  clangASTMatchers
   clangAnalysis
   clangBasic
   clangLex
   clangStaticAnalyzerCore
   )

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

+//===- NumberObjectConversionChecker.cpp -------------------------*- C++ -*-==//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines NumberObjectConversionChecker, which checks for a
+// particular common mistake when dealing with numbers represented as objects
+// passed around by pointers. Namely, the language allows to reinterpret the
+// pointer as a number directly, often without throwing any warnings,
+// but in most cases the result of such conversion is clearly unexpected,
+// as pointer value, rather than number value represented by the pointee object,
+// becomes the result of such operation.
+//
+// Currently the checker supports the Objective-C NSNumber class,
+// and the OSBoolean class found in macOS low-level code; the latter
+// can only hold boolean values.
+//
+// This checker has an option "Pedantic" (boolean), which enables detection of
+// more conversion patterns (which are most likely more harmless, and therefore
+// are more likely to produce false positives) - disabled by default,
+// enabled with `-analyzer-config osx.NumberObjectConversion:Pedantic=true'.
+//
+// A certain ambigious case that remains on in non-pedantic mode is:
+//   NSNumber *x = ...; if (x == 0) ...
+//
+//===----------------------------------------------------------------------===//
+
+#include "ClangSACheckers.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/AnalysisManager.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/ADT/APSInt.h"
+
+using namespace clang;
+using namespace ento;
+using namespace ast_matchers;
+
+namespace {
+
+class NumberObjectConversionChecker : public Checker<check::ASTCodeBody> {
+public:
+  bool Pedantic;
+
+  void checkASTCodeBody(const Decl *D, AnalysisManager &AM,
+                        BugReporter &BR) const;
+};
+
+class Callback : public MatchFinder::MatchCallback {
+  const NumberObjectConversionChecker *C;
+  BugReporter &BR;
+  AnalysisDeclContext *ADC;
+
+public:
+  Callback(const NumberObjectConversionChecker *C,
+           BugReporter &BR, AnalysisDeclContext *ADC)
+      : C(C), BR(BR), ADC(ADC) {}
+  virtual void run(const MatchFinder::MatchResult &Result);
+};
+} // end of anonymous namespace
+
+void Callback::run(const MatchFinder::MatchResult &Result) {
+  bool IsPedanticMatch = (Result.Nodes.getNodeAs<Stmt>("pedantic") != nullptr);
+  if (IsPedanticMatch && !C->Pedantic)
+    return;
+
+  const Stmt *Conv = Result.Nodes.getNodeAs<Stmt>("conv");
+  assert(Conv);
+  const Expr *Osboolean = Result.Nodes.getNodeAs<Expr>("osboolean");
+  const Expr *Nsnumber = Result.Nodes.getNodeAs<Expr>("nsnumber");
+  bool IsObjC = (bool)Nsnumber;
+  const Expr *Obj = IsObjC ? Nsnumber : Osboolean;
+  assert(Obj);
+
+  ASTContext &ACtx = ADC->getASTContext();
+
+  bool IsProbablyIntentional = false;
+  if (const Expr *CheckIfNull =
+          Result.Nodes.getNodeAs<Expr>("check_if_null")) {
+    // We consider NULL to be a pointer, even if it is defined as a plain 0.
+    // FIXME: Introduce a matcher to implement this logic?
+    SourceLocation Loc = CheckIfNull->getLocStart();
+    if (Loc.isMacroID()) {
+      if (Lexer::getImmediateMacroName(Loc, ACtx.getSourceManager(),
+                                       ACtx.getLangOpts()) == "NULL") {
+        return;
+      }
+    } else {
+      // Otherwise, comparison of pointers to 0 might still be intentional.
+      // See if this is the case.
+      llvm::APSInt Result;
+      if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt(
+              Result, ACtx, Expr::SE_AllowSideEffects)) {
+        if (Result == 0)
+          IsProbablyIntentional = true;
+      }
+    }
+  }
+
+  llvm::SmallString<64> Msg;
+  llvm::raw_svector_ostream OS(Msg);
+  OS << "Converting '"
+     << Obj->getType().getCanonicalType().getUnqualifiedType().getAsString()
+     << "' to a plain ";
+
+  if (Result.Nodes.getNodeAs<QualType>("int_type") != nullptr)
+    OS << "integer value";
+  else if (Result.Nodes.getNodeAs<QualType>("objc_bool_type") != nullptr)
+    OS << "BOOL value";
+  else if (Result.Nodes.getNodeAs<QualType>("cpp_bool_type") != nullptr)
+    OS << "bool value";
+  else
+    OS << "boolean value for branching";
+
+  if (IsPedanticMatch || IsProbablyIntentional) {
+    if (IsObjC) {
+      OS << "; please compare the pointer to nil instead "
+            "to suppress this warning";
+    } else {
+      OS << "; please compare the pointer to NULL or nullptr instead "
+            "to suppress this warning";
+    }
+  } else {
+    OS << "; pointer value is being used instead";
+  }
+
+  BR.EmitBasicReport(
+      ADC->getDecl(), C, "Suspicious boolean conversion", "Logic error",
+      OS.str(),
+      PathDiagnosticLocation::createBegin(Obj, BR.getSourceManager(), ADC),
+      Conv->getSourceRange());
+}
+
+void NumberObjectConversionChecker::checkASTCodeBody(const Decl *D,
+                                                     AnalysisManager &AM,
+                                                     BugReporter &BR) const {
+  MatchFinder F;
+  Callback CB(this, BR, AM.getAnalysisDeclContext(D));
+
+  auto OSBooleanExprM =
+      expr(ignoringParenImpCasts(
+          expr(hasType(hasCanonicalType(
+              pointerType(pointee(hasCanonicalType(
+                  recordType(hasDeclaration(
+                      cxxRecordDecl(hasName(
+                          "OSBoolean")))))))))).bind("osboolean")));
+
+  auto NSNumberExprM =
+      expr(ignoringParenImpCasts(expr(hasType(hasCanonicalType(
+          objcObjectPointerType(pointee(
+              qualType(hasCanonicalType(
+                  qualType(hasDeclaration(
+                      objcInterfaceDecl(hasName(
+                          "NSNumber"))))))))))).bind("nsnumber")));
+
+  auto SuspiciousExprM =
+      anyOf(OSBooleanExprM, NSNumberExprM);
+
+  auto AnotherNSNumberExprM =
+      expr(equalsBoundNode("nsnumber"));
+
+  // The .bind here is in order to compose the error message more accurately.
+  auto ObjCBooleanTypeM =
+      qualType(typedefType(hasDeclaration(
+                   typedefDecl(hasName("BOOL"))))).bind("objc_bool_type");
+
+  // The .bind here is in order to compose the error message more accurately.
+  auto AnyBooleanTypeM =
+      qualType(anyOf(qualType(booleanType()).bind("cpp_bool_type"),
+                     ObjCBooleanTypeM));
+
+
+  // The .bind here is in order to compose the error message more accurately.
+  auto AnyNumberTypeM =
+      qualType(hasCanonicalType(isInteger())).bind("int_type");
+
+  auto AnyBooleanOrNumberTypeM =
+      qualType(anyOf(AnyBooleanTypeM, AnyNumberTypeM));
+
+  auto AnyBooleanOrNumberExprM =
+      expr(ignoringParenImpCasts(expr(hasType(AnyBooleanOrNumberTypeM))));
+
+  auto ConversionThroughAssignmentM =
+      binaryOperator(hasOperatorName("="),
+                     hasLHS(AnyBooleanOrNumberExprM),
+                     hasRHS(SuspiciousExprM));
+
+  auto ConversionThroughBranchingM =
+      ifStmt(hasCondition(SuspiciousExprM))
+      .bind("pedantic");
+
+  // We bind "check_as_intentional" to modify the warning message
+  // in case it was intended to compare a pointer to 0 with a relatively-ok
+  // construct "x == 0" or "x != 0".
+  auto ConversionThroughComparisonM =
+      binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!="),
+                           hasOperatorName(">="), hasOperatorName(">"),
+                           hasOperatorName("<="), hasOperatorName("<")),
+                     hasEitherOperand(NSNumberExprM),
+                     hasEitherOperand(AnyBooleanOrNumberExprM
+                                      .bind("check_if_null")));
+
+  auto ConversionThroughConditionalOperatorM =
+      conditionalOperator(
+          hasCondition(SuspiciousExprM),
+          unless(hasTrueExpression(hasDescendant(AnotherNSNumberExprM))),
+          unless(hasFalseExpression(hasDescendant(AnotherNSNumberExprM))))
+      .bind("pedantic");
+
+  auto ConversionThroughExclamationMarkM =
+      unaryOperator(hasOperatorName("!"), has(expr(SuspiciousExprM)))
+      .bind("pedantic");
+
+  auto ConversionThroughExplicitBooleanCastM =
+      explicitCastExpr(hasType(AnyBooleanTypeM),
+                       has(expr(SuspiciousExprM)))
+      .bind("pedantic");
+
+  auto ConversionThroughExplicitNumberCastM =
+      explicitCastExpr(hasType(AnyNumberTypeM),
+                       has(expr(SuspiciousExprM)));
+
+  auto ConversionThroughInitializerM =
+      declStmt(hasSingleDecl(
+          varDecl(hasType(AnyBooleanOrNumberTypeM),
+                  hasInitializer(SuspiciousExprM))));
+
+  auto FinalM = stmt(anyOf(ConversionThroughAssignmentM,
+                           ConversionThroughBranchingM,
+                           ConversionThroughComparisonM,
+                           ConversionThroughConditionalOperatorM,
+                           ConversionThroughExclamationMarkM,
+                           ConversionThroughExplicitBooleanCastM,
+                           ConversionThroughExplicitNumberCastM,
+                           ConversionThroughInitializerM)).bind("conv");
+
+  F.addMatcher(stmt(forEachDescendant(FinalM)), &CB);
+  F.match(*D->getBody(), AM.getASTContext());
+}
+
+void ento::registerNumberObjectConversionChecker(CheckerManager &Mgr) {
+  const LangOptions &LO = Mgr.getLangOpts();
+  if (LO.CPlusPlus || LO.ObjC2) {
+    NumberObjectConversionChecker *Chk =
+        Mgr.registerChecker<NumberObjectConversionChecker>();
+    Chk->Pedantic =
+        Mgr.getAnalyzerOptions().getBooleanOption("Pedantic", false, Chk);
+  }
+}

test/Analysis/Inputs/system-header-simulator-objc.h

 // Like the compiler, the static analyzer treats some functions differently if
 // they come from a system header -- for example, it is assumed that system
 // functions do not arbitrarily free() their parameters, and that some bugs
 // found in system headers cannot be fixed by the user and should be
 // suppressed.
 #pragma clang system_header
 
 typedef unsigned int UInt32;
 typedef unsigned short UInt16;
 
 typedef signed long CFIndex;
 typedef signed char BOOL;
+#define YES ((BOOL)1)
+#define NO ((BOOL)0)
+
 typedef unsigned long NSUInteger;
 typedef unsigned short unichar;
 typedef UInt16 UniChar;
 
+#define NULL ((void *)0)
+#define nil ((id)0)
+
 enum {
     NSASCIIStringEncoding = 1,
     NSNEXTSTEPStringEncoding = 2,
     NSJapaneseEUCStringEncoding = 3,
     NSUTF8StringEncoding = 4,
     NSISOLatin1StringEncoding = 5,
     NSSymbolStringEncoding = 6,
     NSNonLossyASCIIStringEncoding = 7,

 @protocol NSFastEnumeration  - (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id *)stackbuf count:(NSUInteger)len;
 @end           @class NSString, NSDictionary;
 @interface NSValue : NSObject <NSCopying, NSCoding>
 + (NSValue *)valueWithPointer:(const void *)p;
 - (void)getValue:(void *)value;
 @end
 @interface NSNumber : NSValue  - (char)charValue;
 - (id)initWithInt:(int)value;
+- (BOOL)boolValue;
 @end   @class NSString;
 @interface NSArray : NSObject <NSCopying, NSMutableCopying, NSCoding, NSFastEnumeration>  - (NSUInteger)count;
 @end  @interface NSArray (NSArrayCreation)  + (id)array;
 @end       @interface NSAutoreleasePool : NSObject {
 }
 - (void)drain;
 @end extern NSString * const NSBundleDidLoadNotification;
 typedef double NSTimeInterval;

test/Analysis/NSNumber-conversion.m

+// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
+// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify
+// RUN: %clang_cc1 -fblocks -fobjc-arc -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
+// RUN: %clang_cc1 -fblocks -fobjc-arc -w -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
+
+void bad(NSNumber *p) {
+#ifdef PEDANTIC
+  if (p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
+  if (!p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
+  (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
+  (BOOL)p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; please compare the pointer to nil instead to suppress this warning}}
+#endif
+  if (p == YES) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  if (p == NO) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  BOOL x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  x = (p == YES); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  if (p == 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}}
+  if (p == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  if (p > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}}
+  int y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+}
+
+typedef NSNumber *SugaredNumber;
+void bad_sugared(SugaredNumber p) {
+  p == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+}
+
+@interface I : NSObject {
+@public
+  NSNumber *ivar;
+  NSNumber *prop;
+}
+- (NSNumber *)foo;
+@property(copy) NSNumber *prop;
+@end
+
+@implementation I
+@synthesize prop;
+@end
+
+void bad_ivar(I *i) {
+  i->ivar == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  i->prop == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+  [i foo] == YES; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
+}
+
+void good(NSNumber *p) {
+  if ([p boolValue] == NO) {} // no-warning
+  if ([p boolValue] == YES) {} // no-warning
+  BOOL x = [p boolValue]; // no-warning
+}
+
+void suppression(NSNumber *p) {
+  if (p == NULL) {} // no-warning
+  if (p == nil) {} // no-warning
+}
+
+// Test a different definition of NULL.
+#undef NULL
+#define NULL 0
+void test_non_pointer_NULL(NSNumber *p) {
+  if (p == NULL) {} // no-warning
+}

test/Analysis/OSBoolean-conversion.cpp

+// RUN: %clang_cc1 -w -std=c++11 -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
+// RUN: %clang_cc1 -w -std=c++11 -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify
+
+#define NULL ((void *)0)
+#include "Inputs/system-header-simulator-cxx.h" // for nullptr
+
+class OSBoolean {
+public:
+  virtual bool isTrue() const;
+  virtual bool isFalse() const;
+};
+
+extern const OSBoolean *const &kOSBooleanFalse;
+extern const OSBoolean *const &kOSBooleanTrue;
+
+void bad(const OSBoolean *p) {
+#ifdef PEDANTIC
+  if (p) {} // expected-warning{{Converting 'const class OSBoolean *' to a plain boolean value for branching; please compare the pointer to NULL or nullptr instead to suppress this warning}}
+  if (!p) {} // expected-warning{{Converting 'const class OSBoolean *' to a plain boolean value for branching; please compare the pointer to NULL or nullptr instead to suppress this warning}}
+  p ? 1 : 2; // expected-warning{{Converting 'const class OSBoolean *' to a plain boolean value for branching; please compare the pointer to NULL or nullptr instead to suppress this warning}}
+  (bool)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; please compare the pointer to NULL or nullptr instead to suppress this warning}}
+#endif
+  bool x = p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}}
+  x = p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}}
+}
+
+typedef bool sugared_bool;
+typedef const OSBoolean *sugared_OSBoolean;
+void bad_sugared(sugared_OSBoolean p) {
+  sugared_bool x = p; // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}}
+}
+
+void good(const OSBoolean *p) {
+  bool x = p->isTrue(); // no-warning
+  (bool)p->isFalse(); // no-warning
+  if (p == kOSBooleanTrue) {} // no-warning
+}
+
+void suppression(const OSBoolean *p) {
+  if (p == NULL) {} // no-warning
+  bool y = (p == nullptr); // no-warning
+}
+
+// Test a different definition of NULL.
+#undef NULL
+#define NULL 0
+void test_non_pointer_NULL(const OSBoolean *p) {
+  if (p == NULL) {} // no-warning
+}