[cfe-commits] r170864 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp test/SemaObjC/arc.m test/SemaObjC/warn-retain-block-property.m

[Jordan Rose]

Better, and a good warning to have! A few more comments...

On Dec 21, 2012, at 0:04 , Ted Kremenek <kremenek at apple.com> wrote:

> Author: kremenek
> Date: Fri Dec 21 02:04:28 2012
> New Revision: 170864
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=170864&view=rev
> Log:
> Extend checkUnsafeAssigns() to also handle assigning an object literal to a weak reference.
> 
> Thanks to Jordan Rose and John McCall for their sage code review.
> 
> Fixes <rdar://problem/12569201>.
> 
> Modified:
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>    cfe/trunk/lib/Sema/SemaChecking.cpp
>    cfe/trunk/test/SemaObjC/arc.m
>    cfe/trunk/test/SemaObjC/warn-retain-block-property.m
> 
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=170864&r1=170863&r2=170864&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Fri Dec 21 02:04:28 2012
> @@ -3689,6 +3689,11 @@
>   "assigning retained object to unsafe property"
>   "; object will be released after assignment">,
>   InGroup<ARCUnsafeRetainedAssign>;
> +def warn_arc_literal_assign : Warning<
> +  "assigning %select{dictionary literal|array literal|block literal|boxed expression}0"
> +  " to a weak %select{property|variable}1"
> +  "; object will be released after assignment">,
> +  InGroup<ARCUnsafeRetainedAssign>;
> def err_arc_new_array_without_ownership : Error<
>   "'new' cannot allocate an array of %0 with no explicit ownership">;
> def err_arc_autoreleasing_var : Error<
> 
> Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=170864&r1=170863&r2=170864&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Dec 21 02:04:28 2012
> @@ -5766,6 +5766,39 @@
>   return false;
> }
> 
> +static bool checkUnsafeAssignLiteral(Sema &S, SourceLocation Loc,
> +                                     Expr *RHS, bool isProperty) {
> +  // Check if RHS is an Objective-C object literal, which also can get
> +  // immediately zapped in a weak reference.  Note that we explicitly
> +  // allow ObjCStringLiterals, since those are designed to never really die.
> +  RHS = RHS->IgnoreParenImpCasts();
> +  unsigned kind = 4;
> +  switch (RHS->getStmtClass()) {
> +    default:
> +      break;
> +    case Stmt::ObjCDictionaryLiteralClass:
> +      kind = 0;
> +      break;
> +    case Stmt::ObjCArrayLiteralClass:
> +      kind = 1;
> +      break;
> +    case Stmt::BlockExprClass:
> +      kind = 2;
> +      break;
> +    case Stmt::ObjCBoxedExprClass:
> +      kind = 3;
> +      break;
> +  }
> +  if (kind < 4) {
> +    S.Diag(Loc, diag::warn_arc_literal_assign)
> +    << kind
> +    << (isProperty ? 0 : 1)
> +    << RHS->getSourceRange();
> +    return true;
> +  }

This is fine, but I've found that a local enum actually makes this much nicer, like the one I wrote in diagnoseObjCLiteralComparison in SemaExpr.cpp. No semantic difference, but it makes the ordering clear, and is about the same amount of work for keeping it in sync with the %select in the diagnostic.


> @@ -5776,6 +5809,10 @@
>   if (checkUnsafeAssignObject(*this, Loc, LT, RHS, false))
>     return true;
> 
> +  if (LT == Qualifiers::OCL_Weak &&
> +      checkUnsafeAssignLiteral(*this, Loc, RHS, false))
> +    return true;
> +
>   return false;
> }
> 
> @@ -5840,6 +5877,8 @@
>     else if (Attributes & ObjCPropertyDecl::OBJC_PR_weak) {
>       if (checkUnsafeAssignObject(*this, Loc, Qualifiers::OCL_Weak, RHS, true))
>         return;
> +      if (checkUnsafeAssignLiteral(*this, Loc, RHS, true))
> +        return;
>     }
>   }
> }

Why not just put checkUnsafeAssignLiteral into checkUnsafeAssignObject? It seems we always want to run them together. I suppose there's a bit of tweaking for weak vs. unsafe_unretained (the latter is okay for literals), but other than that it seems easy enough.



> Modified: cfe/trunk/test/SemaObjC/arc.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/arc.m?rev=170864&r1=170863&r2=170864&view=diff
> ==============================================================================
> --- cfe/trunk/test/SemaObjC/arc.m (original)
> +++ cfe/trunk/test/SemaObjC/arc.m Fri Dec 21 02:04:28 2012
> @@ -4,6 +4,21 @@
> typedef const void * CFTypeRef;
> CFTypeRef CFBridgingRetain(id X);
> id CFBridgingRelease(CFTypeRef);
> + at protocol NSCopying @end
> + at interface NSDictionary
> ++ (id)dictionaryWithObjects:(const id [])objects forKeys:(const id <NSCopying> [])keys count:(NSUInteger)cnt;
> +- (void)setObject:(id)object forKeyedSubscript:(id)key;
> + at end
> + at class NSFastEnumerationState;
> + at protocol NSFastEnumeration
> +- (NSUInteger)countByEnumeratingWithState:(NSFastEnumerationState *)state objects:(id [])buffer count:(NSUInteger)len;
> + at end
> + at interface NSNumber 
> ++ (NSNumber *)numberWithInt:(int)value;
> + at end
> + at interface NSArray <NSFastEnumeration>
> ++ (id)arrayWithObjects:(const id [])objects count:(NSUInteger)cnt;
> + at end
> 
> void test0(void (*fn)(int), int val) {
>   fn(val);
> @@ -717,3 +732,19 @@
> - init { return 0; }
> @end
> 
> +// <rdar://problem/12569201>.  Warn on cases of initializing a weak variable
> +// with an Objective-C object literal.
> +void rdar12569201(id key, id value) {
> +    // Declarations.
> +    __weak id x = @"foo"; // no-warning
> +    __weak id y = @{ key : value }; // expected-warning {{assigning dictionary literal to a weak variable; object will be released after assignment}}
> +    __weak id z = @[ value ]; // expected-warning {{assigning array literal to a weak variable; object will be released after assignment}}
> +    __weak id b = ^() {}; // expected-warning {{assigning block literal to a weak variable; object will be released after assignment}}
> +    __weak id e = @(42); // expected-warning {{assigning boxed expression to a weak variable; object will be released after assignment}}

Some of our other diagnostics (diagnoseObjCLiteralComparison again) differentiate between numeric literals and other kinds of boxed expressions, just to be nice. Is that important here? (If so, maybe we should move the test in diagnoseObjCLiteralComparison out to a helper method on ObjCBoxedExpr.)

Jordan