[ObjC] Add compatibility mode for type checking of qualified id block parameters.

[vsapsai]

Summary

Commit 73152a2ec20766ac45673a129bf1f5fc97ca9bbe fixed type checking for
blocks with qualified id parameters. But there are existing APIs in
Apple SDKs relying on the old type checking behavior. Specifically,
these are APIs using NSItemProviderCompletionHandler in
Foundation/NSItemProvider.h. To keep existing code working and to allow
developers to use affected APIs introduce a compatibility mode that
enables the previous type checking. This mode is enabled only on Darwin
platforms.

[jyknight]

clang/test/SemaObjC/block-type-safety.m

170

It seems a shame to disallow this assignment in the compatibility mode, when it ought to be allowed.

Perhaps it would be better to allow both directions for parameters when the compatibility flag is set? (That is: the compat mode flag would only cause the compiler to allow _more_ things than it otherwise should.)

[vsapsai]

clang/test/SemaObjC/block-type-safety.m

170

Hmm, sounds like an interesting idea. It can make the transitioning to the new compiler version less disruptive. Need to think more if it has any undesirable consequences. So far my only objection is that it makes things less consistent. But given the current state of block type checking that argument isn't particularly compelling.

[vsapsai]

Make compatibility mode accept correct types per James' Y Knight helpful suggestion.

[jyknight]

It looks like you didn't squash your two commits before uploading, so the diff for review now only includes the changes for the comment, not the complete patch. Other than needing to squash the commits, LGTM.

May want to wait for approval from one of the objc reviewers as well though.

[vsapsai]

Squash the commits, so that reviewers can review the entire change.

[ahatanak]

LGTM

[vsapsai]

Thanks everyone for reviews.

[steven_wu]

clang/lib/Driver/ToolChains/Darwin.cpp

2380

Not that I know there is a better place to put this option, this is not really a ClangTarget option.

Putting this in addClangTargetOptions will put this onto codegen flags for -fembed-bitcode, but this is just a frontend option that doesn't affect code generation.

clang/include/clang/Basic/LangOptions.def

 ENUM_LANGOPT(FPRoundingMode, RoundingMode, 3, RoundingMode::NearestTiesToEven, "FP Rounding Mode type")
 ENUM_LANGOPT(FPExceptionMode, FPExceptionModeKind, 2, FPE_Ignore, "FP Exception Behavior Mode type")
 LANGOPT(NoBitFieldTypeAlign , 1, 0, "bit-field type alignment")
 LANGOPT(HexagonQdsp6Compat , 1, 0, "hexagon-qdsp6 backward compatibility")
 LANGOPT(ObjCAutoRefCount , 1, 0, "Objective-C automated reference counting")
 LANGOPT(ObjCWeakRuntime     , 1, 0, "__weak support in the ARC runtime")
 LANGOPT(ObjCWeak            , 1, 0, "Objective-C __weak in ARC and MRC files")
 LANGOPT(ObjCSubscriptingLegacyRuntime         , 1, 0, "Subscripting support in legacy ObjectiveC runtime")
+BENIGN_LANGOPT(CompatibilityQualifiedIdBlockParamTypeChecking, 1, 0,
+               "compatibility mode for type checking block parameters "
+               "involving qualified id types")
 LANGOPT(CFProtectionBranch , 1, 0, "Control-Flow Branch Protection enabled")
 LANGOPT(FakeAddressSpaceMap , 1, 0, "OpenCL fake address space map")
 ENUM_LANGOPT(AddressSpaceMapMangling , AddrSpaceMapMangling, 2, ASMM_Target, "OpenCL address space map mangling mode")
 LANGOPT(IncludeDefaultHeader, 1, 0, "Include default header file for OpenCL")
 LANGOPT(DeclareOpenCLBuiltins, 1, 0, "Declare OpenCL builtin functions")
 BENIGN_LANGOPT(DelayedTemplateParsing , 1, 0, "delayed template parsing")
 LANGOPT(BlocksRuntimeOptional , 1, 0, "optional blocks runtime")
 LANGOPT(

clang/include/clang/Driver/CC1Options.td

 def fpreserve_vec3_type : Flag<["-"], "fpreserve-vec3-type">,
   HelpText<"Preserve 3-component vector type">;
 def fwchar_type_EQ : Joined<["-"], "fwchar-type=">,
   HelpText<"Select underlying type for wchar_t">, Values<"char,short,int">;
 def fsigned_wchar : Flag<["-"], "fsigned-wchar">,
   HelpText<"Use a signed type for wchar_t">;
 def fno_signed_wchar : Flag<["-"], "fno-signed-wchar">,
   HelpText<"Use an unsigned type for wchar_t">;
+def fcompatibility_qualified_id_block_param_type_checking : Flag<["-"], "fcompatibility-qualified-id-block-type-checking">,
+  HelpText<"Allow using blocks with parameters of more specific type than "
+           "the type system guarantees when a parameter is qualified id">;
 
 // FIXME: Remove these entirely once functionality/tests have been excised.
 def fobjc_gc_only : Flag<["-"], "fobjc-gc-only">, Group<f_Group>,
   HelpText<"Use GC exclusively for Objective-C related memory management">;
 def fobjc_gc : Flag<["-"], "fobjc-gc">, Group<f_Group>,
   HelpText<"Enable Objective-C garbage collection">;
 
 //===----------------------------------------------------------------------===//

clang/lib/AST/ASTContext.cpp

bool ASTContext::canAssignObjCInterfacesInBlockPointer(
   if (RHSOPT->isObjCBuiltinType() || LHSOPT->isObjCIdType())
     return true;
 
   if (LHSOPT->isObjCBuiltinType()) {
     return finish(RHSOPT->isObjCBuiltinType() ||
                   RHSOPT->isObjCQualifiedIdType());
   }
 
-  if (LHSOPT->isObjCQualifiedIdType() || RHSOPT->isObjCQualifiedIdType())
+  if (LHSOPT->isObjCQualifiedIdType() || RHSOPT->isObjCQualifiedIdType()) {
+    if (getLangOpts().CompatibilityQualifiedIdBlockParamTypeChecking)
+      return finish(ObjCQualifiedIdTypesAreCompatible(LHSOPT, RHSOPT, false));
+    else
       return finish(ObjCQualifiedIdTypesAreCompatible(
           (BlockReturnType ? LHSOPT : RHSOPT),
           (BlockReturnType ? RHSOPT : LHSOPT), false));
+  }
 
   const ObjCInterfaceType* LHS = LHSOPT->getInterfaceType();
   const ObjCInterfaceType* RHS = RHSOPT->getInterfaceType();
   if (LHS && RHS)  { // We have 2 user-defined types.
     if (LHS != RHS) {
       if (LHS->getDecl()->isSuperClassOf(RHS->getDecl()))
         return finish(BlockReturnType);
       if (RHS->getDecl()->isSuperClassOf(LHS->getDecl()))

clang/lib/Driver/ToolChains/Darwin.cpp

void Darwin::addClangTargetOptions(const llvm::opt::ArgList &DriverArgs,
   if (SDKInfo) {
     /// Pass the SDK version to the compiler when the SDK information is
     /// available.
     std::string Arg;
     llvm::raw_string_ostream OS(Arg);
     OS << "-target-sdk-version=" << SDKInfo->getVersion();
     CC1Args.push_back(DriverArgs.MakeArgString(OS.str()));
   }
+
+  // Enable compatibility mode for NSItemProviderCompletionHandler in
+  // Foundation/NSItemProvider.h.
+  CC1Args.push_back("-fcompatibility-qualified-id-block-type-checking");
 }
 
 DerivedArgList *
 Darwin::TranslateArgs(const DerivedArgList &Args, StringRef BoundArch,
                       Action::OffloadKind DeviceOffloadKind) const {
   // First get the generic Apple args, before moving onto Darwin-specific ones.
   DerivedArgList *DAL =
       MachO::TranslateArgs(Args, BoundArch, DeviceOffloadKind);

clang/lib/Frontend/CompilerInvocation.cpp

if (Arg *A = Args.getLastArg(OPT_msign_return_address_key_EQ)) {
         else
           Diags.Report(diag::err_drv_invalid_value)
               << A->getAsString(Args) << SignKey;
       }
     }
   }
 
   Opts.BranchTargetEnforcement = Args.hasArg(OPT_mbranch_target_enforce);
+
+  Opts.CompatibilityQualifiedIdBlockParamTypeChecking =
+      Args.hasArg(OPT_fcompatibility_qualified_id_block_param_type_checking);
 }
 
 static bool isStrictlyPreprocessorAction(frontend::ActionKind Action) {
   switch (Action) {
   case frontend::ASTDeclList:
   case frontend::ASTDump:
   case frontend::ASTPrint:
   case frontend::ASTView:

clang/test/Driver/darwin-objc-options.m

 // RUN: touch %t.c
 // RUN: %clang -target x86_64-apple-darwin -x objective-c -S -### %t.c 2>&1 | FileCheck --check-prefix=F %s
 // RUN: %clang -target x86_64-apple-darwin -S -### %t.c 2>&1 | FileCheck --check-prefix=NO_F %s
 // F: -fobjc-runtime=
 // NO_F-NOT: -fobjc-runtime=
 
 // Don't crash with an unexpected target triple.
 // RUN: %clang -target i386-apple-ios7 -S -### %s
+
+// Add -fcompatibility-qualified-id-block-type-checking only on Darwin.
+// RUN: %clang -target x86_64-apple-darwin10 -### %s 2>&1 | FileCheck --check-prefix=DARWIN_COMPATIBILITY %s
+// RUN: %clang -target x86_64-linux-gnu -### %s 2>&1 | FileCheck --check-prefix=OTHER_COMPATIBILITY %s
+// DARWIN_COMPATIBILITY: -fcompatibility-qualified-id-block-type-checking
+// OTHER_COMPATIBILITY-NOT: -fcompatibility-qualified-id-block-type-checking

clang/test/SemaObjC/block-type-safety.m

 // RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class %s
+// RUN: %clang_cc1 -fsyntax-only -verify -fblocks -Wno-objc-root-class \
+// RUN:   -fcompatibility-qualified-id-block-type-checking -DCOMPATIBILITY_QUALIFIED_ID_TYPE_CHECKING=1 %s
 // test for block type safety.
 
 @interface Super  @end
 @interface Sub : Super @end
 
 void f2(void(^f)(Super *)) { // expected-note{{passing argument to parameter 'f' here}}
     Super *o;
     f(o);

 @protocol NSCopying @end
 
 @interface NSAllArray <NSCopying>
 @end
 
 @interface NSAllArray (FooConformance) <Foo>
 @end
 
+#ifndef COMPATIBILITY_QUALIFIED_ID_TYPE_CHECKING
 int test5() {
     // Returned value is used outside of a block, so error on changing
     // a return type to a more general than expected.
     NSAllArray *(^block)(id);
     id <Foo> (^genericBlock)(id);
     genericBlock = block;
     block = genericBlock; // expected-error {{incompatible block pointer types assigning to 'NSAllArray *(^)(id)' from 'id<Foo> (^)(id)'}}
 
     // A parameter is used inside a block, so error on changing a parameter type
     // to a more specific than an argument type it will be called with.
     // rdar://problem/52788423
     void (^blockWithParam)(NSAllArray *);
     void (^genericBlockWithParam)(id<Foo>);
     genericBlockWithParam = blockWithParam; // expected-error {{incompatible block pointer types assigning to 'void (^)(id<Foo>)' from 'void (^)(NSAllArray *)'}}
     blockWithParam = genericBlockWithParam;
     return 0;
 }
+#else
+// In Apple SDK APIs using NSItemProviderCompletionHandler require to work with
+// blocks that have parameters more specific than in method signatures. As
+// explained in non-compatibility test above, it is not safe in general. But
+// to keep existing code working we support a compatibility mode that uses
+// previous type checking.
+int test5() {
+    NSAllArray *(^block)(id);
+    id <Foo> (^genericBlock)(id);
+    genericBlock = block;
+    block = genericBlock; // expected-error {{incompatible block pointer types assigning to 'NSAllArray *(^)(id)' from 'id<Foo> (^)(id)'}}
+
+    void (^blockWithParam)(NSAllArray *);
+    void (^genericBlockWithParam)(id<Foo>);
+    genericBlockWithParam = blockWithParam;
+    blockWithParam = genericBlockWithParam; // expected-error {{incompatible block pointer types assigning to 'void (^)(NSAllArray *)' from 'void (^)(id<Foo>)'}}
+    return 0;
+}
+#endif
 
 // rdar://10798770
 typedef int NSInteger;
 
 typedef enum : NSInteger {NSOrderedAscending = -1L, NSOrderedSame, NSOrderedDescending} NSComparisonResult;
 
 typedef NSComparisonResult (^NSComparator)(id obj1, id obj2);