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

Changes in shadowing behaviour by #15412 caused a property on a concrete type to no longer shadow a protocol property member, which created unintentional ambiguities in 4.2. This commit ensures we at least keep these cases unambiguous in Swift 5 under Swift 4 compatibility mode. This is intentionally narrow in order to best preserve source compatibility under Swift 4 mode by ensuring we don't introduce any new ambiguities.

Resolves SR-7425, SR-7940 & SR-8343.

Author: hamishknight

lib/Sema/CSRanking.cpp

@@ -775,6 +775,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);
@@ -981,6 +984,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() == "??") {
@@ -1134,6 +1163,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/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
+  }
+}