Page MenuHomePhabricator

[ObjC generics] Fix not inheriting type bounds in categories/extensions.
ClosedPublic

Authored by vsapsai on Aug 23 2019, 7:05 PM.

Details

Summary

When a category/extension doesn't repeat a type bound, corresponding
type parameter is substituted with id when used as a type argument. As
a result, in the added test case it was causing errors like

type argument 'T' (aka 'id') does not satisfy the bound ('id<NSCopying>') of type parameter 'T'

We are already checking that type parameters should be consistent
everywhere (see checkTypeParamListConsistency) and update
ObjCTypeParamDecl to have correct underlying type. And when we use the
type parameter as a method return type or a method parameter type, it is
substituted to the bounded type. But when we use the type parameter as a
type argument, we check ObjCTypeParamType that ignores the updated
underlying type and remains id.

Fix by desugaring ObjCTypeParamType to the underlying type, the same
way we are doing with TypedefType.

rdar://problem/54329242

Diff Detail

Event Timeline

vsapsai created this revision.Aug 23 2019, 7:05 PM

I'm a bit curious about clients that use getCanonicalType() to get a full desugaring, instead of doing a single step. It seems like they'd still get the out of date type parameter type. Has that ever worked?

I'm a bit curious about clients that use getCanonicalType() to get a full desugaring, instead of doing a single step. It seems like they'd still get the out of date type parameter type. Has that ever worked?

Good point. I believe it never worked but don't have specific examples. Suspect that we haven't seen such problems because ObjCTypeParamType has limited usage before it is substituted with a concrete type.

While checking the code I've noticed we can make a small cleanup - in

QualType getObjCTypeParamType(const ObjCTypeParamDecl *Decl,
                              ArrayRef<ObjCProtocolDecl *> protocols,
                              QualType Canonical = QualType()) const;

remove the parameter Canonical because no caller provides it and it's not the best idea to allow to provide Canonical type that can be inconsistent with ObjCTypeParamDecl type. Plan to do the cleanup in a separate commit.

Found a use case where getCanonicalType() causes problems:

@interface NSObject
@end

@protocol SomeProtocol
@end

@interface NSString : NSObject
@end
@interface NSNumber : NSObject
@end

@interface Container<T : NSString *>
@end

@interface Container<T> (extension)
T FunctionInExtension(void);
@end

void test(Container *c) {
  NSNumber *n = FunctionInExtension();
}

In this case we don't warn about NSNumber/NSString mismatch but should. At line 8125 in Sema::CheckAssignmentConstraints we get a canonical type for the right-hand side and it turned out to be id, which is compatible with NSNumber.

I believe it's a separate issue and the currently proposed change is correct but insufficient. Prior to using ObjCTypeParamType for generic parameters, we were using TypedefType and I think we need to make ObjCTypeParamType like TypedefType in more places.

erik.pilkington accepted this revision.Sep 30 2019, 11:14 AM

Okay, LGTM.

This revision is now accepted and ready to land.Sep 30 2019, 11:14 AM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 12:32 PM