Fix source compatibility problems with conforming to multiple Collection axes at once. #7136

jckarter · 2017-01-30T00:28:26Z

Sema: Improve handling of potential witnesses during associated type inference.

The associated type inference is a bit unusual, since we want to be able to infer associated types from protocol extension implementations that themselves depend on the types being inferred a certain way, e.g.:

protocol Runcible { associatedtype Spoon; func runce(_: Spoon) }
extension Runcible where Spoon == Hat {
  func runce(_ hat: Hat) {}
}

For this reason, we would collect potential witnesses from all possible extensions of the type without immediately checking the constraints on each extension. This could lead to an inconsistency where, when ranking potential witnesses, associated type inference would end up picking the "best" candidate as something that can't even be used by the type we're inferring the witness for, leading to incomprehensible behavior. The ranking criterion itself is also flawed, because TC.compareDeclarations is normally comparing candidates for the same well-typed function invocation, but during associated type inference we haven't even established the type system we're comparing candidates within. Witness candidates from extensions with mutually exclusive associated type constraints but otherwise more specific constraints would be unnecessarily dismissed as ambiguous because neither candidate's generic signature is a match for the other's.

Both problems can be mitigated by looking at only the constraints for protocol extensions that specifically constrain the Self type, since those conformances are always explicit in the source code and therefore independent of associated type inference. We can pre-filter witness candidates by rejecting candidates for which the conforming type doesn't match the extension's Self constraints. We can score multiple witness candidates by looking only at the relative constraints they put on Self, considering a witness from a context that more specifically constrains Self to be a better candidate; we should ignore the associated type constraints for this scoring because we don't even know if they apply given the solution we'll ultimately pick. There's probably a more systemic way to go about this, but targeted fixes for both of these problems, along with a following patch to the standard library, restores source compatibility for Collection conformers, fixing rdar://problem/30228957.

stdlib: Adding missing default implementations of subscript(Range) for combinations of [Mutable][RangeReplaceable][Bidirectional|RandomAccess]Collection.

These were overlooked, and somehow code that attempted to make a minimal collection conform to RangeReplaceableCollection and RandomAccessCollection managed to compile successfully in Swift 3.0, but in Swift 3.1…something changed to reject a type that conforms to both due to the lack of a suitable default slicing subscript implementation in the stdlib that provided all the requirements. Fill in these missing implementations. The type checker still fails to pick the right witness without an explicit typealias SubSequence, but this is progress toward rdar://problem/30228957.

jckarter · 2017-01-30T00:28:36Z

@swift-ci Please test

jckarter · 2017-01-30T00:29:22Z

@airspeedswift and @dabrahams, how do the standard library changes here look?

@DougGregor or @rudkx, do the type checker changes look alright for 3.1?

jpsim · 2017-01-30T00:35:00Z

lib/Sema/TypeCheckProtocol.cpp

-
-    // Make sure that solution is better than any of the other solutions
+    
+    // Make sure that solution is better than any of the other solutionsa


swift-ci · 2017-01-30T00:38:57Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 7774d9e231ff14698bd585d6ba19f68b8bc0a2d2
Test requested by - @jckarter

airspeedswift · 2017-01-30T00:43:57Z

I suspect PR #4825 might be what surfaced this problem. This is the first time that change has made it off master into a full release.

airspeedswift · 2017-01-30T01:03:38Z

stdlib/public/core/MutableCollection.swift

+  public subscript(bounds: Range<Index>) -> MutableRandomAccessSlice<Self> {
+    get {
+      _failEarlyRangeCheck(bounds, bounds: startIndex..<endIndex)
+      return .init(base: self, bounds: bounds)


never thought about calling init like this, very cute.

I'm trying hard not to decide it's too cute by half. Maybe it's just a new idiom we should be using everywhere, but at first glance it appears to harm readability.

My main concern was gyb-ability. I can expand out the name if you prefer.

I like it, especially for short functions. Why repeat the name. And makes the fact that you're creating the thing you're returning clearer.

@airspeedswift The reason why repeat it is that despite years of Swift I still think of .xxx as looking up xxx in the current scope instead of the type context. It feels as though it's chaining to Self.init. Of course I'm projecting, but I have a hard time seeing how other people don't make the same mistake. Finally, I don't see how it clarifies anything useful: CapitalizedName(...) is always creating something.

@jckarter how does "gyb-ability" come into play?

Only that I judged init(...) to be clearer than repeating the complicated ${foo}${bar}${bas}Slice expansion.

I guess gyb makes the .init thing more appealing since you have to return an ugly gybbed typename like RangeReplaceable${capability}Slice (not in this example, but in some others).

Re the working of .xxx... that seems like just something to get over, since it's pervasive – this is just one more example. Sure, CapitalizedName(...) means creation but I think the bare .init does make it clearer what it's doing "at a glance" once you're used to it, to me at least, and also eliminates any chance of there being an additional subtype conversion you could be overlooking.

airspeedswift

Changes LGTM. There are probably additional tests needed, like to test that slices have the expected properties, but we can merge without then add them later, will raise a Jira for that. We’ll need that kind of test for testing the conditional conformance refactoring too.

dabrahams · 2017-01-30T02:44:24Z

test/stdlib/collection-combinatorics.swift.gyb

+    where C.Iterator.Element == Int
+  {}
+%       end
+}


Nice thorough testing, Joe!

dabrahams

Modulo a decision about the .init(...) cuteness, it looks great.

dabrahams · 2017-01-30T02:47:50Z

on Sun Jan 29 2017, Ben Cohen <notifications-AT-i.8713187.xyz> wrote:

Changes LGTM. There are probably additional tests needed, like to test that slices have the expected properties

There ought to be prepackaged checks for that stuff in StdlibUnittest

…

-- -Dave

DougGregor

This looks fantastic, @jckarter!

DougGregor · 2017-01-30T17:07:58Z

lib/Sema/TypeCheckProtocol.cpp

@@ -2976,7 +2977,54 @@ InferredAssociatedTypesByWitnesses
 ConformanceChecker::inferTypeWitnessesViaValueWitnesses(ValueDecl *req) {
  InferredAssociatedTypesByWitnesses result;

+  auto isExtensionUsableForInference = [&](ExtensionDecl *extension) -> bool {
+    // Assume concrete extensions we found witnesses in are always viable.
+    if (!extension->getExtendedType()->isAnyExistentialType())


The assumption isn't quite true: constrained extensions of concrete types won't be able to provide suitable witnesses, so that return true; could more accurately be:

return !extension->isConstrainedExtension();

for now. When conditional conformances come in, we'll need an "isAtLeastAsSpecializedAs" kind of check.

DougGregor · 2017-01-30T17:08:23Z

lib/Sema/TypeCheckProtocol.cpp

+    //
+    // TODO: This seems like it shouldn't happen, but without this check, we
+    // crash building the standard library.
+    if (!extension->getGenericSignature())


Yeah, it's it's recursion breaker.

DougGregor · 2017-01-30T17:09:38Z

lib/Sema/TypeCheckProtocol.cpp

  for (auto witness : lookupValueWitnesses(req, /*ignoringNames=*/nullptr)) {
+    // If the potential witness came from a protocol extension, and our `Self`


Can drop the "protocol" part of this comment, if you take my suggestion above regarding constrained extensions of concrete types.

DougGregor · 2017-01-30T17:11:37Z

lib/Sema/TypeCheckProtocol.cpp

@@ -3780,6 +3850,154 @@ void ConformanceChecker::resolveTypeWitnesses() {
  // If we (still) have more than one solution, find the one with
  // more-specialized witnesses.
  if (solutions.size() > 1) {
+    auto compareDeclsForInference =


Can you pull this out into a static function? It's pretty large to be inline.

…inference. The associated type inference is a bit unusual, since we want to be able to infer associated types from protocol extension implementations that themselves depend on the types being inferred a certain way, e.g.: ``` protocol Runcible { associatedtype Spoon; func runce(_: Spoon) } extension Runcible where Spoon == Hat { func runce(_ hat: Hat) {} } ``` For this reason, we would collect potential witnesses from all possible extensions of the type without immediately checking the constraints on each extension. This could lead to an inconsistency where, when ranking potential witnesses, associated type inference would end up picking the "best" candidate as something that can't even be used by the type we're inferring the witness for, leading to incomprehensible behavior. The ranking criterion itself is also flawed, because `TC.compareDeclarations` is normally comparing candidates for the same well-typed function invocation, but during associated type inference we haven't even established the type system we're comparing candidates within. Witness candidates from extensions with mutually exclusive associated type constraints but otherwise more specific constraints would be unnecessarily dismissed as ambiguous because neither candidate's generic signature is a match for the other's. Both problems can be mitigated by looking at only the constraints for protocol extensions that specifically constrain the `Self` type, since those conformances are always explicit in the source code and therefore independent of associated type inference. We can pre-filter witness candidates by rejecting candidates for which the conforming type doesn't match the extension's Self constraints. We can score multiple witness candidates by looking only at the relative constraints they put on `Self`, considering a witness from a context that more specifically constrains Self to be a better candidate; we should ignore the associated type constraints for this scoring because we don't even know if they apply given the solution we'll ultimately pick. There's probably a more systemic way to go about this, but targeted fixes for both of these problems, along with a following patch to the standard library, restores source compatibility for Collection conformers, fixing rdar://problem/30228957.

…ex>) for combinations of [Mutable][RangeReplaceable][Bidirectional|RandomAccess]Collection. These were overlooked, and somehow code that attempted to make a minimal collection conform to RangeReplaceableCollection and RandomAccessCollection managed to compile successfully in Swift 3.0, but in Swift 3.1…*something* changed to reject a type that conforms to both due to the lack of a suitable default slicing subscript implementation in the stdlib that provided all the requirements. Fill in these missing implementations, fixing rdar://problem/30228957.

jckarter · 2017-01-30T18:00:52Z

@airspeedswift @dabrahams I went ahead and expanded out the uses of .init shorthand. Not worth holding up this fix on a style debate, IMO.

@DougGregor I also made the type checker changes you requested. How does this look?

jckarter · 2017-01-30T18:01:01Z

@swift-ci Please test

jckarter · 2017-01-30T18:01:09Z

@swift-ci Please test

swift-ci · 2017-01-30T18:12:15Z

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 34ec424
Test requested by - @jckarter

DougGregor

LGTM, thanks!

jckarter · 2017-01-30T22:03:05Z

@swift-ci Please test Linux

rudkx · 2017-01-30T23:49:00Z

Awesome, thanks!

dabrahams · 2017-01-31T19:04:12Z

test/stdlib/collection-combinatorics.swift.gyb

+%   for rangeReplaceable in ['', 'RangeReplaceable']:
+%     for capability in ['', 'Bidirectional', 'RandomAccess']:
+
+struct ${mutable}${rangeReplaceable}${capability}Butt


Put some 👖on that identifier!

jckarter requested review from dabrahams, rudkx, DougGregor and airspeedswift January 30, 2017 00:29

jpsim reviewed Jan 30, 2017

View reviewed changes

jpsim mentioned this pull request Jan 30, 2017

Support Xcode 8.3 beta 1 realm/realm-swift#4586

Closed

airspeedswift reviewed Jan 30, 2017

View reviewed changes

airspeedswift approved these changes Jan 30, 2017

View reviewed changes

dabrahams reviewed Jan 30, 2017

View reviewed changes

test/stdlib/collection-combinatorics.swift.gyb

where C.Iterator.Element == Int

{}

% end

}

Copy link

Contributor

dabrahams Jan 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice thorough testing, Joe!

dabrahams approved these changes Jan 30, 2017

View reviewed changes

DougGregor reviewed Jan 30, 2017

View reviewed changes

jckarter added 2 commits January 30, 2017 09:54

jckarter force-pushed the missing-slice-default-implementations branch from 7774d9e to 34ec424 Compare January 30, 2017 17:55

DougGregor approved these changes Jan 30, 2017

View reviewed changes

jckarter merged commit fd91396 into swiftlang:master Jan 30, 2017

jckarter deleted the missing-slice-default-implementations branch January 31, 2017 01:56

dabrahams reviewed Jan 31, 2017

View reviewed changes

jpsim mentioned this pull request Feb 6, 2017

Fix compilation issues with Xcode 8.3 beta 2 realm/realm-swift#4640

Merged


		// Make sure that solution is better than any of the other solutions

		// Make sure that solution is better than any of the other solutionsa

		for (auto witness : lookupValueWitnesses(req, /ignoringNames=/nullptr)) {
		// If the potential witness came from a protocol extension, and our `Self`

Fix source compatibility problems with conforming to multiple Collection axes at once. #7136

Fix source compatibility problems with conforming to multiple Collection axes at once. #7136

Uh oh!

Conversation

jckarter commented Jan 30, 2017

Uh oh!

jckarter commented Jan 30, 2017

Uh oh!

jckarter commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

swift-ci commented Jan 30, 2017

Uh oh!

airspeedswift commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

airspeedswift left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabrahams left a comment

Choose a reason for hiding this comment

Uh oh!

dabrahams commented Jan 30, 2017 via email

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jan 30, 2017

Uh oh!

jckarter commented Jan 30, 2017

Uh oh!

jckarter commented Jan 30, 2017

Uh oh!

swift-ci commented Jan 30, 2017

Uh oh!

DougGregor left a comment

Choose a reason for hiding this comment

Uh oh!

jckarter commented Jan 30, 2017

Uh oh!

rudkx commented Jan 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!