Page MenuHomePhabricator

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

Authored by vsapsai on Jan 16 2020, 1:18 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 wasn't updated and
remains id.

Fix by updating not only ObjCTypeParamDecl UnderlyingType but also
TypeForDecl as we use the underlying type to create a canonical type for
ObjCTypeParamType (see ASTContext::getObjCTypeParamType).

This is a different approach to fixing the issue. The previous one was
02c2ab3d8872416589bd1a6ca3dfb96ba373a3b9 which was reverted in
4c539e8da1b3de38a53ef3f7497f5c45a3243b61. The problem with the previous
approach was that ObjCTypeParamType::desugar was returning underlying
type for ObjCTypeParamDecl without applying any protocols stored in
ObjCTypeParamType. It caused inconsistencies in comparing types before
and after desugaring.

rdar://problem/54329242

Diff Detail

Event Timeline

vsapsai created this revision.Jan 16 2020, 1:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2020, 1:18 PM

Unit tests: pass. 61930 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

vsapsai added a subscriber: hans.Jan 17 2020, 12:40 PM
hans added a comment.Feb 11 2020, 2:01 AM

I don't have the context here. Was I added as a subscriber because it's related to the clang 10 release?

I don't have the context here. Was I added as a subscriber because it's related to the clang 10 release?

It's not related to clang 10 release. I've added you because earlier you've found a problem with a previous approach https://github.com/llvm/llvm-project/commit/4c539e8da1b3de38a53ef3f7497f5c45a3243b61 So in case it breaks something else, you have extra visibility into the change.

hans added a comment.Feb 12 2020, 7:07 AM

I don't have the context here. Was I added as a subscriber because it's related to the clang 10 release?

It's not related to clang 10 release. I've added you because earlier you've found a problem with a previous approach https://github.com/llvm/llvm-project/commit/4c539e8da1b3de38a53ef3f7497f5c45a3243b61 So in case it breaks something else, you have extra visibility into the change.

Thanks! I applied the patch and was able to build large parts of Chromium without any problems.

[...snip...]
Thanks! I applied the patch and was able to build large parts of Chromium without any problems.

Thanks for checking!

vsapsai updated this revision to Diff 246005.Feb 21 2020, 3:06 PM
  • Fix some clang-format checks. Ignore the check to add a space between a generic name and its type parameters as it's not the style the rest of the header uses and not what Apple SDK headers are using.
erik.pilkington accepted this revision.Apr 2 2020, 11:54 AM

LGTM, sorry for the delay in reviewing this.

This revision is now accepted and ready to land.Apr 2 2020, 11:54 AM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.