[clang] a8c8b62 - [ObjC generics] Fix not inheriting type bounds in categories/extensions.
Mikael Holmén via cfe-commits
cfe-commits at lists.llvm.org
Mon Apr 6 23:28:49 PDT 2020
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