From c575231ea9dfccafa44d5c49ea05063fb3efb7fc Mon Sep 17 00:00:00 2001 From: Douglas Gregor Date: Tue, 7 Jul 2015 03:58:01 +0000 Subject: [PATCH] Improve the Objective-C common-type computation used by the ternary operator. The Objective-C common-type computation had a few problems that required a significant rework, including: - Quadradic behavior when finding the common base type; now it's linear. - Keeping around type arguments when computing the common type between a specialized and an unspecialized type - Introducing redundant protocol qualifiers. Part of rdar://problem/6294649. Also fixes rdar://problem/19572837 by addressing a longstanding bug in ASTContext::CollectInheritedProtocols(). git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@241544 91177308-0d34-0410-b5e6-96231b3b80d8 --- include/clang/AST/Type.h | 4 +- lib/AST/ASTContext.cpp | 248 ++++++++++++++------ lib/AST/DeclObjC.cpp | 2 +- lib/AST/Type.cpp | 9 + lib/Sema/SemaExpr.cpp | 10 +- test/SemaObjC/block-type-safety.m | 8 + test/SemaObjC/conditional-expr-8.m | 33 ++- test/SemaObjC/conditional-expr.m | 11 + test/SemaObjC/parameterized_classes_subst.m | 12 +- test/SemaObjC/protocol-warn.m | 2 +- 10 files changed, 250 insertions(+), 89 deletions(-) diff --git a/include/clang/AST/Type.h b/include/clang/AST/Type.h index 22805e17c8..3a360f772b 100644 --- a/include/clang/AST/Type.h +++ b/include/clang/AST/Type.h @@ -4772,9 +4772,7 @@ public: /// qualifiers on the interface are ignored. /// /// \return null if the base type for this pointer is 'id' or 'Class' - const ObjCInterfaceType *getInterfaceType() const { - return getObjectType()->getBaseType()->getAs(); - } + const ObjCInterfaceType *getInterfaceType() const; /// getInterfaceDecl - If this pointer points to an Objective \@interface /// type, gets the declaration for that interface. diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp index d04121ac2f..a92f0052f0 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -1917,11 +1917,7 @@ void ASTContext::CollectInheritedProtocols(const Decl *CDecl, // We can use protocol_iterator here instead of // all_referenced_protocol_iterator since we are walking all categories. for (auto *Proto : OI->all_referenced_protocols()) { - Protocols.insert(Proto->getCanonicalDecl()); - for (auto *P : Proto->protocols()) { - Protocols.insert(P->getCanonicalDecl()); - CollectInheritedProtocols(P, Protocols); - } + CollectInheritedProtocols(Proto, Protocols); } // Categories of this Interface. @@ -1935,16 +1931,16 @@ void ASTContext::CollectInheritedProtocols(const Decl *CDecl, } } else if (const ObjCCategoryDecl *OC = dyn_cast(CDecl)) { for (auto *Proto : OC->protocols()) { - Protocols.insert(Proto->getCanonicalDecl()); - for (const auto *P : Proto->protocols()) - CollectInheritedProtocols(P, Protocols); + CollectInheritedProtocols(Proto, Protocols); } } else if (const ObjCProtocolDecl *OP = dyn_cast(CDecl)) { - for (auto *Proto : OP->protocols()) { - Protocols.insert(Proto->getCanonicalDecl()); - for (const auto *P : Proto->protocols()) - CollectInheritedProtocols(P, Protocols); - } + // Insert the protocol. + if (!Protocols.insert( + const_cast(OP->getCanonicalDecl())).second) + return; + + for (auto *Proto : OP->protocols()) + CollectInheritedProtocols(Proto, Protocols); } } @@ -5663,13 +5659,8 @@ void ASTContext::getObjCEncodingForTypeImpl(QualType T, std::string& S, case Type::ObjCInterface: { // Ignore protocol qualifiers when mangling at this level. - T = T->castAs()->getBaseType(); - - // The assumption seems to be that this assert will succeed - // because nested levels will have filtered out 'id' and 'Class'. - const ObjCInterfaceType *OIT = T->castAs(); // @encode(class_name) - ObjCInterfaceDecl *OI = OIT->getDecl(); + ObjCInterfaceDecl *OI = T->castAs()->getInterface(); S += '{'; S += OI->getObjCRuntimeNameAsString(); S += '='; @@ -6836,88 +6827,199 @@ bool ASTContext::canAssignObjCInterfacesInBlockPointer( return false; } +/// Comparison routine for Objective-C protocols to be used with +/// llvm::array_pod_sort. +static int compareObjCProtocolsByName(ObjCProtocolDecl * const *lhs, + ObjCProtocolDecl * const *rhs) { + return (*lhs)->getName().compare((*rhs)->getName()); + +} + /// getIntersectionOfProtocols - This routine finds the intersection of set -/// of protocols inherited from two distinct objective-c pointer objects. +/// of protocols inherited from two distinct objective-c pointer objects with +/// the given common base. /// It is used to build composite qualifier list of the composite type of /// the conditional expression involving two objective-c pointer objects. static void getIntersectionOfProtocols(ASTContext &Context, + const ObjCInterfaceDecl *CommonBase, const ObjCObjectPointerType *LHSOPT, const ObjCObjectPointerType *RHSOPT, - SmallVectorImpl &IntersectionOfProtocols) { + SmallVectorImpl &IntersectionSet) { const ObjCObjectType* LHS = LHSOPT->getObjectType(); const ObjCObjectType* RHS = RHSOPT->getObjectType(); assert(LHS->getInterface() && "LHS must have an interface base"); assert(RHS->getInterface() && "RHS must have an interface base"); - - llvm::SmallPtrSet InheritedProtocolSet; - unsigned LHSNumProtocols = LHS->getNumProtocols(); - if (LHSNumProtocols > 0) - InheritedProtocolSet.insert(LHS->qual_begin(), LHS->qual_end()); - else { - llvm::SmallPtrSet LHSInheritedProtocols; - Context.CollectInheritedProtocols(LHS->getInterface(), - LHSInheritedProtocols); - InheritedProtocolSet.insert(LHSInheritedProtocols.begin(), - LHSInheritedProtocols.end()); + + // Add all of the protocols for the LHS. + llvm::SmallPtrSet LHSProtocolSet; + + // Start with the protocol qualifiers. + for (auto proto : LHS->quals()) { + Context.CollectInheritedProtocols(proto, LHSProtocolSet); } - - unsigned RHSNumProtocols = RHS->getNumProtocols(); - if (RHSNumProtocols > 0) { - ObjCProtocolDecl **RHSProtocols = - const_cast(RHS->qual_begin()); - for (unsigned i = 0; i < RHSNumProtocols; ++i) - if (InheritedProtocolSet.count(RHSProtocols[i])) - IntersectionOfProtocols.push_back(RHSProtocols[i]); - } else { - llvm::SmallPtrSet RHSInheritedProtocols; - Context.CollectInheritedProtocols(RHS->getInterface(), - RHSInheritedProtocols); - for (ObjCProtocolDecl *ProtDecl : RHSInheritedProtocols) - if (InheritedProtocolSet.count(ProtDecl)) - IntersectionOfProtocols.push_back(ProtDecl); + + // Also add the protocols associated with the LHS interface. + Context.CollectInheritedProtocols(LHS->getInterface(), LHSProtocolSet); + + // Add all of the protocls for the RHS. + llvm::SmallPtrSet RHSProtocolSet; + + // Start with the protocol qualifiers. + for (auto proto : RHS->quals()) { + Context.CollectInheritedProtocols(proto, RHSProtocolSet); + } + + // Also add the protocols associated with the RHS interface. + Context.CollectInheritedProtocols(RHS->getInterface(), RHSProtocolSet); + + // Compute the intersection of the collected protocol sets. + for (auto proto : LHSProtocolSet) { + if (RHSProtocolSet.count(proto)) + IntersectionSet.push_back(proto); } + + // Compute the set of protocols that is implied by either the common type or + // the protocols within the intersection. + llvm::SmallPtrSet ImpliedProtocols; + Context.CollectInheritedProtocols(CommonBase, ImpliedProtocols); + + // Remove any implied protocols from the list of inherited protocols. + if (!ImpliedProtocols.empty()) { + IntersectionSet.erase( + std::remove_if(IntersectionSet.begin(), + IntersectionSet.end(), + [&](ObjCProtocolDecl *proto) -> bool { + return ImpliedProtocols.count(proto) > 0; + }), + IntersectionSet.end()); + } + + // Sort the remaining protocols by name. + llvm::array_pod_sort(IntersectionSet.begin(), IntersectionSet.end(), + compareObjCProtocolsByName); +} + +// Check that the given Objective-C type argument lists are equivalent. +static bool sameObjCTypeArgs(const ASTContext &ctx, ArrayRef lhsArgs, + ArrayRef rhsArgs) { + if (lhsArgs.size() != rhsArgs.size()) + return false; + + for (unsigned i = 0, n = lhsArgs.size(); i != n; ++i) { + if (!ctx.hasSameType(lhsArgs[i], rhsArgs[i])) + return false; + } + + return true; } -/// areCommonBaseCompatible - Returns common base class of the two classes if -/// one found. Note that this is O'2 algorithm. But it will be called as the -/// last type comparison in a ?-exp of ObjC pointer types before a -/// warning is issued. So, its invokation is extremely rare. QualType ASTContext::areCommonBaseCompatible( - const ObjCObjectPointerType *Lptr, - const ObjCObjectPointerType *Rptr) { + const ObjCObjectPointerType *Lptr, + const ObjCObjectPointerType *Rptr) { const ObjCObjectType *LHS = Lptr->getObjectType(); const ObjCObjectType *RHS = Rptr->getObjectType(); const ObjCInterfaceDecl* LDecl = LHS->getInterface(); const ObjCInterfaceDecl* RDecl = RHS->getInterface(); - if (!LDecl || !RDecl || (declaresSameEntity(LDecl, RDecl))) + + if (!LDecl || !RDecl) return QualType(); - while (!declaresSameEntity(LHS->getInterface(), RDecl)) { - // Strip protocols from the left-hand side. - if (LHS->getNumProtocols() > 0) - LHS = getObjCObjectType(LHS->getBaseType(), LHS->getTypeArgsAsWritten(), - { })->castAs(); + // Follow the left-hand side up the class hierarchy until we either hit a + // root or find the RHS. Record the ancestors in case we don't find it. + llvm::SmallDenseMap + LHSAncestors; + while (true) { + // Record this ancestor. We'll need this if the common type isn't in the + // path from the LHS to the root. + LHSAncestors[LHS->getInterface()->getCanonicalDecl()] = LHS; + + if (declaresSameEntity(LHS->getInterface(), RDecl)) { + // Get the type arguments. + ArrayRef LHSTypeArgs = LHS->getTypeArgsAsWritten(); + bool anyChanges = false; + if (LHS->isSpecialized() && RHS->isSpecialized()) { + // Both have type arguments, compare them. + if (!sameObjCTypeArgs(*this, LHS->getTypeArgs(), RHS->getTypeArgs())) + return QualType(); + } else if (LHS->isSpecialized() != RHS->isSpecialized()) { + // If only one has type arguments, the result will not have type + // arguments. + LHSTypeArgs = { }; + anyChanges = true; + } - if (canAssignObjCInterfaces(LHS, RHS)) { + // Compute the intersection of protocols. SmallVector Protocols; - getIntersectionOfProtocols(*this, Lptr, Rptr, Protocols); - - QualType Result = QualType(LHS, 0); + getIntersectionOfProtocols(*this, LHS->getInterface(), Lptr, Rptr, + Protocols); if (!Protocols.empty()) - Result = getObjCObjectType(Result, Protocols.data(), Protocols.size()); - Result = getObjCObjectPointerType(Result); - return Result; + anyChanges = true; + + // If anything in the LHS will have changed, build a new result type. + if (anyChanges) { + QualType Result = getObjCInterfaceType(LHS->getInterface()); + Result = getObjCObjectType(Result, LHSTypeArgs, Protocols); + return getObjCObjectPointerType(Result); + } + + return getObjCObjectPointerType(QualType(LHS, 0)); } + // Find the superclass. QualType LHSSuperType = LHS->getSuperClassType(); if (LHSSuperType.isNull()) break; LHS = LHSSuperType->castAs(); } - + + // We didn't find anything by following the LHS to its root; now check + // the RHS against the cached set of ancestors. + while (true) { + auto KnownLHS = LHSAncestors.find(RHS->getInterface()->getCanonicalDecl()); + if (KnownLHS != LHSAncestors.end()) { + LHS = KnownLHS->second; + + // Get the type arguments. + ArrayRef RHSTypeArgs = RHS->getTypeArgsAsWritten(); + bool anyChanges = false; + if (LHS->isSpecialized() && RHS->isSpecialized()) { + // Both have type arguments, compare them. + if (!sameObjCTypeArgs(*this, LHS->getTypeArgs(), RHS->getTypeArgs())) + return QualType(); + } else if (LHS->isSpecialized() != RHS->isSpecialized()) { + // If only one has type arguments, the result will not have type + // arguments. + RHSTypeArgs = { }; + anyChanges = true; + } + + // Compute the intersection of protocols. + SmallVector Protocols; + getIntersectionOfProtocols(*this, RHS->getInterface(), Lptr, Rptr, + Protocols); + if (!Protocols.empty()) + anyChanges = true; + + if (anyChanges) { + QualType Result = getObjCInterfaceType(RHS->getInterface()); + Result = getObjCObjectType(Result, RHSTypeArgs, Protocols); + return getObjCObjectPointerType(Result); + } + + return getObjCObjectPointerType(QualType(RHS, 0)); + } + + // Find the superclass of the RHS. + QualType RHSSuperType = RHS->getSuperClassType(); + if (RHSSuperType.isNull()) + break; + + RHS = RHSSuperType->castAs(); + } + return QualType(); } @@ -6946,7 +7048,7 @@ bool ASTContext::canAssignObjCInterfaces(const ObjCObjectType *LHS, // Also, if RHS has explicit quelifiers, include them for comparing with LHS's // qualifiers. for (auto *RHSPI : RHS->quals()) - SuperClassInheritedProtocols.insert(RHSPI->getCanonicalDecl()); + CollectInheritedProtocols(RHSPI, SuperClassInheritedProtocols); // If there is no protocols associated with RHS, it is not a match. if (SuperClassInheritedProtocols.empty()) return false; @@ -6972,13 +7074,9 @@ bool ASTContext::canAssignObjCInterfaces(const ObjCObjectType *LHS, RHSSuper = RHSSuper->getSuperClassType()->castAs(); // If the RHS is specializd, compare type arguments. - if (RHSSuper->isSpecialized()) { - ArrayRef LHSTypeArgs = LHS->getTypeArgs(); - ArrayRef RHSTypeArgs = RHSSuper->getTypeArgs(); - for (unsigned i = 0, n = LHSTypeArgs.size(); i != n; ++i) { - if (!hasSameType(LHSTypeArgs[i], RHSTypeArgs[i])) - return false; - } + if (RHSSuper->isSpecialized() && + !sameObjCTypeArgs(*this, LHS->getTypeArgs(), RHSSuper->getTypeArgs())) { + return false; } } diff --git a/lib/AST/DeclObjC.cpp b/lib/AST/DeclObjC.cpp index 69ff33a60a..714824b02a 100644 --- a/lib/AST/DeclObjC.cpp +++ b/lib/AST/DeclObjC.cpp @@ -251,7 +251,7 @@ ObjCTypeParamList *ObjCInterfaceDecl::getTypeParamList() const { // Otherwise, look at previous declarations to determine whether any // of them has a type parameter list, skipping over those // declarations that do not. - for (auto decl = getPreviousDecl(); decl; decl = decl->getPreviousDecl()) { + for (auto decl = getMostRecentDecl(); decl; decl = decl->getPreviousDecl()) { if (ObjCTypeParamList *written = decl->getTypeParamListAsWritten()) return written; } diff --git a/lib/AST/Type.cpp b/lib/AST/Type.cpp index 9b2bfc0cd9..106d42071c 100644 --- a/lib/AST/Type.cpp +++ b/lib/AST/Type.cpp @@ -1260,6 +1260,15 @@ void ObjCObjectType::computeSuperClassTypeSlow() const { true); } +const ObjCInterfaceType *ObjCObjectPointerType::getInterfaceType() const { + if (auto interfaceDecl = getObjectType()->getInterface()) { + return interfaceDecl->getASTContext().getObjCInterfaceType(interfaceDecl) + ->castAs(); + } + + return nullptr; +} + QualType ObjCObjectPointerType::getSuperClassType() const { QualType superObjectType = getObjectType()->getSuperClassType(); if (superObjectType.isNull()) diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp index afc9279cba..a4911c20c6 100644 --- a/lib/Sema/SemaExpr.cpp +++ b/lib/Sema/SemaExpr.cpp @@ -6273,7 +6273,10 @@ QualType Sema::FindCompositeObjCPointerType(ExprResult &LHS, ExprResult &RHS, // FIXME: Consider unifying with 'areComparableObjCPointerTypes'. // It could return the composite type. - if (Context.canAssignObjCInterfaces(LHSOPT, RHSOPT)) { + if (!(compositeType = + Context.areCommonBaseCompatible(LHSOPT, RHSOPT)).isNull()) { + // Nothing more to do. + } else if (Context.canAssignObjCInterfaces(LHSOPT, RHSOPT)) { compositeType = RHSOPT->isObjCBuiltinType() ? RHSTy : LHSTy; } else if (Context.canAssignObjCInterfaces(RHSOPT, LHSOPT)) { compositeType = LHSOPT->isObjCBuiltinType() ? LHSTy : RHSTy; @@ -6287,10 +6290,7 @@ QualType Sema::FindCompositeObjCPointerType(ExprResult &LHS, ExprResult &RHS, compositeType = Context.getObjCIdType(); } else if (LHSTy->isObjCIdType() || RHSTy->isObjCIdType()) { compositeType = Context.getObjCIdType(); - } else if (!(compositeType = - Context.areCommonBaseCompatible(LHSOPT, RHSOPT)).isNull()) - ; - else { + } else { Diag(QuestionLoc, diag::ext_typecheck_cond_incompatible_operands) << LHSTy << RHSTy << LHS.get()->getSourceRange() << RHS.get()->getSourceRange(); diff --git a/test/SemaObjC/block-type-safety.m b/test/SemaObjC/block-type-safety.m index b2c4398dc3..96c781b6a5 100644 --- a/test/SemaObjC/block-type-safety.m +++ b/test/SemaObjC/block-type-safety.m @@ -198,3 +198,11 @@ void Test3() { NSObject *NSO5 = aBlock; // expected-error {{initializing 'NSObject *' with an expression of incompatible type 'void (^)()'}} NSObject *NSO6 = aBlock; // Ok } + +// rdar://problem/19420731 +typedef NSObject NSObject_P1; +typedef NSObject_P1 NSObject_P1_P2; + +void Test4(void (^handler)(NSObject_P1_P2 *p)) { + Test4(^(NSObject *p) { }); +} diff --git a/test/SemaObjC/conditional-expr-8.m b/test/SemaObjC/conditional-expr-8.m index beddd205a9..35f4e75314 100644 --- a/test/SemaObjC/conditional-expr-8.m +++ b/test/SemaObjC/conditional-expr-8.m @@ -1,7 +1,7 @@ // RUN: %clang_cc1 -fsyntax-only -verify %s // expected-no-diagnostics -// rdar://9296866 +// rdar://9296866 @interface NSResponder @end @@ -24,3 +24,34 @@ } @end +// rdar://problem/19572837 +@protocol NSObject +@end + +__attribute__((objc_root_class)) +@interface NSObject +@end + +@protocol Goable +- (void)go; +@end + +@protocol Drivable +- (void)drive; +@end + +@interface Car : NSObject +- (NSObject *)bestGoable:(NSObject *)drivable; +@end + +@interface Car(Category) +@end + +@interface Truck : Car +@end + +@implementation Truck +- (NSObject *)bestGoable:(NSObject *)drivable value:(int)value{ + return value > 0 ? self : drivable; +} +@end diff --git a/test/SemaObjC/conditional-expr.m b/test/SemaObjC/conditional-expr.m index 71e108cce6..71bdb1b2d3 100644 --- a/test/SemaObjC/conditional-expr.m +++ b/test/SemaObjC/conditional-expr.m @@ -51,6 +51,10 @@ @end @protocol P2 @end +@protocol P3 +@end +@protocol P4 +@end @interface A @end @@ -64,6 +68,9 @@ @interface D @end +@interface E : A +@end + void f0(id x) { x.intProp = 1; } @@ -118,3 +125,7 @@ void f11(int a, id x, id y) { void f12(int a, A *x, A *y) { A* l0 = (a ? x : y ); // expected-warning {{incompatible pointer types initializing 'A *' with an expression of type 'A *'}} } + +void f13(int a, B *x, E *y) { + int *ip = a ? x : y; // expected-warning{{expression of type 'A *'}} +} diff --git a/test/SemaObjC/parameterized_classes_subst.m b/test/SemaObjC/parameterized_classes_subst.m index 186800cddd..979053bfa7 100644 --- a/test/SemaObjC/parameterized_classes_subst.m +++ b/test/SemaObjC/parameterized_classes_subst.m @@ -344,10 +344,16 @@ void test_ternary_operator(NSArray *stringArray, ip = cond ? stringArray : mutStringArray; // expected-warning{{from 'NSArray *'}} ip = cond ? mutStringArray : stringArray; // expected-warning{{from 'NSArray *'}} - ip = cond ? stringArray2 : mutStringArray; // expected-warning{{from 'NSArray *'}} - ip = cond ? mutStringArray : stringArray2; // expected-warning{{from 'NSArray *'}} + ip = cond ? stringArray2 : mutStringArray; // expected-warning{{from 'NSArray *'}} + ip = cond ? mutStringArray : stringArray2; // expected-warning{{from 'NSArray *'}} - ip = cond ? stringArray : mutArray; // FIXME: expected-warning{{from 'NSArray *'}} + ip = cond ? stringArray : mutArray; // expected-warning{{from 'NSArray *'}} + + ip = cond ? stringArray2 : mutArray; // expected-warning{{from 'NSArray *'}} + + ip = cond ? mutArray : stringArray; // expected-warning{{from 'NSArray *'}} + + ip = cond ? mutArray : stringArray2; // expected-warning{{from 'NSArray *'}} object = cond ? stringArray : numberArray; // expected-warning{{incompatible operand types ('NSArray *' and 'NSArray *')}} } diff --git a/test/SemaObjC/protocol-warn.m b/test/SemaObjC/protocol-warn.m index 2d04238058..04df5031ab 100644 --- a/test/SemaObjC/protocol-warn.m +++ b/test/SemaObjC/protocol-warn.m @@ -51,5 +51,5 @@ UIWebPDFView *getView() { UIWebBrowserView *browserView; UIWebPDFView *pdfView; - return pdfView ? pdfView : browserView; // expected-warning {{incompatible pointer types returning 'UIView *' from a function with result type 'UIWebPDFView *'}} + return pdfView ? pdfView : browserView; // expected-warning {{incompatible pointer types returning 'UIView *' from a function with result type 'UIWebPDFView *'}} } -- 2.40.0