[stdlib] prevent MutableCollections from inappropriately inheriting a Slice<Self> subscript #38509

glessard · 2021-07-20T19:36:54Z

adds a conditional protocol extension that defines MutableCollection.MutableCollection.subscript(bounds: Range<_>) -> Slice<Self> only when SubSequence == Slice<Self>.
attempts to mark the unconditional extension method MutableCollection.subscript(bounds: Range<_>) -> Slice<Self> as unavailable
MutableCollections where SubSequence == Slice<Self> still get the expected default implementation.
adds a default implementation of MutableCollection.subscript(bounds: Range<_>) -> SubSequence, marked unavailable, in order to prevent invalid conformances from compiling when SubSequence is not Slice<Self>.

Resolves SR-14850 / rdar://79898408 (and completes the fix to SR-14848).

- add a default implementation of MutableCollection’s subscript(bounds: Range<_>) with the most general signature possible - it is marked unavailable in order to prevent the infinite recursion bug reported in SR-14848 - add a conditionally-available subscript(bounds: Range<_>) -> Slice<Self> only when Subsequence is Slice<Self>. This will supersede the unconditional extension that provides the same signature.

glessard · 2021-07-20T23:49:50Z

Is there a wizard of availability annotations to help figure out what's needed in the last of these commits?

stdlib/public/core/MutableCollection.swift

glessard · 2021-07-21T15:22:36Z

@swift-ci please smoke test

glessard · 2021-07-21T22:27:01Z

@slavapestov I'd like your opinion on availability annotation and ABI impact of these changes.

glessard · 2021-07-22T17:29:25Z

@swift-ci please smoke test

glessard · 2021-07-22T23:00:37Z

@swift-ci please smoke test

glessard · 2021-07-23T03:26:36Z

@swift-ci please smoke test

glessard · 2021-07-23T16:12:08Z

@swift-ci please smoke test

stdlib/public/core/MutableCollection.swift

glessard · 2021-07-23T19:42:57Z

@swift-ci please smoke test macOS platform

glessard · 2021-07-23T21:22:22Z

As it is, this change is tripping up the api-digester test. I believe that's the not-breaking-things test, so an adjustment is clearly needed.

lorentey · 2021-07-28T04:42:56Z

The source/abi compatibility tests aren't perfect -- they sometimes report false positives. We also know that this is technically a source breaking change, so I'd expect we'll need to add at least a couple new expected differences.

Source compat diff

--- /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/api-digester/Output/stability-stdlib-source.swift.tmp.tmp/stability-stdlib-source.swift.expected	2021-07-23 13:23:30.000000000 -0700
+++ /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/api-digester/Output/stability-stdlib-source.swift.tmp.tmp/changes.txt.tmp	2021-07-23 13:23:30.000000000 -0700
@@ -1,4 +1,7 @@
+Accessor MutableCollection.subscript(_:).Get() has generic signature change from <Self where Self : Swift.MutableCollection> to <Self where Self : Swift.MutableCollection, Self.SubSequence == Swift.Slice<Self>>
+Accessor MutableCollection.subscript(_:).Set() has generic signature change from <Self where Self : Swift.MutableCollection> to <Self where Self : Swift.MutableCollection, Self.SubSequence == Swift.Slice<Self>>
+Subscript MutableCollection.subscript(_:) has generic signature change from <Self where Self : Swift.MutableCollection> to <Self where Self : Swift.MutableCollection, Self.SubSequence == Swift.Slice<Self>>

These are good -- the test correctly caught that we added an extra constraint on the Slice-returning subscript. It is a source compat break we've accepted -- so we'll just need to add these three lines to the expected failure list.

ABI diffs

--- /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/api-digester/Output/stability-stdlib-abi-with-asserts.test.tmp.tmp/stability-stdlib-abi.swift.expected.sorted	2021-07-23 13:23:31.000000000 -0700
+++ /Users/buildnode/jenkins/workspace/swift-PR-macos-smoke-test/branch-main/buildbot_incremental/swift-macosx-x86_64/test-macosx-x86_64/api-digester/Output/stability-stdlib-abi-with-asserts.test.tmp.tmp/changes.txt.tmp	2021-07-23 13:23:31.000000000 -0700
@@ -1,3 +1,4 @@
+Accessor _SmallString.subscript(_:).Modify() is a new API without @available attribute

This one is interesting -- it looks like the compiler generated an implicit _modify accessor for the smol string subscript. I did not know the compiler did that!

If true, then this is indeed an ABI break that needs to be fixed. A quick patch is to add an explicit _modify with an @_alwaysEmitIntoClient attribute.

glessard · 2021-07-28T08:15:32Z

@swift-ci please smoke test macOS platform

glessard · 2021-07-28T14:18:49Z

@swift-ci please smoke test

glessard · 2021-07-28T15:32:17Z

@swift-ci please smoke test

glessard · 2021-07-28T16:29:54Z

@swift-ci please smoke test macOS platform

glessard · 2021-07-28T18:32:21Z

@swift-ci please smoke test Linux platform

glessard · 2021-07-28T18:34:14Z

@lorentey Thanks for confirming I wasn't breaking anything I shouldn't break!

glessard added 3 commits July 20, 2021 13:25

[test] add a definition that shouldn't compile

473b573

[stdlib] MutableCollection fix to _SmallString

50e7c98

glessard force-pushed the sr14850 branch from d8d7060 to 797cd71 Compare July 20, 2021 23:40

lorentey reviewed Jul 21, 2021

View reviewed changes

stdlib/public/core/MutableCollection.swift Show resolved Hide resolved

lorentey reviewed Jul 21, 2021

View reviewed changes

stdlib/public/core/MutableCollection.swift Outdated Show resolved Hide resolved

lorentey reviewed Jul 21, 2021

View reviewed changes

stdlib/public/core/MutableCollection.swift Outdated Show resolved Hide resolved

[stdlib] add availability annotations and tests

0ea4e3c

glessard force-pushed the sr14850 branch from 797cd71 to 0ea4e3c Compare July 21, 2021 15:20

glessard changed the title ~~[wip][stdlib] prevent MutableCollections from always inheriting a Slice<Self> subscript~~ [wip][stdlib] prevent MutableCollections from inappropriately inheriting a Slice<Self> subscript Jul 21, 2021

glessard changed the title ~~[wip][stdlib] prevent MutableCollections from inappropriately inheriting a Slice<Self> subscript~~ [stdlib] prevent MutableCollections from inappropriately inheriting a Slice<Self> subscript Jul 21, 2021

glessard requested a review from slavapestov July 21, 2021 22:25

[test] a previously-emitted note no longer is after the SR-14850 fix.

5b0d039

lorentey reviewed Jul 23, 2021

View reviewed changes

stdlib/public/core/MutableCollection.swift Outdated Show resolved Hide resolved

[stdlib] fix availability annotation

53db35c

[stdlib] More MutableCollection fix to _SmallString

9ccf71e

glessard force-pushed the sr14850 branch from 5451d3e to 0f0dcbd Compare July 28, 2021 15:31

[test] list expected changes to MutableCollection

63aef41

glessard force-pushed the sr14850 branch from 0f0dcbd to 63aef41 Compare July 28, 2021 16:29

glessard merged commit 1e6b758 into swiftlang:main Aug 2, 2021

glessard deleted the sr14850 branch August 2, 2021 20:12

This was referenced Aug 4, 2021

add missing setters to complete implementation of MutableCollection apple/swift-nio#1925

Merged

update swift-nio swiftlang/swift-source-compat-suite#568

Merged

glessard mentioned this pull request Aug 2, 2021

[SR-14850] MutableCollection gives wrong Subsequence type to implementor #57197

Closed

[stdlib] prevent MutableCollections from inappropriately inheriting a Slice<Self> subscript #38509

[stdlib] prevent MutableCollections from inappropriately inheriting a Slice<Self> subscript #38509

Uh oh!

Conversation

glessard commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glessard commented Jul 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glessard commented Jul 21, 2021

Uh oh!

glessard commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glessard commented Jul 22, 2021

Uh oh!

glessard commented Jul 22, 2021

Uh oh!

glessard commented Jul 23, 2021

Uh oh!

glessard commented Jul 23, 2021

Uh oh!

Uh oh!

glessard commented Jul 23, 2021

Uh oh!

glessard commented Jul 23, 2021

Uh oh!

lorentey commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Source compat diff

ABI diffs

Uh oh!

glessard commented Jul 28, 2021

Uh oh!

glessard commented Jul 28, 2021

Uh oh!

glessard commented Jul 28, 2021

Uh oh!

glessard commented Jul 28, 2021

Uh oh!

glessard commented Jul 28, 2021

Uh oh!

glessard commented Jul 28, 2021

Uh oh!

Uh oh!

glessard commented Jul 20, 2021 •

edited

Loading

glessard commented Jul 21, 2021 •

edited

Loading

lorentey commented Jul 28, 2021 •

edited

Loading