Page MenuHomePhabricator

Fix errored return value in CheckFunctionReturnType and add a fixit hint
ClosedPublic

Authored by erichkeane on May 2 2017, 1:58 PM.

Details

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!

Event Timeline

erichkeane created this revision.May 2 2017, 1:58 PM
erichkeane retitled this revision from Fix errored return value in CheckFunctionReturnType and add a to Fix errored return value in CheckFunctionReturnType and add a fixit hint.
erichkeane edited the summary of this revision. (Show Details)
rnk edited edge metadata.May 2 2017, 2:47 PM

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.

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 edited edge metadata.May 3 2017, 6:11 AM

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?

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.

@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.

@erichkeane Just share what I investigated.

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.

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 added a comment.May 8 2017, 6:44 AM

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?

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.

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?

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

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.

The testcase I wrote should change behavior, I think.

The testcase I wrote should change behavior, I think.

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

erichkeane added a reviewer: eli.friedman.

Added a test, found a slight difference in behavior!

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?

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.

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

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.

This revision is now accepted and ready to land.May 10 2017, 1:13 PM

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

This revision was automatically updated to reflect the committed changes.