Provide fix-its when implementing a protocol method with bridged types. (#2799)

This is the protocol version of 7bfdd4a2: if a protocol requirement
has a newly-bridged type (say, 'URL' instead of 'NSURL'), then any
conforming types will need to update their implementations of the
requirement. Reuse the override-checking mechanism to do so when
we're reasonably confident about it.

This slots the checking into the existing protocol diagnostics, which
doesn't result in the best user experience.

    note: protocol requires property 'prop' with type 'Refrigerator?'
      var prop: Refrigerator? { get }
          ^
    note: candidate has non-matching type 'APPRefrigerator?'
      var prop: APPRefrigerator? {
          ^     ~~~~~~~~~~~~~~~~
                Refrigerator?

But we can come back and improve that later; right now this is better
than nothing.

rdar://problem/26237030

lib/Sema/MiscDiagnostics.cpp

@@ -997,6 +997,115 @@ bool swift::diagnoseArgumentLabelError(TypeChecker &TC, const Expr *expr,
   return true;
 }
 
+bool swift::fixItOverrideDeclarationTypes(TypeChecker &TC,
+                                          InFlightDiagnostic &diag,
+                                          ValueDecl *decl,
+                                          const ValueDecl *base) {
+  // For now, just rewrite cases where the base uses a value type and the
+  // override uses a reference type, and the value type is bridged to the
+  // reference type. This is a way to migrate code that makes use of types
+  // that previously were not bridged to value types.
+  auto checkType = [&](Type overrideTy, Type baseTy,
+                       SourceRange typeRange) -> bool {
+    if (typeRange.isInvalid())
+      return false;
+    if (!baseTy) {
+      decl->dump();
+      base->dump();
+    }
+
+    auto normalizeType = [](Type ty) -> Type {
+      ty = ty->getInOutObjectType();
+      if (Type unwrappedTy = ty->getAnyOptionalObjectType())
+        ty = unwrappedTy;
+      return ty;
+    };
+
+    // Is the base type bridged?
+    Type normalizedBaseTy = normalizeType(baseTy);
+    const DeclContext *DC = decl->getDeclContext();
+    Optional<Type> maybeBridged =
+        TC.Context.getBridgedToObjC(DC, normalizedBaseTy, &TC);
+
+    // ...and just knowing that it's bridged isn't good enough if we don't
+    // know what it's bridged /to/. Also, don't do this check for trivial
+    // bridging---that doesn't count.
+    Type bridged = maybeBridged.getValueOr(Type());
+    if (!bridged || bridged->isEqual(normalizedBaseTy))
+      return false;
+
+    // ...and is it bridged to the overridden type?
+    Type normalizedOverrideTy = normalizeType(overrideTy);
+    if (!bridged->isEqual(normalizedOverrideTy)) {
+      // If both are nominal types, check again, ignoring generic arguments.
+      auto *overrideNominal = normalizedOverrideTy->getAnyNominal();
+      if (!overrideNominal || bridged->getAnyNominal() != overrideNominal) {
+        return false;
+      }
+    }
+
+    Type newOverrideTy = baseTy;
+
+    // Preserve optionality if we're dealing with a simple type.
+    OptionalTypeKind OTK;
+    if (Type unwrappedTy = newOverrideTy->getAnyOptionalObjectType())
+      newOverrideTy = unwrappedTy;
+    if (overrideTy->getAnyOptionalObjectType(OTK))
+      newOverrideTy = OptionalType::get(OTK, newOverrideTy);
+
+    SmallString<32> baseTypeBuf;
+    llvm::raw_svector_ostream baseTypeStr(baseTypeBuf);
+    PrintOptions options;
+    options.SynthesizeSugarOnTypes = true;
+
+    newOverrideTy->print(baseTypeStr, options);
+    diag.fixItReplace(typeRange, baseTypeStr.str());
+    return true;
+  };
+
+  if (auto *var = dyn_cast<VarDecl>(decl)) {
+    SourceRange typeRange = var->getTypeSourceRangeForDiagnostics();
+    return checkType(var->getType(), base->getType(), typeRange);
+  }
+
+  if (auto *fn = dyn_cast<AbstractFunctionDecl>(decl)) {
+    auto *baseFn = cast<AbstractFunctionDecl>(base);
+    bool fixedAny = false;
+    if (fn->getParameterLists().back()->size() ==
+        baseFn->getParameterLists().back()->size()) {
+      for_each(*fn->getParameterLists().back(),
+               *baseFn->getParameterLists().back(),
+               [&](ParamDecl *param, const ParamDecl *baseParam) {
+        fixedAny |= fixItOverrideDeclarationTypes(TC, diag, param, baseParam);
+      });
+    }
+    if (auto *method = dyn_cast<FuncDecl>(decl)) {
+      auto *baseMethod = cast<FuncDecl>(base);
+      fixedAny |= checkType(method->getBodyResultType(),
+                            baseMethod->getResultType(),
+                            method->getBodyResultTypeLoc().getSourceRange());
+    }
+    return fixedAny;
+  }
+
+  if (auto *subscript = dyn_cast<SubscriptDecl>(decl)) {
+    auto *baseSubscript = cast<SubscriptDecl>(base);
+    bool fixedAny = false;
+    for_each(*subscript->getIndices(),
+             *baseSubscript->getIndices(),
+             [&](ParamDecl *param, const ParamDecl *baseParam) {
+      fixedAny |= fixItOverrideDeclarationTypes(TC, diag, param, baseParam);
+    });
+    fixedAny |= checkType(subscript->getElementType(),
+                          baseSubscript->getElementType(),
+                          subscript->getElementTypeLoc().getSourceRange());
+    return fixedAny;
+  }
+
+  llvm_unreachable("unknown overridable member");
+}
+
+
 
 //===----------------------------------------------------------------------===//
 // Diagnose availability.

lib/Sema/MiscDiagnostics.h

@@ -68,6 +68,16 @@ void fixItAvailableAttrRename(TypeChecker &TC,
                               SourceRange referenceRange,
                               const AvailableAttr *attr,
                               const ApplyExpr *call);
+
+/// Attempt to fix the type of \p decl so that it's a valid override for
+/// \p base...but only if we're highly confident that we know what the user
+/// should have written.
+///
+/// \returns true iff any fix-its were attached to \p diag.
+bool fixItOverrideDeclarationTypes(TypeChecker &TC,
+                                   InFlightDiagnostic &diag,
+                                   ValueDecl *decl,
+                                   const ValueDecl *base);
 } // namespace swift
 
 #endif // SWIFT_SEMA_MISC_DIAGNOSTICS_H

lib/Sema/TypeCheckDecl.cpp

@@ -4783,115 +4783,6 @@ public:
       type = fnType->withExtInfo(extInfo);
   }
 
-  /// Attempt to fix the type of \p decl so that it's a valid override for
-  /// \p base...but only if we're highly confident that we know what the user
-  /// should have written.
-  ///
-  /// \returns true iff any fix-its were attached to \p diag.
-  static bool fixOverrideDeclarationTypes(InFlightDiagnostic &diag,
-                                          TypeChecker &TC,
-                                          ValueDecl *decl,
-                                          const ValueDecl *base) {
-    // For now, just rewrite cases where the base uses a value type and the
-    // override uses a reference type, and the value type is bridged to the
-    // reference type. This is a way to migrate code that makes use of types
-    // that previously were not bridged to value types.
-    auto checkType = [&](Type overrideTy, Type baseTy,
-                         SourceRange typeRange) -> bool {
-      if (typeRange.isInvalid())
-        return false;
-
-      auto normalizeType = [](Type ty) -> Type {
-        ty = ty->getInOutObjectType();
-        if (Type unwrappedTy = ty->getAnyOptionalObjectType())
-          ty = unwrappedTy;
-        return ty;
-      };
-
-      // Is the base type bridged?
-      Type normalizedBaseTy = normalizeType(baseTy);
-      const DeclContext *DC = decl->getDeclContext();
-      Optional<Type> maybeBridged =
-          TC.Context.getBridgedToObjC(DC, normalizedBaseTy, &TC);
-
-      // ...and just knowing that it's bridged isn't good enough if we don't
-      // know what it's bridged /to/. Also, don't do this check for trivial
-      // bridging---that doesn't count.
-      Type bridged = maybeBridged.getValueOr(Type());
-      if (!bridged || bridged->isEqual(normalizedBaseTy))
-        return false;
-
-      // ...and is it bridged to the overridden type?
-      Type normalizedOverrideTy = normalizeType(overrideTy);
-      if (!bridged->isEqual(normalizedOverrideTy)) {
-        // If both are nominal types, check again, ignoring generic arguments.
-        auto *overrideNominal = normalizedOverrideTy->getAnyNominal();
-        if (!overrideNominal || bridged->getAnyNominal() != overrideNominal) {
-          return false;
-        }
-      }
-
-      Type newOverrideTy = baseTy;
-
-      // Preserve optionality if we're dealing with a simple type.
-      OptionalTypeKind OTK;
-      if (Type unwrappedTy = newOverrideTy->getAnyOptionalObjectType())
-        newOverrideTy = unwrappedTy;
-      if (overrideTy->getAnyOptionalObjectType(OTK))
-        newOverrideTy = OptionalType::get(OTK, newOverrideTy);
-
-      SmallString<32> baseTypeBuf;
-      llvm::raw_svector_ostream baseTypeStr(baseTypeBuf);
-      PrintOptions options;
-      options.SynthesizeSugarOnTypes = true;
-
-      newOverrideTy->print(baseTypeStr, options);
-      diag.fixItReplace(typeRange, baseTypeStr.str());
-      return true;
-    };
-
-    if (auto *var = dyn_cast<VarDecl>(decl)) {
-      SourceRange typeRange = var->getTypeSourceRangeForDiagnostics();
-      return checkType(var->getType(), base->getType(), typeRange);
-    }
-
-    if (auto *fn = dyn_cast<AbstractFunctionDecl>(decl)) {
-      auto *baseFn = cast<AbstractFunctionDecl>(base);
-      bool fixedAny = false;
-      if (fn->getParameterLists().back()->size() ==
-          baseFn->getParameterLists().back()->size()) {
-        for_each(*fn->getParameterLists().back(),
-                 *baseFn->getParameterLists().back(),
-                 [&](ParamDecl *param, const ParamDecl *baseParam) {
-          fixedAny |= fixOverrideDeclarationTypes(diag, TC, param, baseParam);
-        });
-      }
-      if (auto *method = dyn_cast<FuncDecl>(decl)) {
-        auto *baseMethod = cast<FuncDecl>(base);
-        fixedAny |= checkType(method->getBodyResultType(),
-                              baseMethod->getBodyResultType(),
-                              method->getBodyResultTypeLoc().getSourceRange());
-      }
-      return fixedAny;
-    }
-
-    if (auto *subscript = dyn_cast<SubscriptDecl>(decl)) {
-      auto *baseSubscript = cast<SubscriptDecl>(base);
-      bool fixedAny = false;
-      for_each(*subscript->getIndices(),
-               *baseSubscript->getIndices(),
-               [&](ParamDecl *param, const ParamDecl *baseParam) {
-        fixedAny |= fixOverrideDeclarationTypes(diag, TC, param, baseParam);
-      });
-      fixedAny |= checkType(subscript->getElementType(),
-                            baseSubscript->getElementType(),
-                            subscript->getElementTypeLoc().getSourceRange());
-      return fixedAny;
-    }
-
-    llvm_unreachable("unknown overridable member");
-  }
-
   /// If the difference between the types of \p decl and \p base is something
   /// we feel confident about fixing (even partially), emit a note with fix-its
   /// attached. Otherwise, no note will be emitted.
@@ -4926,7 +4817,7 @@ public:
                                        base->getDescriptiveKind(), baseTy));
       }
 
-      if (fixOverrideDeclarationTypes(*activeDiag, TC, decl, base))
+      if (fixItOverrideDeclarationTypes(TC, *activeDiag, decl, base))
         return true;
     }
 

lib/Sema/TypeCheckProtocol.cpp

@@ -1733,11 +1733,14 @@ diagnoseMatch(TypeChecker &tc, Module *module,
     // about them.
     break;
 
-  case MatchKind::TypeConflict:
+  case MatchKind::TypeConflict: {
-    tc.diagnose(match.Witness, diag::protocol_witness_type_conflict,
+    auto diag = tc.diagnose(match.Witness, diag::protocol_witness_type_conflict,
-                getTypeForDisplay(tc, module, match.Witness),
+                            getTypeForDisplay(tc, module, match.Witness),
-                withAssocTypes);
+                            withAssocTypes);
+    if (!isa<TypeDecl>(req))
+      fixItOverrideDeclarationTypes(tc, diag, match.Witness, req);
     break;
+  }
 
   case MatchKind::ThrowsConflict:
     tc.diagnose(match.Witness, diag::protocol_witness_throws_conflict);

test/ClangModules/objc_bridging_custom.swift

@@ -85,3 +85,125 @@ class Sub : Base {
     return super.prop // expected-error {{return type}}
   }
 }
+
+protocol TestProto {
+  func test(a: Refrigerator, b: Refrigerator) -> Refrigerator? // expected-note {{protocol requires}} {{none}}
+  func testGeneric(a: ManufacturerInfo<NSString>, b: ManufacturerInfo<NSString>) -> ManufacturerInfo<NSString>? // expected-note {{protocol requires}} {{none}}
+  static func testInout(_: inout Refrigerator) // expected-note {{protocol requires}} {{none}}
+  func testUnmigrated(a: NSRuncingMode, b: Refrigerator, c: NSCoding) // expected-note {{protocol requires}} {{none}}
+  func testPartialMigrated(a: NSRuncingMode, b: Refrigerator) // expected-note {{protocol requires}} {{none}}
+
+  subscript(a a: Refrigerator, b b: Refrigerator) -> Refrigerator? { get } // expected-note {{protocol requires}} {{none}}
+  subscript(generic a: ManufacturerInfo<NSString>, b b: ManufacturerInfo<NSString>) -> ManufacturerInfo<NSString>? { get } // expected-note {{protocol requires}} {{none}}
+
+  init?(a: Refrigerator, b: Refrigerator) // expected-note {{protocol requires}} {{none}}
+  init?(generic: ManufacturerInfo<NSString>, b: ManufacturerInfo<NSString>) // expected-note {{protocol requires}} {{none}}
+  init(singleArgument: Refrigerator) // expected-note {{protocol requires}} {{none}}
+
+  var prop: Refrigerator? { get } // expected-note {{protocol requires}} {{none}}
+  var propGeneric: ManufacturerInfo<NSString>? { get } // expected-note {{protocol requires}} {{none}}
+}
+
+class TestProtoImpl : NSObject, TestProto { // expected-error {{type 'TestProtoImpl' does not conform to protocol 'TestProto'}}
+  // expected-note@+1 {{candidate has non-matching type '(a: APPRefrigerator, b: APPRefrigerator?) -> APPRefrigerator'}} {{16-31=Refrigerator}} {{36-52=Refrigerator?}} {{57-72=Refrigerator}}
+  func test(a: APPRefrigerator, b: APPRefrigerator?) -> APPRefrigerator {
+    return a
+  }
+  // expected-note@+1 {{candidate has non-matching type '(a: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject>'}} {{23-53=ManufacturerInfo<NSString>}} {{58-89=ManufacturerInfo<NSString>?}} {{94-124=ManufacturerInfo<NSString>}}
+  func testGeneric(a: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject> {
+    return a
+  }
+  // expected-note@+1 {{candidate has non-matching type '(inout APPRefrigerator) -> ()'}} {{27-48=inout Refrigerator}}
+  class func testInout(_: inout APPRefrigerator) {}
+
+  // expected-note@+1 {{candidate has non-matching type '(a: NSObject, b: NSObject, c: NSObject) -> ()'}}
+  func testUnmigrated(a: NSObject, b: NSObject, c: NSObject) {}
+
+  // expected-note@+1 {{candidate has non-matching type '(a: NSObject, b: APPRefrigerator) -> ()'}} {{44-59=Refrigerator}}
+  func testPartialMigrated(a: NSObject, b: APPRefrigerator) {}
+
+  // expected-note@+1 {{candidate has non-matching type '(a: APPRefrigerator, b: APPRefrigerator?) -> APPRefrigerator'}} {{18-33=Refrigerator}} {{40-56=Refrigerator?}} {{61-76=Refrigerator}}
+  subscript(a a: APPRefrigerator, b b: APPRefrigerator?) -> APPRefrigerator {
+    return a
+  }
+  // expected-note@+1 {{candidate has non-matching type '(generic: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject>'}} {{24-54=ManufacturerInfo<NSString>}} {{61-92=ManufacturerInfo<NSString>?}} {{97-127=ManufacturerInfo<NSString>}}
+  subscript(generic a: APPManufacturerInfo<AnyObject>, b b: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject> {
+    return a
+  }
+
+  // expected-note@+1 {{candidate has non-matching type '(a: APPRefrigerator, b: APPRefrigerator?)'}} {{11-26=Refrigerator}} {{31-47=Refrigerator?}}
+  init(a: APPRefrigerator, b: APPRefrigerator?) {}
+  // expected-note@+1 {{candidate has non-matching type '(generic: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?)'}} {{19-49=ManufacturerInfo<NSString>}} {{54-85=ManufacturerInfo<NSString>?}}
+  init(generic a: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?) {}
+  // expected-note@+1 {{candidate has non-matching type '(singleArgument: APPRefrigerator)'}} {{24-39=Refrigerator}}
+  init(singleArgument: APPRefrigerator) {}
+
+  // expected-note@+1 {{candidate has non-matching type 'APPRefrigerator?'}} {{13-29=Refrigerator?}} {{none}}
+  var prop: APPRefrigerator? {
+    return nil
+  }
+  // expected-note@+1 {{candidate has non-matching type 'APPManufacturerInfo<AnyObject>?'}} {{20-51=ManufacturerInfo<NSString>?}}
+  var propGeneric: APPManufacturerInfo<AnyObject>? {
+    return nil
+  }
+}
+
+
+@objc protocol TestObjCProto {
+  @objc optional func test(a: Refrigerator, b: Refrigerator) -> Refrigerator? // expected-note {{here}} {{none}}
+  @objc optional func testGeneric(a: ManufacturerInfo<NSString>, b: ManufacturerInfo<NSString>) -> ManufacturerInfo<NSString>? // expected-note {{here}} {{none}}
+  @objc optional func testUnmigrated(a: NSRuncingMode, b: Refrigerator, c: NSCoding) // expected-note {{here}} {{none}}
+  @objc optional func testPartialMigrated(a: NSRuncingMode, b: Refrigerator) // expected-note {{here}} {{none}}
+
+  @objc optional subscript(a a: Refrigerator) -> Refrigerator? { get } // expected-note {{here}} {{none}}
+  @objc optional subscript(generic a: ManufacturerInfo<NSString>) -> ManufacturerInfo<NSString>? { get } // expected-note {{here}} {{none}}
+
+  @objc optional var prop: Refrigerator? { get } // expected-note {{here}} {{none}}
+  @objc optional var propGeneric: ManufacturerInfo<NSString>? { get } // expected-note {{here}} {{none}}
+}
+
+class TestObjCProtoImpl : NSObject, TestObjCProto {
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type '(a: APPRefrigerator, b: APPRefrigerator?) -> APPRefrigerator'}} {{16-31=Refrigerator}} {{36-52=Refrigerator?}} {{57-72=Refrigerator}}
+  func test(a: APPRefrigerator, b: APPRefrigerator?) -> APPRefrigerator { // expected-warning {{instance method 'test(a:b:)' nearly matches optional requirement 'test(a:b:)' of protocol 'TestObjCProto'}}
+    return a
+  }
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type '(a: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject>'}} {{23-53=ManufacturerInfo<NSString>}} {{58-89=ManufacturerInfo<NSString>?}} {{94-124=ManufacturerInfo<NSString>}}
+  func testGeneric(a: APPManufacturerInfo<AnyObject>, b: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject> { // expected-warning {{instance method 'testGeneric(a:b:)' nearly matches optional requirement 'testGeneric(a:b:)' of protocol 'TestObjCProto'}} {{none}}
+    return a
+  }
+
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type '(a: NSObject, b: NSObject, c: NSObject) -> ()'}} {{none}}
+  func testUnmigrated(a: NSObject, b: NSObject, c: NSObject) {} // expected-warning {{instance method 'testUnmigrated(a:b:c:)' nearly matches optional requirement 'testUnmigrated(a:b:c:)' of protocol 'TestObjCProto'}}
+
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type '(a: NSObject, b: APPRefrigerator) -> ()'}} {{44-59=Refrigerator}}
+  func testPartialMigrated(a: NSObject, b: APPRefrigerator) {} // expected-warning {{instance method 'testPartialMigrated(a:b:)' nearly matches optional requirement 'testPartialMigrated(a:b:)' of protocol 'TestObjCProto'}} {{none}}
+
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type '(a: APPRefrigerator?) -> APPRefrigerator'}} {{18-34=Refrigerator?}} {{39-54=Refrigerator}}
+  subscript(a a: APPRefrigerator?) -> APPRefrigerator { // expected-warning {{subscript 'subscript(a:)' nearly matches optional requirement 'subscript(a:)' of protocol 'TestObjCProto'}} {{none}}
+  // expected-note@-1 {{here}}
+    return a!
+  }
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type '(generic: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject>'}} {{24-55=ManufacturerInfo<NSString>?}} {{60-90=ManufacturerInfo<NSString>}}
+  subscript(generic a: APPManufacturerInfo<AnyObject>?) -> APPManufacturerInfo<AnyObject> { // expected-warning {{subscript 'subscript(generic:)' nearly matches optional requirement 'subscript(generic:)' of protocol 'TestObjCProto'}} {{none}}
+  // expected-error@-1 {{subscript getter with Objective-C selector 'objectForKeyedSubscript:' conflicts with previous declaration with the same Objective-C selector}}
+    return a!
+  }
+
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type 'APPRefrigerator?'}} {{13-29=Refrigerator?}}
+  var prop: APPRefrigerator? { // expected-warning {{var 'prop' nearly matches optional requirement 'prop' of protocol 'TestObjCProto'}} {{none}}
+    return nil
+  }
+
+  // expected-note@+2 {{private}} expected-note@+2 {{@nonobjc}} expected-note@+2 {{extension}}
+  // expected-note@+1 {{candidate has non-matching type 'APPManufacturerInfo<AnyObject>?'}} {{20-51=ManufacturerInfo<NSString>?}}
+  var propGeneric: APPManufacturerInfo<AnyObject>? { // expected-warning {{var 'propGeneric' nearly matches optional requirement 'propGeneric' of protocol 'TestObjCProto'}} {{none}}
+    return nil
+  }
+}