[CSRanking] Swift 4.1 compatibility hack for favouring properties on concrete types over protocols

lib/Sema/CSRanking.cpp

@@ -756,6 +756,9 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
   bool isStdlibOptionalMPlusOperator1 = false;
   bool isStdlibOptionalMPlusOperator2 = false;
 
+  bool isVarAndNotProtocol1 = false;
+  bool isVarAndNotProtocol2 = false;
+
   auto getWeight = [&](ConstraintLocator *locator) -> unsigned {
     if (auto *anchor = locator->getAnchor()) {
       auto weight = weights.find(anchor);
@@ -962,6 +965,32 @@ 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 (!tc.Context.isSwiftVersionAtLeast(5) &&
+        choice1.getKind() != OverloadChoiceKind::DeclViaDynamic &&
+        choice2.getKind() != OverloadChoiceKind::DeclViaDynamic &&
+        isa<VarDecl>(decl1) && isa<VarDecl>(decl2)) {
+      auto *nominal1 = dc1->getSelfNominalTypeDecl();
+      auto *nominal2 = dc2->getSelfNominalTypeDecl();
+      if (nominal1 && nominal2 && nominal1 != nominal2) {
+        isVarAndNotProtocol1 = !isa<ProtocolDecl>(nominal1);
+        isVarAndNotProtocol2 = !isa<ProtocolDecl>(nominal2);
+      }
+    }
+
     // 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 +1147,14 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
     score1 += isStdlibOptionalMPlusOperator2;
   }
 
+  // All other things being equal, apply the Swift 4.1 compatibility hack for
+  // preferring var members in concrete types over a protocol requirement
+  // (see the comment above for the rationale of this hack).
+  if (!tc.Context.isSwiftVersionAtLeast(5) && score1 == score2) {
+    score1 += isVarAndNotProtocol1;
+    score2 += isVarAndNotProtocol2;
+  }
+
   // 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.

test/Constraints/dynamic_lookup.swift

@@ -341,18 +341,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/rdar39401774.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -swift-version 5
 
 class A<T> {
   var foo: Int? { return 42 }      // expected-note {{found this candidate}}

test/Constraints/sr7425.swift

@@ -0,0 +1,79 @@
+// RUN: %target-typecheck-verify-swift -swift-version 4
+
+protocol X {
+  var foo: Int { get }
+  var bar: Int { get }
+}
+
+class Y {
+  var foo: Int = 0
+}
+extension Y {
+  var bar: Int { return foo }
+}
+
+protocol Z : Y {
+  var foo: Int { get }
+  var bar: Int { get }
+}
+
+class GenericClass<T> {
+  var foo: T
+  init(_ foo: T) { self.foo = foo }
+}
+extension GenericClass {
+  var bar: T { return foo }
+}
+
+// Make sure we keep all of the following cases unambiguous to retain source compatibility with Swift 4.1.
+
+func testGenericPropertyProtocolClass<T : X & Y>(_ t: T) {
+  _ = t.foo
+  _ = t.bar
+}
+
+func testExistentialPropertyProtocolClass(_ t: X & Y) {
+  _ = t.foo
+  _ = t.bar
+}
+
+func testGenericPropertySubclassConstrainedProtocol<T : Z>(_ t: T) {
+  _ = t.foo
+  _ = t.bar
+}
+
+func testExistentialPropertySubclassConstrainedProtocol(_ t: Z) {
+  _ = t.foo
+  _ = t.bar
+}
+
+func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<Int> & X) {
+  _ = t.foo
+  _ = t.bar
+}
+
+func testExistentialPropertyProtocolGenericClass(_ t: GenericClass<String> & X) {
+  _ = t.foo
+  _ = t.bar
+}
+
+extension X where Self : Y {
+  func testGenericPropertyProtocolClass(_ x: Self) {
+    _ = self.foo
+    _ = self.bar
+  }
+}
+
+extension X where Self : GenericClass<Int> {
+  func testGenericPropertyProtocolGenericClass(_ x: Self) {
+    _ = self.foo
+    _ = self.bar
+  }
+}
+
+extension X where Self : GenericClass<String> {
+  func testGenericPropertyProtocolGenericClass(_ x: Self) {
+    _ = self.foo
+    _ = self.bar
+  }
+}