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