[CSRanking] Favour concrete members over protocol requirements

lib/Sema/CSRanking.cpp

@@ -389,15 +389,18 @@ static bool paramIsIUO(Decl *decl, int paramNum) {
 ///
 /// "Specialized" is essentially a form of subtyping, defined below.
 static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
-                                  ValueDecl *decl1, ValueDecl *decl2) {
+                                  ValueDecl *decl1, ValueDecl *decl2,
+                                  bool isDynamicOverloadComparison = false) {
 
   if (tc.getLangOpts().DebugConstraintSolver) {
     auto &log = tc.Context.TypeCheckerDebug->getStream();
     log << "Comparing declarations\n";
     decl1->print(log); 
     log << "\nand\n";
     decl2->print(log);
-    log << "\n";
+    log << "\n(isDynamicOverloadComparison: ";
+    log << isDynamicOverloadComparison;
+    log << ")\n";
   }
 
   auto *innerDC1 = decl1->getInnermostDeclContext();
@@ -406,7 +409,9 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
   auto *outerDC1 = decl1->getDeclContext();
   auto *outerDC2 = decl2->getDeclContext();
 
-  if (!tc.specializedOverloadComparisonCache.count({decl1, decl2})) {
+  auto overloadComparisonKey =
+      std::make_tuple(decl1, decl2, isDynamicOverloadComparison);
+  if (!tc.specializedOverloadComparisonCache.count(overloadComparisonKey)) {
 
     auto compareSpecializations = [&] () -> bool {
       // If the kinds are different, there's nothing we can do.
@@ -446,6 +451,32 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
         return inProtocolExtension2;
       }
 
+      // A concrete type member is always more specialised than a protocol
+      // member (bearing in mind that we have already handled the case where
+      // exactly one member is in a protocol extension). Only apply this rule in
+      // Swift 5 mode to better maintain source compatibility under Swift 4
+      // mode.
+      //
+      // Don't apply this rule when comparing two overloads found through
+      // dynamic lookup to ensure we keep cases like this ambiguous:
+      //
+      //    @objc protocol P {
+      //      var i: String { get }
+      //    }
+      //    class C {
+      //      @objc var i: Int { return 0 }
+      //    }
+      //    func foo(_ x: AnyObject) {
+      //      x.i // ensure ambiguous.
+      //    }
+      //
+      if (true && !isDynamicOverloadComparison) {
+        auto *proto1 = dyn_cast<ProtocolDecl>(outerDC1);
+        auto *proto2 = dyn_cast<ProtocolDecl>(outerDC2);
+        if (proto1 != proto2)
+          return proto2;
+      }
+
       Type type1 = decl1->getInterfaceType();
       Type type2 = decl2->getInterfaceType();
 
@@ -697,21 +728,21 @@ static bool isDeclAsSpecializedAs(TypeChecker &tc, DeclContext *dc,
       return false;
     };
 
-    tc.specializedOverloadComparisonCache[{decl1, decl2}] = 
+    tc.specializedOverloadComparisonCache[overloadComparisonKey] =
         compareSpecializations();
   } else if (tc.getLangOpts().DebugConstraintSolver) {
     auto &log = tc.Context.TypeCheckerDebug->getStream();
-    log << "Found cached comparison: " 
-        << tc.specializedOverloadComparisonCache[{decl1, decl2}] << "\n";
+    log << "Found cached comparison: "
+        << tc.specializedOverloadComparisonCache[overloadComparisonKey] << "\n";
   }
 
   if (tc.getLangOpts().DebugConstraintSolver) {
     auto &log = tc.Context.TypeCheckerDebug->getStream();
-    auto result = tc.specializedOverloadComparisonCache[{decl1, decl2}];
+    auto result = tc.specializedOverloadComparisonCache[overloadComparisonKey];
     log << "comparison result: " << (result ? "better" : "not better") << "\n";
   }
 
-  return tc.specializedOverloadComparisonCache[{decl1, decl2}];
+  return tc.specializedOverloadComparisonCache[overloadComparisonKey];
 }
 
 Comparison TypeChecker::compareDeclarations(DeclContext *dc,
@@ -756,6 +787,9 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
   bool isStdlibOptionalMPlusOperator1 = false;
   bool isStdlibOptionalMPlusOperator2 = false;
 
+  bool isSwift4ConcreteOverProtocolVar1 = false;
+  bool isSwift4ConcreteOverProtocolVar2 = false;
+
   auto getWeight = [&](ConstraintLocator *locator) -> unsigned {
     if (auto *anchor = locator->getAnchor()) {
       auto weight = weights.find(anchor);
@@ -849,15 +883,23 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
     case OverloadChoiceKind::DynamicMemberLookup:
       break;
     }
-
+
+    // We don't apply some ranking rules to overloads found through dynamic
+    // lookup in order to keep a few potentially ill-formed cases ambiguous.
+    bool isDynamicOverloadComparison =
+        choice1.getKind() == OverloadChoiceKind::DeclViaDynamic &&
+        choice2.getKind() == OverloadChoiceKind::DeclViaDynamic;
+
     // Determine whether one declaration is more specialized than the other.
     bool firstAsSpecializedAs = false;
     bool secondAsSpecializedAs = false;
-    if (isDeclAsSpecializedAs(tc, cs.DC, decl1, decl2)) {
+    if (isDeclAsSpecializedAs(tc, cs.DC, decl1, decl2,
+                              isDynamicOverloadComparison)) {
       score1 += weight;
       firstAsSpecializedAs = true;
     }
-    if (isDeclAsSpecializedAs(tc, cs.DC, decl2, decl1)) {
+    if (isDeclAsSpecializedAs(tc, cs.DC, decl2, decl1,
+                              isDynamicOverloadComparison)) {
       score2 += weight;
       secondAsSpecializedAs = true;
     }
@@ -962,6 +1004,30 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
       }
     }
 
+    // Swift 4.1 compatibility hack: If everything else is considered equal,
+    // favour a property on a concrete type over a protocol property member.
+    //
+    // This hack is required due to changes in shadowing behaviour where a
+    // protocol property member will no longer shadow a property on a concrete
+    // type, which created unintentional ambiguities in 4.2. This hack ensures
+    // we at least keep these cases unambiguous in Swift 5 under Swift 4
+    // compatibility mode. Don't however apply this hack for decls found through
+    // dynamic lookup, as we want the user to have to disambiguate those.
+    //
+    // This is intentionally narrow in order to best preserve source
+    // compatibility under Swift 4 mode by ensuring we don't introduce any new
+    // ambiguities. This will become a more general "is more specialised" rule
+    // in Swift 5 mode.
+    if (false && !isDynamicOverloadComparison &&
+        isa<VarDecl>(decl1) && isa<VarDecl>(decl2)) {
+      auto *nominal1 = dc1->getSelfNominalTypeDecl();
+      auto *nominal2 = dc2->getSelfNominalTypeDecl();
+      if (nominal1 && nominal2 && nominal1 != nominal2) {
+        isSwift4ConcreteOverProtocolVar1 = isa<ProtocolDecl>(nominal2);
+        isSwift4ConcreteOverProtocolVar2 = isa<ProtocolDecl>(nominal1);
+      }
+    }
+
     // FIXME: Lousy hack for ?? to prefer the catamorphism (flattening)
     // over the mplus (non-flattening) overload if all else is equal.
     if (decl1->getBaseName() == "??") {
@@ -1118,6 +1184,14 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
     score1 += isStdlibOptionalMPlusOperator2;
   }
 
+  // All other things being equal, apply the Swift 4 compatibility hack for
+  // preferring var members in concrete types over a protocol requirement
+  // (see the comment above for the rationale of this hack).
+  if (false && score1 == score2) {
+    score1 += isSwift4ConcreteOverProtocolVar1;
+    score2 += isSwift4ConcreteOverProtocolVar2;
+  }
+
   // FIXME: There are type variables and overloads not common to both solutions
   // that haven't been considered. They make the systems different, but don't
   // affect ranking. We need to handle this.

lib/Sema/TypeChecker.h

@@ -555,9 +555,11 @@ class TypeChecker final : public LazyResolver {
   SmallVector<NominalTypeDecl*, 8> DelayedCircularityChecks;
 
   // Caches whether a given declaration is "as specialized" as another.
-  llvm::DenseMap<std::pair<ValueDecl*, ValueDecl*>, bool> 
-    specializedOverloadComparisonCache;
-
+  llvm::DenseMap<std::tuple<ValueDecl *, ValueDecl *,
+                            /*isDynamicOverloadComparison*/ unsigned>,
+                 bool>
+      specializedOverloadComparisonCache;
+
   /// A list of closures for the most recently type-checked function, which we
   /// will need to compute captures for.
   std::vector<AnyFunctionRef> ClosuresWithUncomputedCaptures;

test/Constraints/dynamic_lookup.swift

@@ -1,6 +1,7 @@
 // RUN: %empty-directory(%t)
 // RUN: %target-swift-frontend -emit-module %S/Inputs/PrivateObjC.swift -o %t
-// RUN: %target-typecheck-verify-swift -I %t -verify-ignore-unknown
+// RUN: %target-typecheck-verify-swift -swift-version 4 -I %t -verify-ignore-unknown
+// RUN: %target-typecheck-verify-swift -swift-version 5 -I %t -verify-ignore-unknown
 
 // REQUIRES: objc_interop
 import Foundation
@@ -341,18 +342,51 @@ func dynamicInitCrash(ao: AnyObject.Type) {
   // expected-error@-1 {{incorrect argument label in call (have 'blahblah:', expected 'toMemory:')}}
 }
 
-// Test that we correctly diagnose ambiguity for different typed properties available
+// Test that we correctly diagnose ambiguity for different typed members available
 // through dynamic lookup.
 @objc protocol P3 {
-  var i: UInt { get } // expected-note {{found this candidate}}
-  var j: Int { get }
+  var ambiguousProperty: String { get } // expected-note {{found this candidate}}
+  var unambiguousProperty: Int { get }
+
+  func ambiguousMethod() -> String // expected-note 2{{found this candidate}}
+  func unambiguousMethod() -> Int
+
+  func ambiguousMethodParam(_ x: String) // expected-note {{found this candidate}}
+  func unambiguousMethodParam(_ x: Int)
+
+  subscript(ambiguousSubscript _: Int) -> String { get } // expected-note {{found this candidate}}
+  subscript(unambiguousSubscript _: String) -> Int { get } // expected-note {{found this candidate}}
 }
 
 class C1 {
-  @objc var i: Int { return 0 } // expected-note {{found this candidate}}
-  @objc var j: Int { return 0 }
+  @objc var ambiguousProperty: Int { return 0 } // expected-note {{found this candidate}}
+  @objc var unambiguousProperty: Int { return 0 }
+
+  @objc func ambiguousMethod() -> Int { return 0 } // expected-note 2{{found this candidate}}
+  @objc func unambiguousMethod() -> Int { return 0 }
+
+  @objc func ambiguousMethodParam(_ x: Int) {} // expected-note {{found this candidate}}
+  @objc func unambiguousMethodParam(_ x: Int) {}
+
+  @objc subscript(ambiguousSubscript _: Int) -> Int { return 0 } // expected-note {{found this candidate}}
+  @objc subscript(unambiguousSubscript _: String) -> Int { return 0 } // expected-note {{found this candidate}}
 }
 
-_ = (C1() as AnyObject).i // expected-error {{ambiguous use of 'i'}}
-_ = (C1() as AnyObject).j // okay
+func testAnyObjectAmbiguity(_ x: AnyObject) {
+  _ = x.ambiguousProperty // expected-error {{ambiguous use of 'ambiguousProperty'}}
+  _ = x.unambiguousProperty
 
+  _ = x.ambiguousMethod() // expected-error {{ambiguous use of 'ambiguousMethod()'}}
+  _ = x.unambiguousMethod()
+
+  _ = x.ambiguousMethod // expected-error {{ambiguous use of 'ambiguousMethod()'}}
+  _ = x.unambiguousMethod
+
+  _ = x.ambiguousMethodParam // expected-error {{ambiguous use of 'ambiguousMethodParam'}}
+  _ = x.unambiguousMethodParam
+
+  _ = x[ambiguousSubscript: 0] // expected-error {{ambiguous use of 'subscript(ambiguousSubscript:)'}}
+
+  // FIX-ME(SR-8611): This is currently ambiguous but shouldn't be.
+  _ = x[unambiguousSubscript: ""] // expected-error {{ambiguous use of 'subscript(unambiguousSubscript:)'}}
+}

test/Constraints/ranking.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-emit-silgen %s -verify | %FileCheck %s
+// RUN: %target-swift-emit-silgen %s -verify -swift-version 5 | %FileCheck %s
 
 protocol P {
   var p: P { get set }
@@ -155,3 +155,116 @@ func testDerived(b: B) {
   f0(f1(b))
   // CHECK: end sil function '$S7ranking11testDerived1byAA1BC_tF'
 }
+
+protocol X {
+  var foo: Int { get }
+  var bar: Int { get }
+  func baz() -> Int
+  subscript(foo: String) -> Int { get }
+}
+
+class Y {
+  var foo: Int = 0
+  func baz() -> Int { return foo }
+  subscript(foo: String) -> Int { return 0 }
+}
+extension Y {
+  var bar: Int { return foo }
+}
+
+protocol Z : Y {
+  var foo: Int { get }
+  var bar: Int { get }
+  func baz() -> Int
+  subscript(foo: String) -> Int { get }
+}
+
+class GenericClass<T> {
+  var foo: T
+  init(_ foo: T) { self.foo = foo }
+  func baz() -> T { return foo }
+}
+extension GenericClass {
+  var bar: T { return foo }
+  subscript(foo: String) -> Int { return 0 }
+}
+
+// Make sure we favour the class implementation over the protocol requirement.
+
+// CHECK-LABEL: sil hidden @$S7ranking32testGenericPropertyProtocolClassyyxAA1YCRbzAA1XRzlF
+func testGenericPropertyProtocolClass<T : X & Y>(_ t: T) {
+  _ = t.foo   // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
+  _ = t.bar   // CHECK: function_ref @$S7ranking1YC3barSivg
+  _ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
+  _ = t[""]   // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
+}
+
+// CHECK-LABEL: sil hidden @$S7ranking36testExistentialPropertyProtocolClassyyAA1X_AA1YCXcF
+func testExistentialPropertyProtocolClass(_ t: X & Y) {
+  _ = t.foo   // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
+  _ = t.bar   // CHECK: function_ref @$S7ranking1YC3barSivg
+  _ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
+  _ = t[""]   // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
+}
+
+// CHECK-LABEL: sil hidden @$S7ranking46testGenericPropertySubclassConstrainedProtocolyyxAA1ZRzlF
+func testGenericPropertySubclassConstrainedProtocol<T : Z>(_ t: T) {
+  _ = t.foo   // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
+  _ = t.bar   // CHECK: function_ref @$S7ranking1YC3barSivg
+  _ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
+  _ = t[""]   // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
+}
+
+// CHECK-LABEL: sil hidden @$S7ranking50testExistentialPropertySubclassConstrainedProtocolyyAA1Z_pF
+func testExistentialPropertySubclassConstrainedProtocol(_ t: Z) {
+  _ = t.foo   // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
+  _ = t.bar   // CHECK: function_ref @$S7ranking1YC3barSivg
+  _ = t.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
+  _ = t[""]   // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
+}
+
+// CHECK-LABEL: sil hidden @$S7ranking43testExistentialPropertyProtocolGenericClassyyAA1X_AA0fG0CySiGXcF
+func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<Int> & X) {
+  _ = t.foo   // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.foo!getter.1
+  _ = t.bar   // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
+  _ = t.baz() // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.baz
+  _ = t[""]   // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
+}
+
+// CHECK-LABEL: sil hidden @$S7ranking43testExistentialPropertyProtocolGenericClassyyAA1X_AA0fG0CySSGXcF
+func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<String> & X) {
+  _ = t.foo   // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.foo!getter.1
+  _ = t.bar   // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
+  _ = t.baz() // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.baz
+  _ = t[""]   // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
+}
+
+extension X where Self : Y {
+  // CHECK-LABEL: sil hidden @$S7ranking1XPA2A1YCRbzrlE32testGenericPropertyProtocolClassyyxF
+  func testGenericPropertyProtocolClass(_ x: Self) {
+    _ = self.foo   // CHECK: class_method {{%.*}} : $Y, #Y.foo!getter.1
+    _ = self.bar   // CHECK: function_ref @$S7ranking1YC3barSivg
+    _ = self.baz() // CHECK: class_method {{%.*}} : $Y, #Y.baz
+    _ = self[""]   // CHECK: class_method {{%.*}} : $Y, #Y.subscript!getter.1
+  }
+}
+
+extension X where Self : GenericClass<Int> {
+  // CHECK-LABEL: sil hidden @$S7ranking1XPA2A12GenericClassCySiGRbzrlE04testb16PropertyProtocolbC0yyxF
+  func testGenericPropertyProtocolGenericClass(_ x: Self) {
+    _ = self.foo   // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.foo!getter.1
+    _ = self.bar   // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
+    _ = self.baz() // CHECK: class_method {{%.*}} : $GenericClass<Int>, #GenericClass.baz
+    _ = self[""]   // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
+  }
+}
+
+extension X where Self : GenericClass<String> {
+  // CHECK-LABEL: sil hidden @$S7ranking1XPA2A12GenericClassCySSGRbzrlE04testb16PropertyProtocolbC0yyxF
+  func testGenericPropertyProtocolGenericClass(_ x: Self) {
+    _ = self.foo   // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.foo!getter.1
+    _ = self.bar   // CHECK: function_ref @$S7ranking12GenericClassC3barxvg
+    _ = self.baz() // CHECK: class_method {{%.*}} : $GenericClass<String>, #GenericClass.baz
+    _ = self[""]   // CHECK: function_ref @$S7ranking12GenericClassCySiSScig
+  }
+}