[SR-12881] DefaultIndices dynamic dispatch #32019

NevinBR · 2020-05-26T15:00:01Z

As seen in SR-12881, DefaultIndices was not properly dispatching certain Collection requirements through conditional conformances to BidirectionalCollection and RandomAccessCollection. This should fix that by adding implementations for these methods at the point where DefaultIndices conforms to Collection:

index(_:offsetBy:)
index(_:offsetBy:limitedBy:)
distance(from:to:)

As seen in SR-12881, `DefaultIndices` was not properly dispatching certain `Collection` requirements through conditional conformances to `BidirectionalCollection` and `RandomAccessCollection`. This should fix that.

stdlib/public/core/Indices.swift

Co-authored-by: Xiaodi Wu <[email protected]>

airspeedswift

This is a good change in principle, but the impact on ABI stability needs to be considered. These maybe need to be always emitted into client, or need availability annotations.

lorentey

This looks good — thanks! 👍
Could you please also add unit tests for the correct behavior?

stdlib/public/core/Indices.swift

Co-authored-by: Karoy Lorentey <[email protected]>

NevinBR · 2020-05-26T20:24:59Z

This looks good — thanks! 👍
Could you please also add unit tests for the correct behavior?

I’d be happy to, but I’ve never added unit tests to Swift before: where should they go, and is there a particular form or style that they should adopt?

lorentey · 2020-05-26T20:49:26Z

I’d be happy to, but I’ve never added unit tests to Swift before: where should they go, and is there a particular form or style that they should adopt?

Most stdlib tests live under test/stdlib/. It looks like we don't have dedicated tests for DefaultIndices yet at all (oops!), so the best course of action is to add a new file test/stdlib/DefaultIndices.swift. You can use the existing tests in the same directory to learn by example -- the template to follow is this:

// RUN: %target-run-stdlib-swift
// REQUIRES: executable_test

import StdlibUnittest

var suite = TestSuite("DefaultIndices")
defer { runAllTests() }

suite.test("Some name") {
  expectEqual(4, 2 + 2)
}

For the most part, the assertion syntax is equivalent to XCTest, with the XCTAssert prefix replaced by expect.

The StdlibCollectionUnittest module contains a bunch of very handy collection validation test generators, including addRandomAccessCollectionTests -- unfortunately they are geared towards collections that can hold arbitrary data, so they aren't directly helpful here. So the most straightforward thing to do is to simply write a few tests that exercise the original issue.

NevinBR · 2020-05-28T16:12:49Z

@airspeedswift, I changed @inlinable to @_alwaysEmitIntoClient per @lorentey’s suggestion. Are there other ABI-related changes that are needed here?

@lorentey, I added unit tests to exercise SR-12881, do they look acceptable?

airspeedswift · 2020-06-05T19:21:10Z

@swift-ci please test

swift-ci · 2020-06-05T19:23:54Z

Build failed
Swift Test Linux Platform
Git Sha - e146d3f

swift-ci · 2020-06-05T19:30:24Z

Build failed
Swift Test OS X Platform
Git Sha - e146d3f

lorentey · 2020-07-13T19:02:45Z

Curiously, these requirements are declared @_nonoverride in BidirectionalCollection, which means they have two different protocol witnesses. (In fact, they have three because RandomAccessCollection does the same.)

So, IIUC, any conforming type that wants to perfectly forward implementations of these to another, user-supplied, collection needs to do this three different times for the (unlikely but not impossible) case that the target collection distinguishes between Collection.index(_:offsetBy:), BidirectionalCollection.index(_:offsetBy:) and RandomAccessCollection.index(_:offsetBy:).

This change still looks right to me. We are currently lacking stdlib tests for this rarely exercised language facility; if it turns out we need to add more code, we'll be able to do it later.

Cc @DougGregor

lorentey · 2020-07-13T23:19:19Z

@swift-ci test and merge

lorentey · 2020-07-14T00:32:59Z

17:14:59 FAILED: source/Target/CMakeFiles/lldbTarget.dir/SwiftLanguageRuntimeDynamicTypeResolution.cpp.o 
17:14:59 /usr/local/bin/sccache /Applications/Xcode-beta.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++  -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -DLLDB_CONFIGURATION_RELEASE -DLLDB_USE_OS_LOG -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Isource/Target -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/source/Target -Isource -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/include -Iinclude -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/llvm/include -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/llvm-macosx-x86_64/include -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/clang/include -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/llvm-macosx-x86_64/tools/clang/include -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/buildbot_incremental/swift-macosx-x86_64/include -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/swift/include -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/source -I/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.16.sdk/usr/include/python2.7 -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/tools/clang/include -I../clang/include -I/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.16.sdk/usr/include/libxml2 -I/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/source/. -Wno-unknown-warning-option -Werror=unguarded-availability-new -arch x86_64  -fno-stack-protector -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -w -fdiagnostics-color -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3  -isysroot /Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.16.sdk -mmacosx-version-min=10.11   -UNDEBUG  -fno-exceptions -fno-rtti -std=c++14 -MD -MT source/Target/CMakeFiles/lldbTarget.dir/SwiftLanguageRuntimeDynamicTypeResolution.cpp.o -MF source/Target/CMakeFiles/lldbTarget.dir/SwiftLanguageRuntimeDynamicTypeResolution.cpp.o.d -o source/Target/CMakeFiles/lldbTarget.dir/SwiftLanguageRuntimeDynamicTypeResolution.cpp.o -c /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp
17:14:59 
/Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/source/Target/SwiftLanguageRuntimeDynamicTypeResolution.cpp:642:31: error: no viable conversion from 'llvm::Optional<SwiftASTContextReader>' to 'lldb_private::SwiftASTContextReader'
17:14:59     if (SwiftASTContextReader reader = instance->GetScratchSwiftASTContext())
17:14:59                               ^        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
17:14:59 /Users/buildnode/jenkins/workspace/swift-PR-osx/branch-master/llvm-project/lldb/include/lldb/Core/SwiftASTContextReader.h:103:3: note: candidate constructor not viable: no known conversion from 'llvm::Optional<SwiftASTContextReader>' to 'const lldb_private::SwiftASTContextReader &' for 1st argument
17:14:59   SwiftASTContextReader(const SwiftASTContextReader &copy)
17:14:59   ^
17:14:59 1 error generated.

lorentey · 2020-07-14T00:43:48Z

@vedantk It looks like swiftlang/llvm-project#1442 got automerged to llvm-project/master, but it doesn't work there.

lorentey · 2020-07-14T02:01:25Z

This will be resolved by swiftlang/llvm-project#1446.

lorentey · 2020-07-14T02:59:03Z

@swift-ci test

lorentey · 2020-07-14T03:08:11Z

@swift-ci test

vedantk · 2020-07-14T16:47:40Z

@lorentey sorry about that, I wasn't aware of the fact that swift/release/5.3 auto-merges into swift/master for llvm-project. I'll keep this in mind going forward.

[SR-12881] DefaultIndices dynamic dispatch

2857ec9

As seen in SR-12881, `DefaultIndices` was not properly dispatching certain `Collection` requirements through conditional conformances to `BidirectionalCollection` and `RandomAccessCollection`. This should fix that.

theblixguy requested a review from airspeedswift May 26, 2020 15:48

xwu reviewed May 26, 2020

View reviewed changes

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

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

NevinBR and others added 2 commits May 26, 2020 12:45

Update stdlib/public/core/Indices.swift

55b1a4d

Co-authored-by: Xiaodi Wu <[email protected]>

Update stdlib/public/core/Indices.swift

e99b646

Co-authored-by: Xiaodi Wu <[email protected]>

airspeedswift requested changes May 26, 2020

View reviewed changes

lorentey requested changes May 26, 2020

View reviewed changes

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

NevinBR and others added 2 commits May 26, 2020 16:22

Update stdlib/public/core/Indices.swift

6ce51ed

Co-authored-by: Karoy Lorentey <[email protected]>

Changed @inlinable to @_alwaysEmitIntoClient

e146d3f

Created unit tests for DefaultIndices

4bb855b

lorentey approved these changes Jul 13, 2020

View reviewed changes

lorentey merged commit 40d7da4 into swiftlang:master Jul 14, 2020

NevinBR deleted the patch-1 branch July 26, 2020 06:06

NevinBR mentioned this pull request May 26, 2020

[SR-12881] DefaultIndices conditional conformances incomplete #55328

Closed

[SR-12881] DefaultIndices dynamic dispatch #32019

[SR-12881] DefaultIndices dynamic dispatch #32019

Uh oh!

Conversation

NevinBR commented May 26, 2020

Uh oh!

Uh oh!

Uh oh!

airspeedswift left a comment

Choose a reason for hiding this comment

Uh oh!

lorentey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NevinBR commented May 26, 2020

Uh oh!

lorentey commented May 26, 2020

Uh oh!

NevinBR commented May 28, 2020

Uh oh!

airspeedswift commented Jun 5, 2020

Uh oh!

swift-ci commented Jun 5, 2020

Uh oh!

swift-ci commented Jun 5, 2020

Uh oh!

lorentey commented Jul 13, 2020

Uh oh!

lorentey commented Jul 13, 2020

Uh oh!

lorentey commented Jul 14, 2020

Uh oh!

lorentey commented Jul 14, 2020

Uh oh!

lorentey commented Jul 14, 2020

Uh oh!

lorentey commented Jul 14, 2020

Uh oh!

lorentey commented Jul 14, 2020

Uh oh!

vedantk commented Jul 14, 2020

Uh oh!

Uh oh!