Fix errored return value in CheckFunctionReturnType and add a fixit hint

[erichkeane]

Summary

As discovered by ChenWJ and listed on cfe-dev, the error for Objective C return type ended up being wrong. This fixes that. Additionally, as a "while we're there", the other usages of this error and the usage of the FP above both use a FixItHint, so I'll add it here.

I couldn't come up with a condition where this would cause the compiler to do anything odd, so I don't really have a good testcase for it. If any of you do, please let me know and I can add that!

[rnk]

On cfe-dev you said "There IS a case in our tests where it would hit such an assertion". Does "our tests" here refer to the existing clang tests or Intel tests? I just want to make sure we have coverage of this codepath in the test suite. I don't care that much about crafting a test case that shows the behavior difference of fixing the return value.

[erichkeane]

In D32759#743860, @rnk wrote:

On cfe-dev you said "There IS a case in our tests where it would hit such an assertion". Does "our tests" here refer to the existing clang tests or Intel tests? I just want to make sure we have coverage of this codepath in the test suite. I don't care that much about crafting a test case that shows the behavior difference of fixing the return value.

Ah, sorry. "Our Tests" means the lit test SemaObjC/method-bad-param.m (line 11). I ran the lit tests initially with a breakpoint on this line and it never hit, though I must have set up the debugger wrong. Once I replaced it with an assert, method-bad-param failed.

[chenwj]

Ah, sorry. "Our Tests" means the lit test SemaObjC/method-bad-param.m (line 11). I ran the lit tests initially with a breakpoint on this line and it never hit, though I must have set up the debugger wrong. Once I replaced it with an assert, method-bad-param failed.

The fix LGTM. Is it possible to tweak method-bad-param.m so that we can see the difference after the fix?

[erichkeane]

In D32759#744613, @chenwj wrote:

Ah, sorry. "Our Tests" means the lit test SemaObjC/method-bad-param.m (line 11). I ran the lit tests initially with a breakpoint on this line and it never hit, though I must have set up the debugger wrong. Once I replaced it with an assert, method-bad-param failed.

The fix LGTM. Is it possible to tweak method-bad-param.m so that we can see the difference after the fix?

I actually couldn't come up with a way where we COULD see the difference... I was hoiping someone else could come up with something if it were important enough for us.

[chenwj]

@eli.friedman I find you added isObjCObjectType check in svn revision 184006 (git commit ddb5a392). Could you confirm returning zero rather than true here is okay? A little explanation would be even better. Thanks.

[chenwj]

@erichkeane Just share what I investigated.

In D32759#744769, @erichkeane wrote:

In D32759#744613, @chenwj wrote:

Ah, sorry. "Our Tests" means the lit test SemaObjC/method-bad-param.m (line 11). I ran the lit tests initially with a breakpoint on this line and it never hit, though I must have set up the debugger wrong. Once I replaced it with an assert, method-bad-param failed.

The fix LGTM. Is it possible to tweak method-bad-param.m so that we can see the difference after the fix?

I actually couldn't come up with a way where we COULD see the difference... I was hoiping someone else could come up with something if it were important enough for us.

I fwd to @eli.friedman who added the check. Maybe he is the right person who can explain what's going on here, and decide if this patch is okay or not.

[efriedma]

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

@class NSObject;
template<typename T> struct C {
    static T f();
};
int g() { NSObject *x = C<NSObject>::f(); }

[chenwj]

In D32759#748007, @efriedma wrote:

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

So when we see T->isObjCObjectType() is true, then we should return true since the return type is invalid?

[efriedma]

In D32759#748653, @chenwj wrote:

In D32759#748007, @efriedma wrote:

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

So when we see T->isObjCObjectType() is true, then we should return true since the return type is invalid?

Yes.

[chenwj]

In D32759#748916, @efriedma wrote:

In D32759#748653, @chenwj wrote:

In D32759#748007, @efriedma wrote:

The difference between returning true and false here is just the way error recovery works: when we return true, we know the type is invalid, so we suppress it, and subsequent errors involving the declaration. Example (Objective-C++) where we currently print two errors:

So when we see T->isObjCObjectType() is true, then we should return true since the return type is invalid?

Yes.

@efriedma So @erichkeane 's patch looks okay?

[efriedma]

A testcase would be nice... but otherwise, yes.

[erichkeane]

In D32759#751026, @efriedma wrote:

A testcase would be nice... but otherwise, yes.

Do you have a suggestion on how to write one? I wasn't able to find any situation that changed behavior with the different return values.

[efriedma]

The testcase I wrote should change behavior, I think.

[erichkeane]

In D32759#751028, @efriedma wrote:

The testcase I wrote should change behavior, I think.

Alright, I'll look into it again...

[erichkeane]

Added a test, found a slight difference in behavior!

[efriedma]

Our behavior for your testcase is less than ideal (we should be generating an invalid declaration instead of just dropping it on the floor), but that's not something you need to fix here.

Could you also include the Objective-C++ testcase I wrote? Or does it not work the way I expected?

[erichkeane]

In D32759#751339, @efriedma wrote:

Our behavior for your testcase is less than ideal (we should be generating an invalid declaration instead of just dropping it on the floor), but that's not something you need to fix here.

Could you also include the Objective-C++ testcase I wrote? Or does it not work the way I expected?

I hadn't seen it. I see from your diff now that you have a .mm file (p4.mm) which I presume is what you mean. I'll check on it and get back to you on it.

[efriedma]

Err, I meant the one I wrote earlier in this thread: https://reviews.llvm.org/D32759#748007

[erichkeane]

In D32759#751350, @efriedma wrote:

Err, I meant the one I wrote earlier in this thread: https://reviews.llvm.org/D32759#748007

AH! I missed that one. I'll create a new file in SemaObjCXX then.

[efriedma]

LGTM

[efriedma]

Err, oh, meant to add one minor comment: please rename "method-bad-param.mm" to "interface-return-type.mm".

lib/Sema/SemaType.cpp

if (T->isHalfType() && !getLangOpts().HalfArgsAndReturns) {
     Diag(Loc, diag::err_parameters_retval_cannot_have_fp16_type) << 1 <<
       FixItHint::CreateInsertion(Loc, "*");
     return true;
   }
 
   // Methods cannot return interface types. All ObjC objects are
   // passed by reference.
   if (T->isObjCObjectType()) {
-    Diag(Loc, diag::err_object_cannot_be_passed_returned_by_value) << 0 << T;
-    return 0;
+    Diag(Loc, diag::err_object_cannot_be_passed_returned_by_value)
+        << 0 << T << FixItHint::CreateInsertion(Loc, "*");
+    return true;
   }
 
   return false;
 }
 
 /// Check the extended parameter information.  Most of the necessary
 /// checking should occur when applying the parameter attribute; the
 /// only other checks required are positional restrictions.

test/SemaObjC/method-bad-param.m

 -(void) my_method:(foo) my_param  // expected-error {{interface type 'foo' cannot be passed by value; did you forget * in 'foo'}}
 {
 }
 - (foo)cccccc:(long)ddddd // expected-error {{interface type 'foo' cannot be returned by value; did you forget * in 'foo'}}
 {
 }
 @end
 
+// Ensure that this function is properly marked as a failure.
+void func_with_bad_call(bar* b, foo* f) {
+  [b cccccc:5]; // expected-warning {{instance method '-cccccc:' not found}}
+                // expected-note@-17 {{receiver is instance of class declared here}}
+}
+
 void somefunc(foo x) {} // expected-error {{interface type 'foo' cannot be passed by value; did you forget * in 'foo'}}
 foo somefunc2() {} // expected-error {{interface type 'foo' cannot be returned by value; did you forget * in 'foo'}}
 
 // rdar://6780761
 void f0(foo *a0) {
   extern void g0(int x, ...);
   g0(1, *(foo*)a0);  // expected-error {{cannot pass object with interface type 'foo' by value through variadic function}}
 }

test/SemaObjCXX/method-bad-param.mm

+// RUN: %clang_cc1 -fsyntax-only -std=c++11 %s -verify
+
+@class NSObject;
+template<typename T> struct C {
+      static T f(); // expected-error {{interface type 'NSObject' cannot be returned by value; did you forget * in 'NSObject'?}}
+};
+int g() { NSObject *x = C<NSObject>::f(); }//expected-error {{no member named 'f' in 'C<NSObject>'}} expected-note {{in instantiation of template class 'C<NSObject>' requested here}}