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

[Artem Dergachev via cfe-commits]

NoQ marked an inline comment as done.
NoQ added a comment.

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


================
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:62
@@ +61,3 @@
+       << "' to a plain boolean value: probably a forgotten "
+       << (IsObjC ? "'[boolValue]'" : "'->isTrue()'");
+    BR.EmitBasicReport(
----------------
dcoughlin wrote:
> - 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.
> 
Yeah, noticed the lifetime qualifiers. Changed messages significantly.

================
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:117
@@ +116,3 @@
+    auto ConversionThroughComparisonM =
+        binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                       hasEitherOperand(NSNumberExprM),
----------------
zaks.anna wrote:
> Can we use any binary operator here without any restrictions?
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.

================
Comment at: test/Analysis/bool-conversion.cpp:18
@@ +17,3 @@
+#ifdef PEDANTIC
+  if (p) {} // expected-warning{{}}
+  if (!p) {} // expected-warning{{}}
----------------
zaks.anna wrote:
> dcoughlin wrote:
> > 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.
> +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..
> 
> 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.

================
Comment at: test/Analysis/bool-conversion.cpp:21
@@ +20,3 @@
+  p ? 1 : 2; // expected-warning{{}}
+  (bool)p; // expected-warning{{}}
+#endif
----------------
zaks.anna wrote:
> Why is this OK?
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.

================
Comment at: test/Analysis/bool-conversion.m:10
@@ +9,3 @@
+  if (!p) {} // expected-warning{{}}
+  if (p == YES) {} // expected-warning{{}}
+  if (p == NO) {} // expected-warning{{}}
----------------
zaks.anna wrote:
> dcoughlin wrote:
> > There is a Sema warning for `p == YES` already, right? ("comparison between pointer and integer ('NSNumber *' and 'int')")
> These tests seem to be even more relevant to OSBoolean.
> 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.

================
Comment at: test/Analysis/bool-conversion.m:11
@@ +10,3 @@
+  if (p == YES) {} // expected-warning{{}}
+  if (p == NO) {} // expected-warning{{}}
+  (!p) ? 1 : 2; // expected-warning{{}}
----------------
dcoughlin wrote:
> 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.
Whoops, accidental.


https://reviews.llvm.org/D22968