[clang] a8c8b62 - [ObjC generics] Fix not inheriting type bounds in categories/extensions.
Volodymyr Sapsai via cfe-commits
cfe-commits at lists.llvm.org
Tue Apr 7 17:44:52 PDT 2020
Can reproduce test failure locally, reverted in 8fb7cfcea97af440830d256cc18ccd978f218e1d
Thanks for noticing the problem and pointing me to it.
> On Apr 6, 2020, at 23:28, Mikael Holmén <mikael.holmen at ericsson.com> wrote:
>
> We see flakiness in the test in our bots too. Fails one time and then
> passes again.
>
> /Mikael
>
> On Mon, 2020-04-06 at 21:03 -0400, Nico Weber via cfe-commits wrote:
>> This isn't bot-dependent, it's been flaking on many different bots
>> over the last few days. Here's one from just now:
>> http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/34705
>>
>> On Mon, Apr 6, 2020 at 4:40 PM Volodymyr Sapsai <vsapsai at apple.com>
>> wrote:
>>> Is there anything special in the builedbot configuration? Are you
>>> still observing intermittent failures?
>>>
>>> I’m double checking if we see similar failures internally but so
>>> far looks like everything is working fine. I’ve only seen a single
>>> failure
>>> http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-bootstrap-ubsan/builds/18616
>>>
>>> At the moment I don’t know what might be causing the non-
>>> determinism. The type checking depends on the parsing order. We
>>> rely that
>>>
>>> @interface ParameterizedContainer<T> (Cat)
>>>
>>> is parsed before
>>>
>>> - (ParameterizedContainer<T> *)inCategory;
>>>
>>> So we check type parameter T is consistent with original @interface
>>> before using it. What is also confusing is that there is only a
>>> single error. I expect both a category and an extension to have
>>> same errors.
>>>
>>>> On Apr 5, 2020, at 10:10, Nico Weber <thakis at chromium.org> wrote:
>>>>
>>>> The test here flakily fails, maybe 1 in 10 times:
>>>> http://45.33.8.238/mac/11180/step_7.txt
>>>>
>>>> error: 'error' diagnostics seen but not expected:
>>>> File /Users/thakis/src/llvm-
>>>> project/clang/test/SemaObjC/parameterized_classes_subst.m Line
>>>> 479: type argument 'T' (aka 'id') does not satisfy the bound
>>>> ('id<NSCopying>') of type parameter 'T'
>>>> error: 'note' diagnostics seen but not expected:
>>>> File /Users/thakis/src/llvm-
>>>> project/clang/test/SemaObjC/parameterized_classes_subst.m Line
>>>> 475: type parameter 'T' declared here
>>>> 2 errors generated.
>>>>
>>>> Maybe if this is emitted depends on the order of something in a
>>>> data structure that has no deterministic order, or similar?
>>>>
>>>> On Fri, Apr 3, 2020 at 7:29 PM Volodymyr Sapsai via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>>> Author: Volodymyr Sapsai
>>>>> Date: 2020-04-03T16:29:02-07:00
>>>>> New Revision: a8c8b627f23f204fb621bd2a8c495cfc8bc16ae7
>>>>>
>>>>> URL:
>>>>> https://github.com/llvm/llvm-project/commit/a8c8b627f23f204fb621bd2a8c495cfc8bc16ae7
>>>>> DIFF:
>>>>> https://github.com/llvm/llvm-project/commit/a8c8b627f23f204fb621bd2a8c495cfc8bc16ae7.diff
>>>>>
>>>>> LOG: [ObjC generics] Fix not inheriting type bounds in
>>>>> categories/extensions.
>>>>>
>>>>> 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
>>>>>
>>>>> Reviewed By: erik.pilkington
>>>>>
>>>>> Differential Revision: https://reviews.llvm.org/D72872
>>>>>
>>>>> Added:
>>>>>
>>>>>
>>>>> Modified:
>>>>> clang/include/clang/AST/ASTContext.h
>>>>> clang/lib/AST/ASTContext.cpp
>>>>> clang/lib/AST/Type.cpp
>>>>> clang/lib/Sema/SemaDeclObjC.cpp
>>>>>
>>>>> clang/test/SemaObjC/parameterized_classes_collection_literal.m
>>>>> clang/test/SemaObjC/parameterized_classes_subst.m
>>>>>
>>>>> Removed:
>>>>>
>>>>>
>>>>>
>>>>> ###############################################################
>>>>> #################
>>>>> diff --git a/clang/include/clang/AST/ASTContext.h
>>>>> b/clang/include/clang/AST/ASTContext.h
>>>>> index 6813ab58874e..6360f18217c7 100644
>>>>> --- a/clang/include/clang/AST/ASTContext.h
>>>>> +++ b/clang/include/clang/AST/ASTContext.h
>>>>> @@ -1442,6 +1442,8 @@ class ASTContext : public
>>>>> RefCountedBase<ASTContext> {
>>>>>
>>>>> QualType getObjCTypeParamType(const ObjCTypeParamDecl *Decl,
>>>>> ArrayRef<ObjCProtocolDecl *>
>>>>> protocols) const;
>>>>> + void adjustObjCTypeParamBoundType(const ObjCTypeParamDecl
>>>>> *Orig,
>>>>> + ObjCTypeParamDecl *New)
>>>>> const;
>>>>>
>>>>> bool ObjCObjectAdoptsQTypeProtocols(QualType QT,
>>>>> ObjCInterfaceDecl *Decl);
>>>>>
>>>>>
>>>>> diff --git a/clang/lib/AST/ASTContext.cpp
>>>>> b/clang/lib/AST/ASTContext.cpp
>>>>> index 1e81e0a67b4d..06dcb6fa0580 100644
>>>>> --- a/clang/lib/AST/ASTContext.cpp
>>>>> +++ b/clang/lib/AST/ASTContext.cpp
>>>>> @@ -4874,6 +4874,17 @@ ASTContext::getObjCTypeParamType(const
>>>>> ObjCTypeParamDecl *Decl,
>>>>> return QualType(newType, 0);
>>>>> }
>>>>>
>>>>> +void ASTContext::adjustObjCTypeParamBoundType(const
>>>>> ObjCTypeParamDecl *Orig,
>>>>> +
>>>>> ObjCTypeParamDecl *New) const {
>>>>> + New->setTypeSourceInfo(getTrivialTypeSourceInfo(Orig-
>>>>>> getUnderlyingType()));
>>>>> + // Update TypeForDecl after updating TypeSourceInfo.
>>>>> + auto NewTypeParamTy = cast<ObjCTypeParamType>(New-
>>>>>> getTypeForDecl());
>>>>> + SmallVector<ObjCProtocolDecl *, 8> protocols;
>>>>> + protocols.append(NewTypeParamTy->qual_begin(),
>>>>> NewTypeParamTy->qual_end());
>>>>> + QualType UpdatedTy = getObjCTypeParamType(New, protocols);
>>>>> + New->setTypeForDecl(UpdatedTy.getTypePtr());
>>>>> +}
>>>>> +
>>>>> /// ObjCObjectAdoptsQTypeProtocols - Checks that protocols in
>>>>> IC's
>>>>> /// protocol list adopt all protocols in QT's qualified-id
>>>>> protocol
>>>>> /// list.
>>>>>
>>>>> diff --git a/clang/lib/AST/Type.cpp b/clang/lib/AST/Type.cpp
>>>>> index 3428437c3146..7c65378261ad 100644
>>>>> --- a/clang/lib/AST/Type.cpp
>>>>> +++ b/clang/lib/AST/Type.cpp
>>>>> @@ -3534,6 +3534,7 @@ void
>>>>> ObjCTypeParamType::Profile(llvm::FoldingSetNodeID &ID,
>>>>> const ObjCTypeParamDecl
>>>>> *OTPDecl,
>>>>> ArrayRef<ObjCProtocolDecl *>
>>>>> protocols) {
>>>>> ID.AddPointer(OTPDecl);
>>>>> + ID.AddPointer(OTPDecl-
>>>>>> getUnderlyingType().getAsOpaquePtr());
>>>>> ID.AddInteger(protocols.size());
>>>>> for (auto proto : protocols)
>>>>> ID.AddPointer(proto);
>>>>>
>>>>> diff --git a/clang/lib/Sema/SemaDeclObjC.cpp
>>>>> b/clang/lib/Sema/SemaDeclObjC.cpp
>>>>> index 934e1a23141c..6db57898e378 100644
>>>>> --- a/clang/lib/Sema/SemaDeclObjC.cpp
>>>>> +++ b/clang/lib/Sema/SemaDeclObjC.cpp
>>>>> @@ -938,8 +938,7 @@ static bool
>>>>> checkTypeParamListConsistency(Sema &S,
>>>>>
>>>>> // Override the new type parameter's bound type with the
>>>>> previous type,
>>>>> // so that it's consistent.
>>>>> - newTypeParam->setTypeSourceInfo(
>>>>> - S.Context.getTrivialTypeSourceInfo(prevTypeParam-
>>>>>> getUnderlyingType()));
>>>>> + S.Context.adjustObjCTypeParamBoundType(prevTypeParam,
>>>>> newTypeParam);
>>>>> continue;
>>>>> }
>>>>>
>>>>> @@ -966,8 +965,7 @@ static bool
>>>>> checkTypeParamListConsistency(Sema &S,
>>>>> }
>>>>>
>>>>> // Update the new type parameter's bound to match the
>>>>> previous one.
>>>>> - newTypeParam->setTypeSourceInfo(
>>>>> - S.Context.getTrivialTypeSourceInfo(prevTypeParam-
>>>>>> getUnderlyingType()));
>>>>> + S.Context.adjustObjCTypeParamBoundType(prevTypeParam,
>>>>> newTypeParam);
>>>>> }
>>>>>
>>>>> return false;
>>>>>
>>>>> diff --git
>>>>> a/clang/test/SemaObjC/parameterized_classes_collection_literal.
>>>>> m
>>>>> b/clang/test/SemaObjC/parameterized_classes_collection_literal.
>>>>> m
>>>>> index 472746e09db9..034d2e8da217 100644
>>>>> ---
>>>>> a/clang/test/SemaObjC/parameterized_classes_collection_literal.
>>>>> m
>>>>> +++
>>>>> b/clang/test/SemaObjC/parameterized_classes_collection_literal.
>>>>> m
>>>>> @@ -29,7 +29,9 @@ + (instancetype)arrayWithObjects:(const T
>>>>> [])objects count:(NSUInteger)cnt;
>>>>> @end
>>>>>
>>>>> @interface NSDictionary<K, V> : NSObject <NSCopying>
>>>>> -+ (instancetype)dictionaryWithObjects:(const V [])objects
>>>>> forKeys:(const K [])keys count:(NSUInteger)cnt;
>>>>> ++ (instancetype)dictionaryWithObjects:(const V [])objects
>>>>> + forKeys:(const K <NSCopying>
>>>>> [])keys
>>>>> + count:(NSUInteger)cnt;
>>>>> @end
>>>>>
>>>>> void testArrayLiteral(void) {
>>>>> @@ -50,3 +52,9 @@ void testDictionaryLiteral(void) {
>>>>> @"world" : @"blah" // expected-warning{{object of type
>>>>> 'NSString *' is not compatible with dictionary value type
>>>>> 'NSNumber *'}}
>>>>> };
>>>>> }
>>>>> +
>>>>> +void testCastingInDictionaryLiteral(NSString *arg) {
>>>>> + NSDictionary *dict = @{
>>>>> + (id)arg : @"foo",
>>>>> + };
>>>>> +}
>>>>>
>>>>> diff --git a/clang/test/SemaObjC/parameterized_classes_subst.m
>>>>> b/clang/test/SemaObjC/parameterized_classes_subst.m
>>>>> index d14a6e9deb40..b6d884760d29 100644
>>>>> --- a/clang/test/SemaObjC/parameterized_classes_subst.m
>>>>> +++ b/clang/test/SemaObjC/parameterized_classes_subst.m
>>>>> @@ -467,3 +467,17 @@ - (void)mapUsingBlock:(id (^)(id))block {
>>>>> - (void)mapUsingBlock2:(id)block { // expected-
>>>>> warning{{conflicting parameter types in implementation}}
>>>>> }
>>>>> @end
>>>>> +
>>>>> +// ---------------------------------------------------------
>>>>> -----------------
>>>>> +// Use a type parameter as a type argument.
>>>>> +// ---------------------------------------------------------
>>>>> -----------------
>>>>> +// Type bounds in a category/extension are omitted.
>>>>> rdar://problem/54329242
>>>>> + at interface ParameterizedContainer<T : id<NSCopying>>
>>>>> +- (ParameterizedContainer<T> *)inInterface;
>>>>> + at end
>>>>> + at interface ParameterizedContainer<T> (Cat)
>>>>> +- (ParameterizedContainer<T> *)inCategory;
>>>>> + at end
>>>>> + at interface ParameterizedContainer<U> ()
>>>>> +- (ParameterizedContainer<U> *)inExtension;
>>>>> + at end
>>>>>
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list