Re: [PATCH] D23236: When ARC is enabled, no warning will be generated when a method1. Returns 'nil' in a method that is attributed to return a 'nonnull'2. The return-statement is a ConditionalOperator

2016-09-24 Thread Hendrik von Prince via cfe-commits 
parallaxe added a comment.

Uhm, is there something missing from my side that i have overseen to go forward 
with this patch?


https://reviews.llvm.org/D23236



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23236: When ARC is enabled, no warning will be generated when a method1. Returns 'nil' in a method that is attributed to return a 'nonnull'2. The return-statement is a ConditionalOperator

2016-09-28 Thread Hendrik von Prince via cfe-commits 
parallaxe added a comment.

I tried to commit by using arc, though it didn't work as I was prompted to 
login into the subversion-repository, which I can't. At this point I'm not sure 
if this should have worked or not, but I assumed that arc + phabricator would 
handle it in a way that i wouldn't need write-access to the repository itself.
So, yes, it would be nice when someone can actually commit it for me.


https://reviews.llvm.org/D23236



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23236: When ARC is enabled, no warning will be generated when a method1. Returns 'nil' in a method that is attributed to return a 'nonnull'2. The return-statement is a ConditionalOperator, wh

2016-10-04 Thread Hendrik von Prince via cfe-commits 
parallaxe added a comment.

In https://reviews.llvm.org/D23236#557898, @dcoughlin wrote:

> Upon reflection, I don't think this is the right approach.
>
> Desugaring any AttributedType in the return type seems like a really, really 
> big hammer and could be an unexpected surprise for future attributed types 
> where this is not the right behavior. I think a better approach for the 
> nullability issue would be to update `lookThroughImplicitCasts()` in 
> NullabilityChecker.cpp to look though `ExprWithCleanups` expressions just 
> like it currently does for implicit casts.
>
> What do you think?


As far as I understand the code, I would leave my change as it is.
The part of the code I have changed tries to guess the return-type for the 
expression inside the return-stmt. At this point, no type could be computed (as 
RelatedRetType.isNull() is true). Assuming that any annotation that the 
return-type of the function has, will also apply to the return-type of the 
expression, might be misleading. The improvements of the warnings in 
nullability.m are a good sign of this (which would be lost when I would just 
make a change in the NullabilityChecker).

Does that sound sensible?


https://reviews.llvm.org/D23236



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23236: When ARC is enabled, no warning will be generated when a method1. Returns 'nil' in a method that is attributed to return a 'nonnull'2. The return-statement is a ConditionalOperator

2016-08-23 Thread Hendrik von Prince via cfe-commits 
parallaxe marked 4 inline comments as done.
parallaxe added a comment.

In https://reviews.llvm.org/D23236#509013, @ahatanak wrote:

> +cfe-commits
>
> If this patch is applied, does clang issue a warning if a method marked 
> "nonnull" returns a null value? I see a warning is issued for conditional 
> expressions in the test case you've added, but I don't see a test case for a 
> function returning just nil or 0.
>
> I was wondering whether the change made in this patch contradicts what's 
> stated in r240146's commit log:
>
> "Note that we don't warn about nil returns from Objective-C methods,
>
>   because it's common for Objective-C methods to mimic the nil-swallowing
>   behavior of the receiver by checking ostensibly non-null parameters
>   and returning nil from otherwise non-null methods in that
>   case."


I'm not sure how this is meant. As far as I understand the commit-message you 
quoted, it is related to methods that return nil when a precondition fails. A 
test for this would be `doNotWarnWhenPreconditionIsViolatedInTopFunc` in 
nullability_nullonly.mm, which is not affected by my patch. 
On the other side, according to the test `testReturnsNilInNonnull` in 
nullability_nullonly.mm

  - (SomeClass * _Nonnull)testReturnsNilInNonnull {
SomeClass *local = nil;
return local; // expected-warning {{Null is returned from a method that is 
expected to return a non-null value}}
  }

it is expected to produce a warning, when a method marked as nonnull returns 
nil while no precondition is violated. 
So I would expect that the existing tests already proof that my patch didn't 
produce unwanted side-effects.

Sorry in case that I'm missing your point here, maybe you can elaborate it a 
bit more?


https://reviews.llvm.org/D23236



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D23236: When ARC is enabled, no warning will be generated when a method1. Returns 'nil' in a method that is attributed to return a 'nonnull'2. The return-statement is a ConditionalOperator

2016-08-23 Thread Hendrik von Prince via cfe-commits 
parallaxe updated this revision to Diff 69000.
parallaxe added a comment.

Moved the tests into `nullability_nullonly.mm` according to @dcoughlin.
Applied the changes according to @aaron.ballman.


https://reviews.llvm.org/D23236

Files:
  lib/Sema/SemaStmt.cpp
  test/Analysis/nullability_nullonly.mm
  test/SemaObjC/nullability.m

Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -82,7 +82,7 @@
 }
 - (NSFoo *)redundantMethod1 {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' 
from a function with result type 'NSFoo *'}}
 }
 @end
 
@@ -96,7 +96,7 @@
 @implementation NSMerge
 - (NSFoo *)methodA:(NSFoo*)foo {
   int *ptr = foo; // expected-warning{{incompatible pointer types initializing 
'int *' with an expression of type 'NSFoo * _Nonnull'}}
-  return ptr; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ptr; // expected-warning{{incompatible pointer types returning 'int 
*' from a function with result type 'NSFoo *'}}
 }
 
 - (nullable NSFoo *)methodB:(null_unspecified NSFoo*)foo { // 
expected-error{{nullability specifier 'nullable' conflicts with existing 
specifier 'nonnull'}} \
@@ -106,7 +106,7 @@
 
 - (nonnull NSFoo *)methodC:(nullable NSFoo*)foo {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' 
from a function with result type 'NSFoo *'}}
 }
 @end
 
@@ -191,7 +191,7 @@
 @implementation NSResettable // expected-warning{{synthesized setter 
'setResettable4:' for null_resettable property 'resettable4' does not handle 
nil}}
 - (NSResettable *)resettable1 {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSResettable * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' 
from a function with result type 'NSResettable *'}}
 }
 
 - (void)setResettable1:(NSResettable *)param {
Index: test/Analysis/nullability_nullonly.mm
===
--- test/Analysis/nullability_nullonly.mm
+++ test/Analysis/nullability_nullonly.mm
@@ -64,6 +64,10 @@
   }
 }
 
+NSString *_Nonnull testNullReturnInTernaryOperator(int x) {
+  return x > 3 ? nil : [@"" stringByAppendingString:@""]; // expected-warning 
{{Null is returned from a function that is expected to return a non-null value}}
+}
+
 void testPreconditionViolationInInlinedFunction(Dummy *p) {
   doNotWarnWhenPreconditionIsViolated(p);
 }
@@ -145,6 +149,10 @@
   else
 return p; // no-warning
 }
+
+- (NSString * _Nonnull)testNullReturnInTernaryOperator:(int)x {
+  return x > 3 ? nil : [@"" stringByAppendingString:@""]; // expected-warning 
{{Null is returned from a method that is expected to return a non-null value}}
+}
 @end
 
 
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -3340,7 +3340,17 @@
 assert(RetValExp || HasDependentReturnType);
 const VarDecl *NRVOCandidate = nullptr;
 
-QualType RetType = RelatedRetType.isNull() ? FnRetType : RelatedRetType;
+QualType RetType;
+if (RelatedRetType.isNull()) {
+  RetType = FnRetType;
+  // The nullability-attributes of the return-type of a function/method
+  // is not necessarily conforming to the nullability-attributes of
+  // the expression itself, so we remove the nullability-attributes.
+  if(const auto *AttrType = RetType->getAs())
+RetType = AttrType->desugar();
+} else {
+  RetType = RelatedRetType;
+}
 
 // C99 6.8.6.4p3(136): The return statement is not an assignment. The
 // overlap restriction of subclause 6.5.16.1 does not apply to the case of


Index: test/SemaObjC/nullability.m
===
--- test/SemaObjC/nullability.m
+++ test/SemaObjC/nullability.m
@@ -82,7 +82,7 @@
 }
 - (NSFoo *)redundantMethod1 {
   int *ip = 0;
-  return ip; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ip; // expected-warning{{incompatible pointer types returning 'int *' from a function with result type 'NSFoo *'}}
 }
 @end
 
@@ -96,7 +96,7 @@
 @implementation NSMerge
 - (NSFoo *)methodA:(NSFoo*)foo {
   int *ptr = foo; // expected-warning{{incompatible pointer types initializing 'int *' with an expression of type 'NSFoo * _Nonnull'}}
-  return ptr; // expected-warning{{result type 'NSFoo * _Nonnull'}}
+  return ptr; // expected-warning{{incompatible pointer types returning 'int *' from a function with result type 'NSFoo *'}}
 }
 
 - (nullable NSFoo *)methodB:(null_unspecified NSFoo*)foo { // expected-error{{nullability specifier 'nullable' conflicts with existing specifier 'nonnull'}} \
@@ -106,7 +106,7 @@
 
 - (nonnull NS