<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On May 13, 2013, at 14:48 , Anna Zaks <<a href="mailto:ganna@apple.com">ganna@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Author: zaks<br>Date: Mon May 13 16:48:20 2013<br>New Revision: 181738<br><br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project?rev=181738&view=rev">http://llvm.org/viewvc/llvm-project?rev=181738&view=rev</a><br>Log:<br>[analyzer] Warn about nil elements/keys/values in array and dictionary literals.<br><br>Modified:<br> cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp<br> cfe/trunk/test/Analysis/NSContainers.m<br><br>Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp<br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp?rev=181738&r1=181737&r2=181738&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp?rev=181738&r1=181737&r2=181738&view=diff</a><br>==============================================================================<br>--- cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp (original)<br>+++ cfe/trunk/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp Mon May 13 16:48:20 2013<br>@@ -90,20 +90,53 @@ static FoundationClass findKnownClass(co<br>//===----------------------------------------------------------------------===//<br><br>namespace {<br>- class NilArgChecker : public Checker<check::PreObjCMessage> {<br>+ class NilArgChecker : public Checker<check::PreObjCMessage,<br>+ check::PostStmt<ObjCDictionaryLiteral>,<br>+ check::PostStmt<ObjCArrayLiteral> > {<br> mutable OwningPtr<APIMisuse> BT;<br><br>- void WarnIfNilArg(CheckerContext &C,<br>- const ObjCMethodCall &msg, unsigned Arg,<br>- FoundationClass Class,<br>- bool CanBeSubscript = false) const;<br>+ void warnIfNilExpr(const Expr *E,<br>+ const char *Msg,<br>+ CheckerContext &C) const;<br>+<br>+ void warnIfNilArg(CheckerContext &C,<br>+ const ObjCMethodCall &msg, unsigned Arg,<br>+ FoundationClass Class,<br>+ bool CanBeSubscript = false) const;<br>+<br>+ void generateBugReport(ExplodedNode *N,<br>+ llvm::raw_svector_ostream &os,<br>+ SourceRange Range,<br>+ const Expr *Expr,<br>+ CheckerContext &C) const;<br><br> public:<br> void checkPreObjCMessage(const ObjCMethodCall &M, CheckerContext &C) const;<br>+ void checkPostStmt(const ObjCDictionaryLiteral *DL,<br>+ CheckerContext &C) const;<br>+ void checkPostStmt(const ObjCArrayLiteral *AL,<br>+ CheckerContext &C) const;<br> };<br>}<br><br>-void NilArgChecker::WarnIfNilArg(CheckerContext &C,<br>+void NilArgChecker::warnIfNilExpr(const Expr *E,<br>+ const char *Msg,<br>+ CheckerContext &C) const {<br>+ ProgramStateRef State = C.getState();<br>+ SVal SV = State->getSVal(E, C.getLocationContext());<br></div></blockquote><div><br></div><div>C.getSVal(E) ?</div><div><br></div><div>Also, we should probably be recording these assumptions -- otherwise we'll get an inconsistency later:</div><div><br></div><div>int i;</div><div>if (coin) {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>[self doSomethingWith:@[ value ]];</div><div>} else {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>i = 0;</div><div>}</div><div><br></div><div>if (!value) {</div><div><span class="Apple-tab-span" style="white-space:pre"> </span>use(i); // not actually uninitialized</div><div>}</div><div><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ if (State->isNull(SV).isConstrainedTrue()) {<br>+<br>+ if (ExplodedNode *N = C.generateSink()) {<br>+ SmallString<128> sbuf;<br>+ llvm::raw_svector_ostream os(sbuf);<br>+ os << Msg;<br>+ generateBugReport(N, os, E->getSourceRange(), E, C);<br>+ }<br>+ <br>+ }<br>+}<br>+<br>+void NilArgChecker::warnIfNilArg(CheckerContext &C,<br> const ObjCMethodCall &msg,<br> unsigned int Arg,<br> FoundationClass Class,<br>@@ -113,9 +146,6 @@ void NilArgChecker::WarnIfNilArg(Checker<br> if (!State->isNull(msg.getArgSVal(Arg)).isConstrainedTrue())<br> return;<br><br>- if (!BT)<br>- BT.reset(new APIMisuse("nil argument"));<br>-<br> if (ExplodedNode *N = C.generateSink()) {<br> SmallString<128> sbuf;<br> llvm::raw_svector_ostream os(sbuf);<br>@@ -149,14 +179,26 @@ void NilArgChecker::WarnIfNilArg(Checker<br> << msg.getSelector().getAsString() << "' cannot be nil";<br> }<br> }<br>-<br>- BugReport *R = new BugReport(*BT, os.str(), N);<br>- R->addRange(msg.getArgSourceRange(Arg));<br>- bugreporter::trackNullOrUndefValue(N, msg.getArgExpr(Arg), *R);<br>- C.emitReport(R);<br>+ <br>+ generateBugReport(N, os, msg.getArgSourceRange(Arg),<br>+ msg.getArgExpr(Arg), C);<br> }<br>}<br><br>+void NilArgChecker::generateBugReport(ExplodedNode *N,<br>+ llvm::raw_svector_ostream &os,<br></div></blockquote><div><br></div><div>Since we're not putting anything else into the string, it makes more sense to just pass a StringRef here.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ SourceRange Range,<br>+ const Expr *Expr,<br></div></blockquote><div><br></div><div>'Expr' is not a good name because it shadows the type.</div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ CheckerContext &C) const {<br>+ if (!BT)<br>+ BT.reset(new APIMisuse("nil argument"));<br>+<br>+ BugReport *R = new BugReport(*BT, os.str(), N);<br>+ R->addRange(Range);<br>+ bugreporter::trackNullOrUndefValue(N, Expr, *R);<br>+ C.emitReport(R);<br>+}<br>+<br>void NilArgChecker::checkPreObjCMessage(const ObjCMethodCall &msg,<br> CheckerContext &C) const {<br> const ObjCInterfaceDecl *ID = msg.getReceiverInterface();<br>@@ -225,28 +267,43 @@ void NilArgChecker::checkPreObjCMessage(<br> if (S.getNameForSlot(0).equals("dictionaryWithObject") &&<br> S.getNameForSlot(1).equals("forKey")) {<br> Arg = 0;<br>- WarnIfNilArg(C, msg, /* Arg */1, Class);<br>+ warnIfNilArg(C, msg, /* Arg */1, Class);<br> } else if (S.getNameForSlot(0).equals("setObject") &&<br> S.getNameForSlot(1).equals("forKey")) {<br> Arg = 0;<br>- WarnIfNilArg(C, msg, /* Arg */1, Class);<br>+ warnIfNilArg(C, msg, /* Arg */1, Class);<br> } else if (S.getNameForSlot(0).equals("setObject") &&<br> S.getNameForSlot(1).equals("forKeyedSubscript")) {<br> CanBeSubscript = true;<br> Arg = 0;<br>- WarnIfNilArg(C, msg, /* Arg */1, Class, CanBeSubscript);<br>+ warnIfNilArg(C, msg, /* Arg */1, Class, CanBeSubscript);<br> } else if (S.getNameForSlot(0).equals("removeObjectForKey")) {<br> Arg = 0;<br> }<br> }<br><br>-<br> // If argument is '0', report a warning.<br> if ((Arg != InvalidArgIndex))<br>- WarnIfNilArg(C, msg, Arg, Class, CanBeSubscript);<br>+ warnIfNilArg(C, msg, Arg, Class, CanBeSubscript);<br><br>}<br><br>+void NilArgChecker::checkPostStmt(const ObjCArrayLiteral *AL,<br>+ CheckerContext &C) const {<br>+ for (unsigned i = 0; i < AL->getNumElements(); ++i) {<br></div></blockquote><div><br></div><div>We should avoid calling AL->getNumElements() over and over (yes, the compiler will deal with it, but still).</div><div><br></div><div><br></div><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">+ warnIfNilExpr(AL->getElement(i), "Array element cannot be nil", C);<br>+ }<br>+}<br>+<br>+void NilArgChecker::checkPostStmt(const ObjCDictionaryLiteral *DL,<br>+ CheckerContext &C) const {<br>+ for (unsigned i = 0; i < DL->getNumElements(); ++i) {<br>+ ObjCDictionaryElement Element = DL->getKeyValueElement(i);<br>+ warnIfNilExpr(Element.Key, "Dictionary key cannot be nil", C);<br>+ warnIfNilExpr(Element.Value, "Dictionary value cannot be nil", C);<br>+ }<br>+}<br>+<br>//===----------------------------------------------------------------------===//<br>// Error reporting.<br>//===----------------------------------------------------------------------===//<br><br>Modified: cfe/trunk/test/Analysis/NSContainers.m<br>URL:<span class="Apple-converted-space"> </span><a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/NSContainers.m?rev=181738&r1=181737&r2=181738&view=diff">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/NSContainers.m?rev=181738&r1=181737&r2=181738&view=diff</a><br>==============================================================================<br>--- cfe/trunk/test/Analysis/NSContainers.m (original)<br>+++ cfe/trunk/test/Analysis/NSContainers.m Mon May 13 16:48:20 2013<br>@@ -36,6 +36,10 @@ typedef struct _NSZone NSZone;<br>- (void)setObject:(id)obj atIndexedSubscript:(NSUInteger)idx __attribute__((availability(macosx,introduced=10.8)));<br>@end<br><br>+@interface NSArray (NSArrayCreation)<br>++ (instancetype)arrayWithObjects:(const id [])objects count:(NSUInteger)cnt;<br>+@end<br>+<br>@interface NSMutableArray : NSArray<br><br>- (void)addObject:(id)anObject;<br>@@ -58,6 +62,8 @@ typedef struct _NSZone NSZone;<br><br>+ (id)dictionary;<br>+ (id)dictionaryWithObject:(id)object forKey:(id <NSCopying>)key;<br>++ (instancetype)dictionaryWithObjects:(const id [])objects forKeys:(const id <NSCopying> [])keys count:(NSUInteger)cnt;<br>+<br>@end<br><br>@interface NSMutableDictionary : NSDictionary<br>@@ -147,6 +153,33 @@ NSDictionary *testNilArgNSDictionary2(NS<br> return [NSDictionary dictionaryWithObject:obj forKey:0]; // expected-warning {{Key argument to 'dictionaryWithObject:forKey:' cannot be nil}}<br>}<br><br>+id testCreateDictionaryLiteralKey(id value, id nilKey) {<br>+ if (nilKey)<br>+ ;<br>+ return @{@"abc":value, nilKey:@"abc"}; // expected-warning {{Dictionary key cannot be nil}}<br>+}<br>+<br>+id testCreateDictionaryLiteralValue(id nilValue) {<br>+ if (nilValue)<br>+ ;<br>+ return @{@"abc":nilValue}; // expected-warning {{Dictionary value cannot be nil}}<br>+}<br>+<br>+id testCreateDictionaryLiteral(id nilValue, id nilKey) {<br>+ if (nilValue)<br>+ ;<br>+ if (nilKey)<br>+ ;<br>+ return @{@"abc":nilValue, nilKey:@"abc"}; // expected-warning {{Dictionary key cannot be nil}}<br>+ // expected-warning@-1 {{Dictionary value cannot be nil}}<br>+}<br>+<br>+id testCreateArrayLiteral(id myNil) {<br>+ if (myNil)<br>+ ;<br>+ return @[ @"a", myNil, @"c" ]; // expected-warning {{Array element cannot be nil}}<br>+}<br>+<br></div></blockquote><div><br></div><div>We should have a test to show that we're getting path notes and/or suppression for at least one of these cases.</div><div><br></div><br><blockquote type="cite"><div style="letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">// Test inline defensive checks suppression.<br>void idc(id x) {<br> if (x)<br><br><br>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a></div></blockquote></div><br></body></html>