[analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.

[NoQ]

Summary

OSNumber and CFNumberRef, much like NSNumber*, can also be accidentally mistaken for a numeric value, so it is worth it to support this type in our new NumberObjectConversion checker.

[zaks.anna]

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

72 ↗

(On Diff #75011)

I'd keep the names more specific. By reading this code alone it looks like you are just checking for any ObjC object.

111 ↗

(On Diff #75011)

Why do we need a case split here? Would calling getCanonicalType().getUnqualifiedType() result in a wrong result for ObjC?

129 ↗

(On Diff #75011)

Let's just recommend nullptr for C++, but allow both NULL and nullptr.

152 ↗

(On Diff #75011)

Please, use more a expressive name. I'd prefer a super long name if it's more expressive.

test/Analysis/number-object-conversion.c

14 ↗

(On Diff #75011)

How about:
"Converting 'CFNumberRef' pointer to a plain boolean value; instead, compare the pointer to NULL or compare to the encapsulated scalar value"

• I've added "pointer".
• I would remove "for branching". Does it add anything?
• Also, we should remove "please" as it makes the warning text longer.

[NoQ]

Ouch, i think i forgot about OSNumber, including tests.

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

111 ↗

(On Diff #75011)

It'd result in a wrong result for CFNumberRef, which is a typedef we shouldn't remove (turning this into struct __CFNumber * would be ugly).

I'd probably rather avoid the case split by removing the .getCanonicalType() part, because rarely anybody makes typedefs for NSNumber* or OSBoolean* etc (such as in tests with the word "sugared").

Or i could descend down to the necessary typedef for C objects, discarding all user's typedefs but not library typedefs. But that'd be even more complicated and probably not very useful for such minor matter.

[alexander-shaposhnikov]

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

149 ↗

(On Diff #75011)

probably it would make sense to move "MatchFinder F;" to the line 276 (closer to the place where it's actually being used)(or maybe i'm missing smth ?)

[zaks.anna]

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

111 ↗

(On Diff #75011)

It'd result in a wrong result for CFNumberRef, which is a typedef we shouldn't
remove (turning this into struct __CFNumber * would be ugly).

I see. Displaying "CFNumberRef" is definitely the right thing to do! Please, add a comment to explain this.

[NoQ]

Address review comments. Add the forgotten tests.

[NoQ]

lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

149 ↗

(On Diff #75011)

Yep, great idea.

test/Analysis/number-object-conversion.c

14 ↗

(On Diff #75011)

I would remove "for branching". Does it add anything?

Because there's otherwise no obvious boolean value around, i wanted to point out what exactly is going on.

or compare to the encapsulated scalar value

They don't necessarily compare the value. Maybe "take"?

I've added "pointer".

Not sure it's worth keeping for other objects ("'NSNumber *' pointer" sounds like a pointer to a pointer).

[zaks.anna]

test/Analysis/number-object-conversion.c

23 ↗

(On Diff #75411)

Comparing a pointer value of type '[CFNumberRef|NSNumber *]' to a primitive [boolean|integer] value; did you mean to compare the result of calling [API Name].

14 ↗

(On Diff #75011)

or compare to the encapsulated scalar value

They don't necessarily compare the value. Maybe "take"?

OSBoolean or CFNumber:
[Comparing|Converting] a pointer value of type '[CFNumberRef|NSNumber *]' to a scalar [boolean|integer] value; instead, either compare the pointer to [NULL|nullptr|nil] or [call [APIName] | compare the result of calling [API Name]].

NSNumber or OSNumber:
Converting a pointer value of type '[NSNumber *]' to a scalar [boolean|integer] value; instead, either compare the pointer to [NULL|nullptr|nil] or call a method on '[NSNumber *]' to get the scalar value.

Comparing a pointer value of type '[NSNumber *]' to a scalar [boolean|integer] value; instead, either compare the pointer to [NULL|nullptr|nil] or call a method on '[NSNumber *]' to get the scalar value.

test/Analysis/number-object-conversion.cpp

32 ↗

(On Diff #75411)

Converting a pointer value of type '[CFNumberRef|NSNumber *]' to a primitive [boolean|integer] value; did you mean to call [APIName].

41 ↗

(On Diff #75411)

Same as cast.

[NoQ]

• Update warning messages. I think it's better to pattern-match for integer sizes after all when we're suggesting API, this especially looks ugly for OSNumber (which is rare).
• Add tests for converting CFNumberRef into intptr_t.
• An unrelated change: Move casts to bools to non-pedantic mode (i mistakenly thought this check is loud).

[NoQ]

Do not suggest any API when we're not sure (was already advised by Anna but i missed it somehow).

[zaks.anna]

Minor nit below.

Thanks for iterating so much on this!
Anna.

test/Analysis/number-object-conversion.cpp

46 ↗

(On Diff #75612)

Minor nit: Here, we want to use scalar instead of primitive so that both parts of the sentence are consistent. Devin did not like "scalar", but that's what it is called in the API docs, so I think should use that word in this specific case. (We can keep 'primitive' for the error messages that do not have "to get the ... value")

Sorry, this is a bit confusing.

[NoQ]

• Fix warning messages, finally, hopefully.
• Make handling of macros much more careful, because errors of form x == Y, where X is an NSNumber pointer, and Y is a custom macro that expands to 0, were found.

[dcoughlin]

LGTM.

test/Analysis/number-object-conversion.m

98 ↗

(On Diff #76043)

This is great! And a good catch.

cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp

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);
+  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();
 
   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?
+    // Unless the macro indicates that the intended type is clearly not
+    // a pointer type, we should avoid warning on comparing pointers
+    // to zero literals in non-pedantic mode.
+    // FIXME: Introduce an AST matcher to implement the macro-related logic?
+    bool MacroIndicatesWeShouldSkipTheCheck = false;
     SourceLocation Loc = CheckIfNull->getLocStart();
     if (Loc.isMacroID()) {
       StringRef MacroName = Lexer::getImmediateMacroName(
           Loc, ACtx.getSourceManager(), ACtx.getLangOpts());
-      if (MacroName != "YES" && MacroName != "NO")
+      if (MacroName == "NULL" || MacroName == "nil")
         return;
-    } else {
-      // Otherwise, comparison of pointers to 0 might still be intentional.
-      // See if this is the case.
+      if (MacroName == "YES" || MacroName == "NO")
+        MacroIndicatesWeShouldSkipTheCheck = true;
+    }
+    if (!MacroIndicatesWeShouldSkipTheCheck) {
       llvm::APSInt Result;
       if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt(
               Result, ACtx, Expr::SE_AllowSideEffects)) {
         if (Result == 0) {
           if (!C->Pedantic)
             return;
           IsPedanticMatch = true;
         }
       }
     }
   }
 
+  const Stmt *Conv = Result.Nodes.getNodeAs<Stmt>("conv");
+  assert(Conv);
+
+  const Expr *ConvertedCObject = Result.Nodes.getNodeAs<Expr>("c_object");
+  const Expr *ConvertedCppObject = Result.Nodes.getNodeAs<Expr>("cpp_object");
+  const Expr *ConvertedObjCObject = Result.Nodes.getNodeAs<Expr>("objc_object");
+  bool IsCpp = (ConvertedCppObject != nullptr);
+  bool IsObjC = (ConvertedObjCObject != nullptr);
+  const Expr *Obj = IsObjC ? ConvertedObjCObject
+                  : IsCpp ? ConvertedCppObject
+                  : ConvertedCObject;
+  assert(Obj);
+
+  bool IsComparison =
+      (Result.Nodes.getNodeAs<Stmt>("comparison") != nullptr);
+
+  bool IsOSNumber =
+      (Result.Nodes.getNodeAs<Decl>("osnumber") != nullptr);
+
+  bool IsInteger =
+      (Result.Nodes.getNodeAs<QualType>("int_type") != nullptr);
+  bool IsObjCBool =
+      (Result.Nodes.getNodeAs<QualType>("objc_bool_type") != nullptr);
+  bool IsCppBool =
+      (Result.Nodes.getNodeAs<QualType>("cpp_bool_type") != nullptr);
+
   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) {
-    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";
+  // Remove ObjC ARC qualifiers.
+  QualType ObjT = Obj->getType().getUnqualifiedType();
+
+  // Remove consts from pointers.
+  if (IsCpp) {
+    assert(ObjT.getCanonicalType()->isPointerType());
+    ObjT = ACtx.getPointerType(
+        ObjT->getPointeeType().getCanonicalType().getUnqualifiedType());
+
+  if (IsComparison)
+    OS << "Comparing ";
+  else
+    OS << "Converting ";
+
+  OS << "a pointer value of type '" << ObjT.getAsString() << "' to a ";
+
+  std::string EuphemismForPlain = "primitive";
+  std::string SuggestedApi = IsObjC ? (IsInteger ? "" : "-boolValue")
+                           : IsCpp ? (IsOSNumber ? "" : "getValue()")
+                           : "CFNumberGetValue()";
+  if (SuggestedApi.empty()) {
+    // A generic message if we're not sure what API should be called.
+    // FIXME: Pattern-match the integer type to make a better guess?
+    SuggestedApi =
+        "a method on '" + ObjT.getAsString() + "' to get the scalar value";
+    // "scalar" is not quite correct or common, but some documentation uses it
+    // when describing object methods we suggest. For consistency, we use
+    // "scalar" in the whole sentence when we need to use this word in at least
+    // one place, otherwise we use "primitive".
+    EuphemismForPlain = "scalar";
   }
 
+  if (IsInteger)
+    OS << EuphemismForPlain << " integer value";
+  else if (IsObjCBool)
+    OS << EuphemismForPlain << " BOOL value";
+  else if (IsCppBool)
+    OS << EuphemismForPlain << " bool value";
+  else // Branch condition?
+    OS << EuphemismForPlain << " boolean value";
+
+
+  if (IsPedanticMatch)
+    OS << "; instead, either compare the pointer to "
+       << (IsObjC ? "nil" : IsCpp ? "nullptr" : "NULL") << " or ";
+  else
+    OS << "; did you mean to ";
+
+  if (IsComparison)
+    OS << "compare the result of calling " << SuggestedApi;
+  else
+    OS << "call " << SuggestedApi;
+
+  if (!IsPedanticMatch)
+    OS << "?";
+
   BR.EmitBasicReport(
       ADC->getDecl(), C, "Suspicious number object 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));
+  // Currently this matches CoreFoundation opaque pointer typedefs.
+  auto CSuspiciousNumberObjectExprM =
+      expr(ignoringParenImpCasts(
+          expr(hasType(
+              typedefType(hasDeclaration(anyOf(
+                  typedefDecl(hasName("CFNumberRef")),
+                  typedefDecl(hasName("CFBooleanRef")))))))
+          .bind("c_object")));
 
-  auto OSBooleanExprM =
+  // Currently this matches XNU kernel number-object pointers.
+  auto CppSuspiciousNumberObjectExprM =
       expr(ignoringParenImpCasts(
           expr(hasType(hasCanonicalType(
               pointerType(pointee(hasCanonicalType(
                   recordType(hasDeclaration(
-                      cxxRecordDecl(hasName(
-                          "OSBoolean")))))))))).bind("osboolean")));
+                      anyOf(
+                        cxxRecordDecl(hasName("OSBoolean")),
+                        cxxRecordDecl(hasName("OSNumber"))
+                            .bind("osnumber"))))))))))
+          .bind("cpp_object")));
 
-  auto NSNumberExprM =
-      expr(ignoringParenImpCasts(expr(hasType(hasCanonicalType(
+  // Currently this matches NeXTSTEP number objects.
+  auto ObjCSuspiciousNumberObjectExprM =
+      expr(ignoringParenImpCasts(
+          expr(hasType(hasCanonicalType(
               objcObjectPointerType(pointee(
                   qualType(hasCanonicalType(
                       qualType(hasDeclaration(
-                      objcInterfaceDecl(hasName(
-                          "NSNumber"))))))))))).bind("nsnumber")));
-
-  auto SuspiciousExprM =
-      anyOf(OSBooleanExprM, NSNumberExprM);
+                          objcInterfaceDecl(hasName("NSNumber")))))))))))
+          .bind("objc_object")));
 
-  auto AnotherNSNumberExprM =
-      expr(equalsBoundNode("nsnumber"));
+  auto SuspiciousNumberObjectExprM = anyOf(
+      CSuspiciousNumberObjectExprM,
+      CppSuspiciousNumberObjectExprM,
+      ObjCSuspiciousNumberObjectExprM);
+
+  // Useful for predicates like "Unless we've seen the same object elsewhere".
+  auto AnotherSuspiciousNumberObjectExprM =
+      expr(anyOf(
+          equalsBoundNode("c_object"),
+          equalsBoundNode("objc_object"),
+          equalsBoundNode("cpp_object")));
 
   // The .bind here is in order to compose the error message more accurately.
-  auto ObjCBooleanTypeM =
+  auto ObjCSuspiciousScalarBooleanTypeM =
       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 =
+  auto SuspiciousScalarBooleanTypeM =
       qualType(anyOf(qualType(booleanType()).bind("cpp_bool_type"),
-                     ObjCBooleanTypeM));
-
+                     ObjCSuspiciousScalarBooleanTypeM));
 
   // The .bind here is in order to compose the error message more accurately.
-  auto AnyNumberTypeM =
+  // Also avoid intptr_t and uintptr_t because they were specifically created
+  // for storing pointers.
+  auto SuspiciousScalarNumberTypeM =
       qualType(hasCanonicalType(isInteger()),
                unless(typedefType(hasDeclaration(
                    typedefDecl(matchesName("^::u?intptr_t$"))))))
       .bind("int_type");
 
-  auto AnyBooleanOrNumberTypeM =
-      qualType(anyOf(AnyBooleanTypeM, AnyNumberTypeM));
+  auto SuspiciousScalarTypeM =
+      qualType(anyOf(SuspiciousScalarBooleanTypeM,
+                     SuspiciousScalarNumberTypeM));
 
-  auto AnyBooleanOrNumberExprM =
-      expr(ignoringParenImpCasts(expr(hasType(AnyBooleanOrNumberTypeM))));
+  auto SuspiciousScalarExprM =
+      expr(ignoringParenImpCasts(expr(hasType(SuspiciousScalarTypeM))));
 
   auto ConversionThroughAssignmentM =
       binaryOperator(hasOperatorName("="),
-                     hasLHS(AnyBooleanOrNumberExprM),
-                     hasRHS(SuspiciousExprM));
+                     hasLHS(SuspiciousScalarExprM),
+                     hasRHS(SuspiciousNumberObjectExprM));
 
   auto ConversionThroughBranchingM =
-      ifStmt(hasCondition(SuspiciousExprM))
+      ifStmt(hasCondition(SuspiciousNumberObjectExprM))
       .bind("pedantic");
 
   auto ConversionThroughCallM =
-      callExpr(hasAnyArgument(allOf(hasType(AnyBooleanOrNumberTypeM),
-                                    ignoringParenImpCasts(SuspiciousExprM))));
+      callExpr(hasAnyArgument(allOf(hasType(SuspiciousScalarTypeM),
+                                    ignoringParenImpCasts(
+                                        SuspiciousNumberObjectExprM))));
 
   // We bind "check_if_null" 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 ConversionThroughEquivalenceM =
       binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
-                     hasEitherOperand(SuspiciousExprM),
-                     hasEitherOperand(AnyBooleanOrNumberExprM
-                                      .bind("check_if_null")));
+                     hasEitherOperand(SuspiciousNumberObjectExprM),
+                     hasEitherOperand(SuspiciousScalarExprM
+                                      .bind("check_if_null")))
+      .bind("comparison");
 
   auto ConversionThroughComparisonM =
       binaryOperator(anyOf(hasOperatorName(">="), hasOperatorName(">"),
                            hasOperatorName("<="), hasOperatorName("<")),
-                     hasEitherOperand(SuspiciousExprM),
-                     hasEitherOperand(AnyBooleanOrNumberExprM));
+                     hasEitherOperand(SuspiciousNumberObjectExprM),
+                     hasEitherOperand(SuspiciousScalarExprM))
+      .bind("comparison");
 
   auto ConversionThroughConditionalOperatorM =
       conditionalOperator(
-          hasCondition(SuspiciousExprM),
-          unless(hasTrueExpression(hasDescendant(AnotherNSNumberExprM))),
-          unless(hasFalseExpression(hasDescendant(AnotherNSNumberExprM))))
+          hasCondition(SuspiciousNumberObjectExprM),
+          unless(hasTrueExpression(
+              hasDescendant(AnotherSuspiciousNumberObjectExprM))),
+          unless(hasFalseExpression(
+              hasDescendant(AnotherSuspiciousNumberObjectExprM))))
       .bind("pedantic");
 
   auto ConversionThroughExclamationMarkM =
-      unaryOperator(hasOperatorName("!"), has(expr(SuspiciousExprM)))
+      unaryOperator(hasOperatorName("!"),
+                    has(expr(SuspiciousNumberObjectExprM)))
       .bind("pedantic");
 
   auto ConversionThroughExplicitBooleanCastM =
-      explicitCastExpr(hasType(AnyBooleanTypeM),
-                       has(expr(SuspiciousExprM)))
-      .bind("pedantic");
+      explicitCastExpr(hasType(SuspiciousScalarBooleanTypeM),
+                       has(expr(SuspiciousNumberObjectExprM)));
 
   auto ConversionThroughExplicitNumberCastM =
-      explicitCastExpr(hasType(AnyNumberTypeM),
-                       has(expr(SuspiciousExprM)));
+      explicitCastExpr(hasType(SuspiciousScalarNumberTypeM),
+                       has(expr(SuspiciousNumberObjectExprM)));
 
   auto ConversionThroughInitializerM =
       declStmt(hasSingleDecl(
-          varDecl(hasType(AnyBooleanOrNumberTypeM),
-                  hasInitializer(SuspiciousExprM))));
+          varDecl(hasType(SuspiciousScalarTypeM),
+                  hasInitializer(SuspiciousNumberObjectExprM))));
 
   auto FinalM = stmt(anyOf(ConversionThroughAssignmentM,
                            ConversionThroughBranchingM,
                            ConversionThroughCallM,
                            ConversionThroughComparisonM,
                            ConversionThroughConditionalOperatorM,
                            ConversionThroughEquivalenceM,
                            ConversionThroughExclamationMarkM,
                            ConversionThroughExplicitBooleanCastM,
                            ConversionThroughExplicitNumberCastM,
                            ConversionThroughInitializerM)).bind("conv");
 
+  MatchFinder F;
+  Callback CB(this, BR, AM.getAnalysisDeclContext(D));
+
   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);
-  }
-}

cfe/trunk/test/Analysis/number-object-conversion.c

+// RUN: %clang_cc1 -triple i386-apple-darwin10 -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
+// RUN: %clang_cc1 -triple i386-apple-darwin10 -w -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify
+
+#define NULL ((void *)0)
+
+typedef const struct __CFNumber *CFNumberRef;
+
+void takes_int(int);
+
+void bad(CFNumberRef p) {
+#ifdef PEDANTIC
+  if (p) {} // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive boolean value; instead, either compare the pointer to NULL or call CFNumberGetValue()}}
+  if (!p) {} // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive boolean value; instead, either compare the pointer to NULL or call CFNumberGetValue()}}
+  p ? 1 : 2; // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive boolean value; instead, either compare the pointer to NULL or call CFNumberGetValue()}}
+  if (p == 0) {} // expected-warning{{Comparing a pointer value of type 'CFNumberRef' to a primitive integer value; instead, either compare the pointer to NULL or compare the result of calling CFNumberGetValue()}}
+#else
+  if (p) {} // no-warning
+  if (!p) {} // no-warning
+  p ? 1 : 2; // no-warning
+  if (p == 0) {} // no-warning
+#endif
+  if (p > 0) {} // expected-warning{{Comparing a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to compare the result of calling CFNumberGetValue()?}}
+  int x = p; // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to call CFNumberGetValue()?}}
+  x = p; // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to call CFNumberGetValue()?}}
+  takes_int(p); // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to call CFNumberGetValue()?}}
+  takes_int(x); // no-warning
+}
+
+// Conversion of a pointer to an intptr_t is fine.
+typedef long intptr_t;
+typedef unsigned long uintptr_t;
+typedef long fintptr_t; // Fake, for testing the regex.
+void test_intptr_t(CFNumberRef p) {
+  (long)p; // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to call CFNumberGetValue()?}}
+  (intptr_t)p; // no-warning
+  (unsigned long)p; // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to call CFNumberGetValue()?}}
+  (uintptr_t)p; // no-warning
+  (fintptr_t)p; // expected-warning{{Converting a pointer value of type 'CFNumberRef' to a primitive integer value; did you mean to call CFNumberGetValue()?}}
+}
+

cfe/trunk/test/Analysis/number-object-conversion.cpp

 // RUN: %clang_cc1 -triple i386-apple-darwin10 -w -std=c++11 -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
 // RUN: %clang_cc1 -triple i386-apple-darwin10 -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;
 };
 
+class OSNumber {
+public:
+  virtual bool isEqualTo(const OSNumber *);
+  virtual unsigned char unsigned8BitValue() const;
+  virtual unsigned short unsigned16BitValue() const;
+  virtual unsigned int unsigned32BitValue() const;
+  virtual unsigned long long unsigned64BitValue() const;
+};
+
 extern const OSBoolean *const &kOSBooleanFalse;
 extern const OSBoolean *const &kOSBooleanTrue;
 
 void takes_bool(bool);
 
-void bad(const OSBoolean *p) {
+void bad_boolean(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}}
+  if (p) {} // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive boolean value; instead, either compare the pointer to nullptr or call getValue()}}
+  if (!p) {} // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive boolean value; instead, either compare the pointer to nullptr or call getValue()}}
+  p ? 1 : 2; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive boolean value; instead, either compare the pointer to nullptr or call getValue()}}
+#else
+  if (p) {} // no-warning
+  if (!p) {} // no-warning
+  p ? 1 : 2; // no-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}}
-  takes_bool(p); // expected-warning{{Converting 'const class OSBoolean *' to a plain bool value; pointer value is being used instead}}
+  (bool)p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive bool value; did you mean to call getValue()?}}
+  bool x = p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive bool value; did you mean to call getValue()?}}
+  x = p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive bool value; did you mean to call getValue()?}}
+  takes_bool(p); // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive bool value; did you mean to call getValue()?}}
   takes_bool(x); // no-warning
 }
 
+void bad_number(const OSNumber *p) {
+#ifdef PEDANTIC
+  if (p) {} // expected-warning{{Converting a pointer value of type 'class OSNumber *' to a scalar boolean value; instead, either compare the pointer to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
+  if (!p) {} // expected-warning{{Converting a pointer value of type 'class OSNumber *' to a scalar boolean value; instead, either compare the pointer to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
+  p ? 1 : 2; // expected-warning{{Converting a pointer value of type 'class OSNumber *' to a scalar boolean value; instead, either compare the pointer to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
+  if (p == 0) {} // expected-warning{{Comparing a pointer value of type 'class OSNumber *' to a scalar integer value; instead, either compare the pointer to nullptr or compare the result of calling a method on 'class OSNumber *' to get the scalar value}}
+#else
+  if (p) {} // no-warning
+  if (!p) {} // no-warning
+  p ? 1 : 2; // no-warning
+  if (p == 0) {} // no-warning
+#endif
+  (int)p; // expected-warning{{Converting a pointer value of type 'class OSNumber *' to a scalar integer value; did you mean to call a method on 'class OSNumber *' to get the scalar value?}}
+  takes_bool(p); // expected-warning{{Converting a pointer value of type 'class OSNumber *' to a scalar bool value; did you mean to call a method on 'class OSNumber *' to get the scalar value?}}
+}
+
 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}}
+  sugared_bool x = p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive bool value; did you mean to call getValue()?}}
 }
 
 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
 }
 
 // Conversion of a pointer to an intptr_t is fine.
 typedef long intptr_t;
 typedef unsigned long uintptr_t;
 typedef long fintptr_t; // Fake, for testing the regex.
 void test_intptr_t(const OSBoolean *p) {
-  (long)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain integer value; pointer value is being used instead}}
+  (long)p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive integer value; did you mean to call getValue()?}}
   (intptr_t)p; // no-warning
-  (unsigned long)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain integer value; pointer value is being used instead}}
+  (unsigned long)p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive integer value; did you mean to call getValue()?}}
   (uintptr_t)p; // no-warning
-  (fintptr_t)p; // expected-warning{{Converting 'const class OSBoolean *' to a plain integer value; pointer value is being used instead}}
+  (fintptr_t)p; // expected-warning{{Converting a pointer value of type 'class OSBoolean *' to a primitive integer value; did you mean to call getValue()?}}
 }
 
 // Test a different definition of NULL.
 #undef NULL
 #define NULL 0
 void test_non_pointer_NULL(const OSBoolean *p) {
   if (p == NULL) {} // no-warning
 }

cfe/trunk/test/Analysis/number-object-conversion.m

 // RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
 // RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -w -analyze -analyzer-checker=osx.NumberObjectConversion -analyzer-config osx.NumberObjectConversion:Pedantic=true -DPEDANTIC %s -verify
 // RUN: %clang_cc1 -triple i386-apple-darwin10 -fblocks -fobjc-arc -w -analyze -analyzer-checker=osx.NumberObjectConversion %s -verify
 // RUN: %clang_cc1 -triple i386-apple-darwin10 -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 takes_boolean(BOOL);
 void takes_integer(int);
 
 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}}
-  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 > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  if (p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+  if (!p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+  (!p) ? 1 : 2; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+  if (p == 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; instead, either compare the pointer to nil or compare the result of calling a method on 'NSNumber *' to get the scalar value}}
+#else
+  if (p) {} // no-warning
+  if (!p) {} // no-warning
+  (!p) ? 1 : 2; // no-warning
+  if (p == 0) {} // no-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 == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
-  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}}
-  takes_boolean(p); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
-  takes_integer(p); // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  (BOOL)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+  if (p > 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to compare the result of calling a method on 'NSNumber *' to get the scalar value?}}
+  if (p == YES) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+  if (p == NO) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+  BOOL x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+  x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+  x = (p == YES); // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+  if (p == 1) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to compare the result of calling a method on 'NSNumber *' to get the scalar value?}}
+  int y = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to call a method on 'NSNumber *' to get the scalar value?}}
+  y = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to call a method on 'NSNumber *' to get the scalar value?}}
+  takes_boolean(p); // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+  takes_integer(p); // expected-warning{{Converting a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to call a method on 'NSNumber *' to get the scalar value?}}
   takes_boolean(x); // no-warning
   takes_integer(y); // no-warning
 }
 
 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}}
+  p == YES; // expected-warning{{Comparing a pointer value of type 'SugaredNumber' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
 }
 
 @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}}
+  i->ivar == YES; // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+  i->prop == YES; // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+  [i foo] == YES; // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
 }
 
 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
 }
 
 // Conversion of a pointer to an intptr_t is fine.
 typedef long intptr_t;
 typedef unsigned long uintptr_t;
 typedef long fintptr_t; // Fake, for testing the regex.
 void test_intptr_t(NSNumber *p) {
-  (long)p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  (long)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to call a method on 'NSNumber *' to get the scalar value?}}
   (intptr_t)p; // no-warning
-  (unsigned long)p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  (unsigned long)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to call a method on 'NSNumber *' to get the scalar value?}}
   (uintptr_t)p; // no-warning
-  (fintptr_t)p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+  (fintptr_t)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to call a method on 'NSNumber *' to get the scalar value?}}
+}
+
+// Test macro suppressions.
+#define FOO 0
+#define BAR 1
+void test_macro(NSNumber *p) {
+  if (p != BAR) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to compare the result of calling a method on 'NSNumber *' to get the scalar value?}}
+#ifdef PEDANTIC
+  if (p != FOO) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; instead, either compare the pointer to nil or compare the result of calling a method on 'NSNumber *' to get the scalar value}}
+#else
+  if (p != FOO) {} // no-warning
+#endif
+}
+
+#define NULL_INSIDE_MACRO NULL
+void test_NULL_inside_macro(NSNumber *p) {
+#ifdef PEDANTIC
+  if (p == NULL_INSIDE_MACRO) {} // no-warning
+#else
+  if (p == NULL_INSIDE_MACRO) {} // no-warning
+#endif
 }
 
 // Test a different definition of NULL.
 #undef NULL
 #define NULL 0
 void test_non_pointer_NULL(NSNumber *p) {
   if (p == NULL) {} // no-warning
 }