Re: [PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.
zaks.anna added a comment.
Here are more comments. Could you address/answer these and upload the latest
patch that compares NSNumber to other numbers?
Thanks!
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:88
@@ +87,3 @@
+
+auto NSNumberExprM =
+expr(ignoringParenImpCasts(expr(hasType(hasCanonicalType(
Can you test if matching for NSNumber works when they come from ivars,
properties, and method returns, works?
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:117
@@ +116,3 @@
+auto ConversionThroughComparisonM =
+binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(NSNumberExprM),
Can we use any binary operator here without any restrictions?
Comment at: test/Analysis/bool-conversion.cpp:18
@@ +17,3 @@
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
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..
Comment at: test/Analysis/bool-conversion.cpp:21
@@ +20,3 @@
+ p ? 1 : 2; // expected-warning{{}}
+ (bool)p; // expected-warning{{}}
+#endif
Why is this OK?
Comment at: test/Analysis/bool-conversion.m:2
@@ +1,3 @@
+// RUN: %clang_cc1 -fblocks -w -analyze
-analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -fblocks -w -analyze
-analyzer-checker=alpha.osx.BoolConversion -analyzer-config
alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify
+
dcoughlin wrote:
> 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.
+ 1!!!
Especially, since we are matching the AST.
Comment at: test/Analysis/bool-conversion.m:10
@@ +9,3 @@
+ if (!p) {} // expected-warning{{}}
+ if (p == YES) {} // expected-warning{{}}
+ if (p == NO) {} // expected-warning{{}}
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.
https://reviews.llvm.org/D22968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.
zaks.anna added a comment.
Let's test it on more real word bugs.
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:11
@@ +10,3 @@
+// This file defines BoolConversionChecker, which checks for a particular
+// common mistake when dealing with NSNumber and OSBoolean objects:
+// When having a pointer P to such object, neither `if (P)' nor `bool X = P'
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.
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:49
@@ +48,3 @@
+ virtual void run(const MatchFinder::MatchResult &Result) {
+// This callback only throws reports. All the logic is in the matchers.
+const Stmt *Conv = Result.Nodes.getNodeAs("conv");
"throws" -> "generates"?
https://reviews.llvm.org/D22968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.
dcoughlin added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:62
@@ +61,3 @@
+ << "' to a plain boolean value: probably a forgotten "
+ << (IsObjC ? "'[boolValue]'" : "'->isTrue()'");
+BR.EmitBasicReport(
- 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.
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:80
@@ +79,3 @@
+
+auto OSBooleanExprM =
+expr(ignoringParenImpCasts(
The AST matchers seem pretty convenient!
Comment at: test/Analysis/bool-conversion.cpp:18
@@ +17,3 @@
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
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.
Comment at: test/Analysis/bool-conversion.m:2
@@ +1,3 @@
+// RUN: %clang_cc1 -fblocks -w -analyze
-analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -fblocks -w -analyze
-analyzer-checker=alpha.osx.BoolConversion -analyzer-config
alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify
+
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.
Comment at: test/Analysis/bool-conversion.m:6
@@ +5,3 @@
+
+void bad(const NSNumber *p) {
+#ifdef PEDANTIC
These 'const's are not idiomatic. It is probably better to remove them.
Comment at: test/Analysis/bool-conversion.m:10
@@ +9,3 @@
+ if (!p) {} // expected-warning{{}}
+ if (p == YES) {} // expected-warning{{}}
+ if (p == NO) {} // expected-warning{{}}
There is a Sema warning for `p == YES` already, right? ("comparison between
pointer and integer ('NSNumber *' and 'int')")
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{{}}
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.
https://reviews.llvm.org/D22968
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.
NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.
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, 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.
https://reviews.llvm.org/D22968
Files:
include/clang/StaticAnalyzer/Checkers/Checkers.td
lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp
lib/StaticAnalyzer/Checkers/CMakeLists.txt
test/Analysis/Inputs/system-header-simulator-objc.h
test/Analysis/bool-conversion.cpp
test/Analysis/bool-conversion.m
Index: test/Analysis/bool-conversion.m
===
--- /dev/null
+++ test/Analysis/bool-conversion.m
@@ -0,0 +1,34 @@
+// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=alpha.osx.BoolConversion -analyzer-config alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify
+
+#include "Inputs/system-header-simulator-objc.h"
+
+void bad(const NSNumber *p) {
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
+ if (p == YES) {} // expected-warning{{}}
+ if (p == NO) {} // expected-warning{{}}
+ (!p) ? 1 : 2; // expected-warning{{}}
+ (BOOL)p; // expected-warning{{}}
+#endif
+ BOOL x = p; // expected-warning{{}}
+ x = p; // expected-warning{{}}
+ x = (p == YES); // expected-warning{{}}
+}
+
+typedef const NSNumber *SugaredNumber;
+void bad_sugared(SugaredNumber p) {
+ p == YES; // expected-warning{{}}
+}
+
+void good(const NSNumber *p) {
+ if ([p boolValue] == NO) {} // no-warning
+ if ([p boolValue] == YES) {} // no-warning
+ BOOL x = [p boolValue]; // no-warning
+}
+
+void suppression(const NSNumber *p) {
+ if (p == NULL) {} // no-warning
+ if (p == nil) {} // no-warning
+}
Index: test/Analysis/bool-conversion.cpp
===
--- /dev/null
+++ test/Analysis/bool-conversion.cpp
@@ -0,0 +1,42 @@
+// RUN: %clang_cc1 -w -std=c++11 -analyze -analyzer-checker=alpha.osx.BoolConversion %s -verify
+// RUN: %clang_cc1 -w -std=c++11 -analyze -analyzer-checker=alpha.osx.BoolConversion -analyzer-config alpha.osx.BoolConversion: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{{}}
+ if (!p) {} // expected-warning{{}}
+ p ? 1 : 2; // expected-warning{{}}
+ (bool)p; // expected-warning{{}}
+#endif
+ bool x = p; // expected-warning{{}}
+ x = p; // expected-warning{{}}
+}
+
+typedef bool sugared_bool;
+typedef const OSBoolean *sugared_OSBoolean;
+void bad_sugared(sugared_OSBoolean p) {
+ sugared_bool x = p; // expected-warning{{}}
+}
+
+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
+}
Index: test/Analysis/Inputs/system-header-simulator-objc.h
===
--- test/Analysis/Inputs/system-header-simulator-objc.h
+++ test/Analysis/Inputs/system-header-simulator-objc.h
@@ -10,10 +10,16 @@
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,
Index: lib/StaticAnalyzer/Checkers/CMakeLists.txt
===
--- lib/StaticAn
