[PATCH] D15463: [Objective-c] Fix a crash

[Justin Bogner via cfe-commits]

Akira Hatanaka <ahatanak at gmail.com> writes:
> On Mon, Dec 14, 2015 at 10:39 AM, Justin Bogner <mail at justinbogner.com>
> wrote:
>
>> Akira Hatanaka via cfe-commits <cfe-commits at lists.llvm.org> writes:
>> > ahatanak created this revision.
>> > ahatanak added a subscriber: cfe-commits.
>> >
>> > This patch fixes a crash that occurs when __kindof is incorrectly used
>> > in the type parameter list of an interface. The crash occurs because
>> > ObjCTypeParamList::back() is called in checkTypeParamListConsistency
>> > on an empty list:
>> >
>> > 00762       diagLoc =
>> S.getLocForEndOfToken(newTypeParams->back()->getLocEnd());
>> >
>> > http://reviews.llvm.org/D15463
>> >
>> > Files:
>> >   lib/Parse/ParseObjc.cpp
>> >   test/SemaObjC/kindof.m
>> >
>> > Index: test/SemaObjC/kindof.m
>> > ===================================================================
>> > --- test/SemaObjC/kindof.m
>> > +++ test/SemaObjC/kindof.m
>> > @@ -302,3 +302,13 @@
>> >    void processCopyable(__typeof(getSomeCopyable()) string);
>> >    processCopyable(0); // expected-warning{{null passed to a callee that
>> requires a non-null argument}}
>> >  }
>> > +
>> > +// __kinddof cannot be used in parameter list.
>> > + at interface Array1<T> : NSObject
>> > + at end
>> > +
>> > + at interface I1 : NSObject
>> > + at end
>> > +
>> > + at interface Array1<__kindof I1*>(extensions) // //
>> expected-error{{expected type parameter name}}
>> > + at end
>> > Index: lib/Parse/ParseObjc.cpp
>> > ===================================================================
>> > --- lib/Parse/ParseObjc.cpp
>> > +++ lib/Parse/ParseObjc.cpp
>> > @@ -603,7 +603,7 @@
>> >    // whether there are any protocol references.
>> >    lAngleLoc = SourceLocation();
>> >    rAngleLoc = SourceLocation();
>> > -  return list;
>> > +  return invalid ? nullptr : list;
>>
>> This looks a bit suspicious to me. Since `invalid` is set *way* earlier
>> in the function, it seems like we should be able to return earlier if
>> this is correct, and not even call actOnObjCTypeParamList. OTOH, are
>> there cases where `invalid == true` but list is non-empty? If so, are we
>> doing the right thing when that happens?
>>
>>
> I'm assuming you meant it should return earlier if this is *incorrect* (but
> let me know if I misunderstood your comment).

I meant, "If `return invalid ? nullptr : list` is correct, then I
suspect returning nullptr before calling actOnObjCTypeParamList makes
more sense.

> The only downside to returning nullptr before actOnObjCTypeParamList is
> called in line 595 is that it won't print the diagnostics that are printed
> in actOnObjCTypeParamList. For example, if the following interface were
> compiled,
>
> @interface I1<T, T, __kindof I2*> NSObject
> @end
>
> actOnObjCTypeParamList could tell the user that parameter "T" was
> redeclared in addition to printing the error message about __kindof. So if
> we return before it's called, we won't see that error message.
>
> When the interface declaration of category extensions1 in the following
> piece of code is compiled, invalid is set to true and the list is not empty
> (it contains the first two parameters).
>
> @interface I1<T1, T2, T3> NSObject
> @end
>
> @interface I1<T, T, __kindof I2*> (extensions1)
> @end
>
> *test3.m:6:23: **error: **expected type parameter name*
>
> @interface I6<T1, T2, __kindof I2*> (extensions1)
>
> *                      ^*
>
> *test3.m:6:21: **error: **category has too few type parameters (expected 3,
> have 2)*
>
> @interface I6<T1, T2, __kindof I2*> (extensions1)
> It looks like it's doing the right thing, but the second diagnostic doesn't
> seem necessary to me.

I'm not sure I follow exactly which errors are being reported with your
patch, so maybe this is already working, but what we basically want is
to only emit errors that are helpful.

That is, if we get the `"T" is a redeclaration` error as well as the
__kindof error, that's nice, but if we get the `too few type parameters`
error that's going to be confusing and bad - the user didn't provide too
few type parameters, but our previous error caused us to see too few.

Basically, we don't ever want to report errors that are the result of
previous errors, but as long as we don't end up doing that then
gathering further errors is totally reasonable. Make sense?

>
>>  }
>> >
>> >  /// Parse an objc-type-parameter-list.
>> >
>> >
>>