Protocol requirement overrides must match in mutating-ness.

Without this change, SILGen will crash when compiling a use of the
derived protocol's requirement: it will instead attempt to use
the base protocol's requirement, but the code will have been
type-checked incorrectly for that.

This has a potential for source-compatibility impact if anyone's
using explicit override checking for their protocol requirements:
reasonable idioms like overriding a mutating requirement with a
non-mutating one will no longer count as an override.  However,
this is arguably a bug-fix, because the current designed intent
of protocol override checking is to not allow any differences in
type, even "covariant" changes like making a mutating requirement
non-mutating.  Moreover, we believe explicit override checking in
protocols is quite uncommon, so the overall compatibility impact
will be low.

This also has a potential for ABI impact whenever something that
was once an override becomes a non-override and thus requires a
new entry.  It might require a contrived test case to demonstrate
that while using the derived entry, but it's quite possible to
imagine a situation where the derived entry is not used directly
but nonetheless has ABI impact.

Furthermore, as part of developing this patch (earlier versions of
which used stricter rules in places), I discovered a number of
places where the standard library was unintentionally introducing
a new requirement in a derived protocol when it intended only to
guide associated type deduction.  Fixing that (as I have in this
patch) *definitely* has ABI impact.

Author: rjmccall

lib/Sema/TypeCheckDeclOverride.cpp

@@ -120,6 +120,37 @@ Type swift::getMemberTypeForComparison(ASTContext &ctx, ValueDecl *member,
   return memberType;
 }
 
+static bool areAccessorsOverrideCompatible(AbstractStorageDecl *storage,
+                                           AbstractStorageDecl *parentStorage) {
+  // It's okay for the storage to disagree about whether to use a getter or
+  // a read accessor; we'll patch up any differences when setting overrides
+  // for the accessors.  We don't want to diagnose anything involving
+  // `@_borrowed` because it is not yet part of the language.
+
+  // All the other checks are for non-static storage only.
+  if (storage->isStatic())
+    return true;
+
+  // The storage must agree on whether reads are mutating.  For accessors,
+  // this is sufficient to imply that they use the same SelfAccessKind
+  // because we do not allow accessors to be consuming.
+  if (storage->isGetterMutating() != parentStorage->isGetterMutating())
+    return false;
+
+  // We allow covariance about whether the storage itself is mutable, so we
+  // can only check mutating-ness of setters if both have one.
+  if (storage->supportsMutation() && parentStorage->supportsMutation()) {
+    // The storage must agree on whether writes are mutating.
+    if (storage->isSetterMutating() != parentStorage->isSetterMutating())
+      return false;
+
+    // Those together should imply that read-write accesses have the same
+    // mutability.
+  }
+
+  return true;
+}
+
 bool swift::isOverrideBasedOnType(ValueDecl *decl, Type declTy,
                                   ValueDecl *parentDecl, Type parentDeclTy) {
   auto *genericSig =
@@ -149,6 +180,23 @@ bool swift::isOverrideBasedOnType(ValueDecl *decl, Type declTy,
     auto fnType2 = parentDeclTy->castTo<AnyFunctionType>();
     return AnyFunctionType::equalParams(fnType1->getParams(),
                                         fnType2->getParams());
+
+  // In a non-static protocol requirement, verify that the self access kind
+  // matches.
+  } else if (auto func = dyn_cast<FuncDecl>(decl)) {
+    // We only compare `isMutating()` rather than `getSelfAccessKind()`
+    // because we don't want to complain about `nonmutating` vs. `__consuming`
+    // conflicts at this time, especially since `__consuming` is not yet
+    // officially part of the language.
+    if (!func->isStatic() &&
+        func->isMutating() != cast<FuncDecl>(parentDecl)->isMutating())
+      return false;
+
+  // In abstract storage, verify that the accessor mutating-ness matches.
+  } else if (auto storage = dyn_cast<AbstractStorageDecl>(decl)) {
+    auto parentStorage = cast<AbstractStorageDecl>(parentDecl);
+    if (!areAccessorsOverrideCompatible(storage, parentStorage))
+      return false;
   }
 
   return canDeclTy == canParentDeclTy;

stdlib/public/core/BidirectionalCollection.swift

@@ -211,6 +211,7 @@ where SubSequence: BidirectionalCollection, Indices: BidirectionalCollection {
   override subscript(bounds: Range<Index>) -> SubSequence { get }
 
   // FIXME: Only needed for associated type inference.
+  @_borrowed
   override subscript(position: Index) -> Element { get }
   override var startIndex: Index { get }
   override var endIndex: Index { get }

stdlib/public/core/Collection.swift

@@ -382,7 +382,7 @@ public protocol Collection: Sequence {
   // we get an `IndexingIterator` rather than properly deducing the
   // Iterator type from makeIterator(). <rdar://problem/21539115>
   /// Returns an iterator over the elements of the collection.
-  override func makeIterator() -> Iterator
+  override __consuming func makeIterator() -> Iterator
 
   /// A sequence that represents a contiguous subrange of the collection's
   /// elements.

stdlib/public/core/CollectionOfOne.swift

@@ -127,9 +127,9 @@ extension CollectionOfOne: RandomAccessCollection, MutableCollection {
   ///   valid position in a `CollectionOfOne` instance is `0`.
   @inlinable // trivial-implementation
   public subscript(position: Int) -> Element {
-    get {
+    _read {
       _precondition(position == 0, "Index out of range")
-      return _element
+      yield _element
     }
     _modify {
       _precondition(position == 0, "Index out of range")

stdlib/public/core/RandomAccessCollection.swift

@@ -84,6 +84,7 @@ where SubSequence: RandomAccessCollection, Indices: RandomAccessCollection
   override subscript(bounds: Range<Index>) -> SubSequence { get }
 
   // FIXME: Associated type inference requires these.
+  @_borrowed
   override subscript(position: Index) -> Element { get }
   override var startIndex: Index { get }
   override var endIndex: Index { get }

stdlib/public/core/RangeReplaceableCollection.swift

@@ -363,6 +363,7 @@ public protocol RangeReplaceableCollection : Collection
     where shouldBeRemoved: (Element) throws -> Bool) rethrows
 
   // FIXME: Associated type inference requires these.
+  @_borrowed
   override subscript(bounds: Index) -> Element { get }
   override subscript(bounds: Range<Index>) -> SubSequence { get }
 }

test/SILGen/protocol_overrides.swift

@@ -0,0 +1,107 @@
+// RUN: %target-swift-emit-silgen -module-name main %s | %FileCheck %s
+
+// rdar://47470420
+
+/*****************************************************************************/
+/* Methods *******************************************************************/
+/*****************************************************************************/
+
+protocol HasMutatingMethod {
+  mutating func foo()
+}
+
+protocol HasMutatingMethodClone: HasMutatingMethod {
+  mutating func foo()
+}
+extension HasMutatingMethodClone {
+// CHECK-LABEL: sil hidden [ossa] @$s4main22HasMutatingMethodClonePAAE10performFooyyF :
+// CHECK:         witness_method $Self, #HasMutatingMethod.foo!1
+  mutating func performFoo() {
+    foo()
+  }
+}
+
+protocol HasNonMutatingMethod: HasMutatingMethod {
+  func foo()
+}
+extension HasNonMutatingMethod {
+// CHECK-LABEL: sil hidden [ossa] @$s4main20HasNonMutatingMethodPAAE10performFooyyF :
+// CHECK:         witness_method $Self, #HasNonMutatingMethod.foo!1 :
+  func performFoo() {
+    foo()
+  }
+}
+
+/*****************************************************************************/
+/* Getters *******************************************************************/
+/*****************************************************************************/
+
+protocol HasMutatingGetter {
+  var foo: Int { mutating get }
+}
+
+protocol HasMutatingGetterClone: HasMutatingGetter {
+  var foo: Int { mutating get }
+}
+extension HasMutatingGetterClone {
+// CHECK-LABEL: sil hidden [ossa] @$s4main22HasMutatingGetterClonePAAE11readFromFooSiyF :
+// CHECK:         witness_method $Self, #HasMutatingGetter.foo!getter.1 :
+  mutating func readFromFoo() -> Int {
+    return foo
+  }
+}
+
+protocol HasNonMutatingGetter: HasMutatingGetter {
+  var foo: Int { get }
+}
+extension HasNonMutatingGetter {
+// CHECK-LABEL: sil hidden [ossa] @$s4main20HasNonMutatingGetterPAAE11readFromFooSiyF :
+// CHECK:         witness_method $Self, #HasNonMutatingGetter.foo!getter.1 :
+  func readFromFoo() -> Int {
+    return foo
+  }
+}
+
+/*****************************************************************************/
+/* Setters *******************************************************************/
+/*****************************************************************************/
+
+protocol HasMutatingSetter {
+  var foo: Int { get set }
+}
+
+protocol HasMutatingSetterClone: HasMutatingSetter {
+  var foo: Int { get set }
+}
+extension HasMutatingSetterClone {
+// CHECK-LABEL: sil hidden [ossa] @$s4main22HasMutatingSetterClonePAAE11readFromFooSiyF :
+// CHECK:         witness_method $Self, #HasMutatingSetter.foo!getter.1 :
+  func readFromFoo() -> Int {
+    return foo
+  }
+
+// CHECK-LABEL: sil hidden [ossa] @$s4main22HasMutatingSetterClonePAAE10writeToFooyySiF :
+// CHECK:         witness_method $Self, #HasMutatingSetter.foo!setter.1 :
+  mutating func writeToFoo(_ x: Int) {
+    foo = x
+  }
+}
+
+protocol HasNonMutatingSetter: HasMutatingSetter {
+  var foo: Int { get nonmutating set }
+}
+extension HasNonMutatingSetter {
+//   It's unfortunate that this uses a new witness table entry,
+//   but we can live with it.
+// CHECK-LABEL: sil hidden [ossa] @$s4main20HasNonMutatingSetterPAAE11readFromFooSiyF :
+// CHECK:         witness_method $Self, #HasNonMutatingSetter.foo!getter.1 :
+  func readFromFoo() -> Int {
+    return foo
+  }
+
+// CHECK-LABEL: sil hidden [ossa] @$s4main20HasNonMutatingSetterPAAE10writeToFooyySiF :
+// CHECK:         witness_method $Self, #HasNonMutatingSetter.foo!setter.1 :
+  func writeToFoo(_ x: Int) {
+    foo = x
+  }
+}

test/api-digester/Outputs/stability-stdlib-abi.swift.expected

@@ -82,3 +82,5 @@ Protocol SIMDStorage has generic signature change from <τ_0_0.Scalar : Hashable
 Func Sequence.flatMap(_:) has been removed
 Subscript String.UnicodeScalarView.subscript(_:) has been removed
 Subscript Substring.subscript(_:) has been removed
+
+Func Collection.makeIterator() has self access kind changing from NonMutating to __Consuming

test/api-digester/Outputs/stability-stdlib-source.swift.expected

@@ -1,3 +1,4 @@
+Func Collection.makeIterator() has self access kind changing from NonMutating to __Consuming
 Func tryReallocateUniquelyReferenced(buffer:newMinimumCapacity:) has been removed
 Protocol SIMD has added inherited protocol Decodable
 Protocol SIMD has added inherited protocol Encodable