adopt swift-collections #3595

tomerd · 2021-07-06T23:55:12Z

motivation: replace TSC versions of ordered collections with the new ones from swift-collections

changes:

pull in swift-collections as a dependency
update bootstrap script
adapt call-sites to swift-collections

tomerd · 2021-07-06T23:56:28Z

@swift-ci please smoke test

Utilities/bootstrap

CMakeLists.txt

tomerd · 2021-07-07T00:47:04Z

CMakeLists.txt

@@ -53,6 +53,8 @@ if(FIND_PM_DEPS)
  find_package(ArgumentParser CONFIG REQUIRED)
  find_package(SwiftCrypto CONFIG REQUIRED)
  find_package(SwiftDriver CONFIG REQUIRED)
+  # FIXME: need to change in SwiftCollections (swift-collections -> SwiftCollections)


cc @compnerd @lorentey

apple/swift-collections#60

tomerd · 2021-07-07T00:47:16Z

Utilities/bootstrap

@@ -591,6 +604,9 @@ def build_swiftpm_with_cmake(args):
        "-DArgumentParser_DIR=" + os.path.join(args.swift_argument_parser_build_dir, "cmake/modules"),
        "-DSwiftDriver_DIR=" + os.path.join(args.swift_driver_build_dir, "cmake/modules"),
        "-DSwiftCrypto_DIR=" + os.path.join(args.swift_crypto_build_dir, "cmake/modules"),
+        # FIXME: need to change in SwiftCollections (swift-collections -> SwiftCollections)


cc @compnerd @lorentey

tomerd · 2021-07-07T00:48:12Z

simple alternative #3590

cc @WowbaggersLiquidLunch @neonichu

tomerd · 2021-07-07T00:48:21Z

@swift-ci please smoke test

tomerd · 2021-07-07T01:38:44Z

manager-PR-macos-smoke-test/branch-main/swift-collections/Sources/DequeModule/Deque+RangeReplaceableCollection.swift /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swift-collections/Sources/DequeModule/Deque+Sequence.swift /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swift-collections/Sources/DequeModule/Deque+Testing.swift /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swift-collections/Sources/DequeModule/UnsafeMutableBufferPointer+Utilities.swift  -Xlinker -install_name -Xlinker @rpath/libDequeModule.dylib    && :
18:18:01 
/Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swift-collections/Sources/DequeModule/Deque+Extras.swift:134:35: error: value of type 'C' has no member '_withContiguousStorageIfAvailable_SR14663'
18:18:01     let done: Void? = newElements._withContiguousStorageIfAvailable_SR14663 { source in
18:18:01                       ~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18:18:01 /Users/buildnode/jenkins/workspace/swift-package-manager-PR-macos-smoke-test/branch-main/swift-collections/Sources/DequeModule/Deque+Extras.swift:169:35: error: value of type 'S' has no member '_withContiguousStorageIfAvailable_SR14663'
18:18:01     let done: Void? = newElements._withContiguousStorageIfAvailable_SR14663 { source in
18:18:01                       ~~~~~~~~~~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18:18:04 [2/3][ 66%][2.727s] Linking Swift shared library lib/libOrderedCollections.dylib
18:18:04 ninja: build stopped: subcommand failed.

@lorentey @shahmishal do you know what this is about? maybe toolchain too old/new?

WowbaggersLiquidLunch · 2021-07-07T19:37:18Z

Sources/Basics/Exports.swift

+import OrderedCollections
+
+public typealias OrderedDictionary = OrderedCollections.OrderedDictionary
+public typealias OrderedSet = OrderedCollections.OrderedSet


What's the benefit of this file (no @_exported for the import compared to #3590) over manually importing OrderedCollections in each file where it's used?

good point. @neonichu @abertelrud do you think re-exporting common modules like swift-collections, swift-system would make sense, or prefer individual imports? I am on the fence.

Personally not a fan of re-exporting, it might also hurt incremental builds?

I strongly prefer individual imports, to make it clear what's being used. Tooling should ideally help add those where needed (the effort of having to add the individual imports is usually the main argument against it). So I agree with neonichu on this.

It also makes it clear that the standard ordered collections are being used rather than something specific to TSC.

Unless I'm reading it wrong, it seems like the general opinion is not to import OrderedCollections via Basics, but to import it individually in files that need it. If this is the case, @tomerd could you remove this Exports file and import OrderedCollections individually before this PR is merged?

Utilities/bootstrap

This is an integration FIXME for SwiftPM: swiftlang/swift-package-manager#3595 (comment)

* [CMake] swift-collections → SwiftCollections This is an integration FIXME for SwiftPM: swiftlang/swift-package-manager#3595 (comment) * [CMake] Generate exports under cmake/modules/SwiftCollectionsConfig.cmake

tomerd · 2021-07-09T02:25:52Z

@swift-ci please smoke test

motivation: replace TSC versions of ordered collections with the new ones from swift-collections changes: * pull in swift-collections as a dependency * update bootstrap script * adapt call-sites to swift-collections

tomerd · 2021-07-12T17:04:56Z

@swift-ci smoke test

tomerd · 2021-07-13T04:46:10Z

@swift-ci please smoke test

tomerd · 2021-07-13T16:39:36Z

@neonichu @abertelrud @WowbaggersLiquidLunch this should be ready to go.

@WowbaggersLiquidLunch do you need help getting swiftlang/swift-tools-support-core#222 over the finish line? once it is merged we can remove the aliases here

WowbaggersLiquidLunch · 2021-07-13T19:14:48Z

@WowbaggersLiquidLunch do you need help getting swiftlang/swift-tools-support-core#222 over the finish line?

Yes, some help would be great. It's stuck on cmake. I've been learning cmake and trying to figure out if TSC needs a bootstrap script like SwiftPM and Swift Driver, and I haven't made any progress on it so far.

neonichu · 2021-07-13T20:31:45Z

TSC does not have a bootstrap script, the easiest way to test the cmake build on macOS in isolation is:

$ cmake -G Ninja .
$ ninja

tomerd · 2021-07-13T20:58:37Z

TSC does not have a bootstrap script, the easiest way to test the cmake build on macOS in isolation is:

what does CI use? I have been working on helping here by expanding build-script-helper.py since with a dependency we now need more than just simple cmake

neonichu · 2021-07-13T21:37:56Z

what does CI use?

CI runs the same presets as SwiftPM: ./swift/utils/build-script "--preset=buildbot_swiftpm_linux_platform,tools=RA,stdlib=RA" on Linux and ./swift/utils/build-script "--preset=buildbot_swiftpm_macos_platform,tools=RA,stdlib=RA" on macOS.

friedbunny · 2021-07-19T21:10:31Z

@swift-ci please smoke test

This reverts commit 0b0ac51.

This reverts commit c61078f.

* Revert "Revert "adopt swift-collections (#3595)" (#3628)" This reverts commit c61078f. * Update CONTRIBUTING.md * Update Package.swift

* Create Screen Shot 2021-05-17 at 7.15.09 PM.png * Delete Screen Shot 2021-05-17 at 7.15.09 PM.png * Create BitArray.swift * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * Added BitArray as a product to the package * Update Package.swift * Update Package.swift * Update Package.swift * Update BitArray.swift * Update Package.swift * Update Collections.swift * Update Collections.swift * Update Package.swift * Create BitArray.xcscheme * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * build: add a CMake based build system (#42) Add a CMake based build system to enable bootstrapping the toolchain with the ability to use Swift Collections in SPM. * Update BitArray.swift * Update BitArray.swift * git: ignore vim swap files (#41) * Initial conformation to Collection. Lots of faulty code right now * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * Update BitArray.swift * [Deque] Work around stdlib issue with Array.withContiguousStorageIfAvailable (#44) Resolves #27. * [manifest] Ignore CMake files (#47) The package manager doesn’t like it when it finds unexpected non-source files in source directories. * [meta] Update dependencies (#48) (This only affects local development.) * [Deque][NFC] Remove unused code (#49) * Changes to support OpenBSD. (#52) * [cmake] Support amd64 architecture name. On OpenBSD, "x86_64" is called "amd64", and therefore recognition of this name is required for the cmake build. * [cmake] Add missing file. This is required since, for example, `Deque+Extras` requires `_withContiguousStorageIfAvailable_SR14663`. * ManagedBuffer.capacity is unavailable on OpenBSD. ManagedBuffer requires nonstandard API to implement `capacity` and subsequently is unavailable on OpenBSD. For portability, just use `minimumCapacity` on this platform. * Fixed subscript to have bit manipulations * Update BitArray.swift * Update BitArray.swift * Moved Collection Conformance into appropriate file * Implements some bit manip * Fixed issue with endIndex resetting when storage element is full When an element of the storage array in the BitArray filled, the endIndex would reset to 0, but then would be the correct value at any other time. This was due to a calculation error in endIndex * Update invalid OrderedDictionary invariant check. (#53) * Added XCTests, MutableCollection Conformance, Fixed endIndex bug MutableCollection conformance still has bugs to be fixed Additionally, the code is still very rough endIndex had an issue where it was 0 when the array was empty. Fixed this by switching the code for count and endIndex, and making endIndex = 1 when count is 0 * Update BitArrayTests.swift * Fixed subscript bug Wouldn't work sometimes when current value was true because the return for `storage[index] & mask` could be anything >= 1, and not == 1, hence resulting in a unwanted toggle of the value. Fixed by comparing a boolean value of the current value to the boolean value of the newValue instead of the integers * Code clean up ish * Fixing a small typo on quick help documentation. (#55) * Modifying Existing Code From Feedback - `endIndex` corrected - changed `remaining` in `count` to use ternary operator instead of being a computed local variable - added `_split` as a helper function to `subscript` - modified `set{...}` portion of `subscript` and included comments of the different options - removed `clear()` as `removeAll()` seems to already exist somewhere - modified unit tests to have a new BitArray for every test instead of a shared one - Improved `testSubscriptSetOnly()` to hopefully be more thorough * Added Bitwise Operations (No Tests) Both mutable and immutable OR, AND, XOR operations * Mark OrderedSetDiffingTests with availability (#54) * Added Rough Bitwise Operation UnitTests * Added BitSet Files Did not add methods as I know a lot will go in BitStorage, which Im still figuring out * Beginning implementations for BitSet * Mutating Bitwise Operations Performance-UNfriendly starter implementations * Fixed subscript set mask for the &= operation and changed fatalErrors to preconditions changed mask on &= operation from 0 to ~mask * Also corrected subscript set test to fail if mask of 0 was used (instead of ~mask) * Implemented BitSet with BitArray storage type and some tests Tests are not yet passing. Pushing early so mentor can see progress So sorry for waiting to push so late! * Fixed error in first BitSet test ( which is testAppend() ) so it passes! * BitSet formUnion initial implementation * formUnion() test added and implementation modified to pass test formUnion() is still incomplete and needs more tests atm * Finished formUnion() for BitSet * Modified formUnion() to use bit shifts instead * Initial formIntersection function for BitSet Untested * Added an array view for BitSet, fixed BitSet.count bug, added and modified tests * Initial Cartesian Product implementation and unfinished test * Update BitArrayConformances.swift * [CMake] swift-collections → SwiftCollections (#60) * [CMake] swift-collections → SwiftCollections This is an integration FIXME for SwiftPM: swiftlang/swift-package-manager#3595 (comment) * [CMake] Generate exports under cmake/modules/SwiftCollectionsConfig.cmake * Work around another MergeModules crash (#62) * [utils] Add utility for shuffling sources to help reproduce MergeModules issues * [Deque] Collapse Sequence/Collection conformances into a single file The MergeModules phase in the compiler is broken: it isn’t always able to correctly handle circular dependencies across source files, like the ones that keep organically popping up with Sequence/Collection conformances that are defined in separate files. Such dependencies sometimes triggers a compiler crash, depending on some (unknown) environmental parameters. Trying to manually find and eliminate these circular dependencies has not proved productive. Instead, roll all Collection conformances into a single, huge source file. * [Deque] Update CMakeLists * [test] checkCollection: Don’t pass decreasing indices to distance(from:to:) (#63) Also, try running the conformance checkers on the minimal collection types to catch similar issues. * Attempting to make an Index * First steps in implementing Index struct for BitSet * Fixed up endIndex * Last tweaks of tonight Tomorrow: fix up Index implementations, SetAlgebra, Unit Testing * Adding index function preconditions * Altered index(_ index: Index, offsetBy distance: Int) -> Index * Update BitSetConformances.swift * Update BitSetBitwiseOperations.swift * Time for SetAlgebra! * Transitioning to using CollectionsTestSupport * Conformance to Set Algebra (without bringing in bitarray methods) * Overloaded Bitwise Operators on BitArray * Tilday (bitwiseNOT operations) * Fixed coding style - 2 space indents * Initial implementation od initializers Bug found * Fixed count bug * Completed and XCTested BitArray initializers By"XCTested" I mean using my own XCTests without the thorough CollectionsTestSupport * Update BitArray.swift * BitSet Initializers -- XCTested By "XCTested" I mean not thoroughly tested with the CollectionsTestSupport methods * BitSet's "custom" SetAlgebra * contains(Int) and dropExcessFalses() methods for BitSet * Consistently use UNIT.bitWidth instead of the number 8 This will be useful for benchmarking purposes * Modified BitArray.swift according to feedback Also added more typealias consistencies (using `UNIT.max` instead of hardcoded 255) * Modified BitArray bitwise operations according to feedback Added overloaded mutating operators * Modified BitArrayConformances.swift according to feedback * Modified BitArrayOperations.swift according to feedbacl * Adjust Bitwise Operations' naming to be consistent with that of the atomics package * . * Modified According to Feedback * testing * First working test using CollectionsTestSupport Testing sequence initializer * All initializer tests for BitArray (except using BitSet) * Update BitArrayTests.swift * BitArray Append test * Improved BitArray append() test to test multiple appends * Untested remove functions Also updated any UInt8 usages to use the UNIT typealias instead * BitArray removeLast() Test * Update BitArrayTests.swift * BitArray removeAll() test * BitArray removeLast(_ rangeSize: Int) function test and bug fix * BitArray removet(at: Int) tests Also added test for discardable results for current and existing remove tests * BitArray removeFirst(_ rangeSize: Int) test implemented and passing Current method has terrible run time for now as I can't get the optimized version to work yet * BitArray first and last true index functions TEST and BUG FIX firstTrueIndex() lastTrueIndex() * Failing OR and AND bitwise operations test on BitArray * Update BitArrayTests.swift * Update BitArrayTests.swift * Update BitArrayTests.swift * PUSHHHHH * removeFirst(_ rangeSize: Int) optimization/bugFix * BitArray bitwise operations tests and bug fixes * BitSet tests, bug fixes, and name changes sequence init test and bug fix change "UNIT" to "WORD" * Update BitArrayTests.swift * add BitSet layout to test support * BitSet testExpressibleByArrayLiteralAndArrayLiteralInit() * testExpressibleByArrayLiteralAndArrayLiteralInit() * BitSet insert and contains functions test + bug fixes * forceInsert test for BitSet * BitSet remove function test and improvements * Update BitSetSetAlgebra.swift * BitSet intersection tests and bug fixes * BitSet symmetricDifference test and bug fix * starting index test and found interesting bug... * BitArray init(BitSet) test * Update BitSetTests.swift * patch * bug fix in BitSet index after * BitSet index before test * Clean up unneeded code * Delete BitArrayBenchmarks.swift * My added, not-working benchmark * BitSet init from BitArray benchmark * BiSet insert benchmark * BitSet forceInsert benchmark * Passing BitSet index offset by test I plan to add more to the test to test offsets from indices in the middle and not just start and end * BitArray testing modification to accommodate varying WORD sizes * Minor comment clean-up * Correcting bitset tests for varying WORD sizes * Headers with License info * simple set operation comparison incomplete * . * adjustments * Update BitSetOperations.swift * Benchmarks and preconditions * Update BitArrayOperations.swift * Update BitSetTests.swift * quick edits * . * Documentation for bitwise operations and some initializers * Testing `==` operator to ensure bug fix * BitSet SetAlgebra and Operations Documentation * Update BitSetOperations.swift Co-authored-by: Saleem Abdulrasool <compnerd@compnerd.org> Co-authored-by: Karoy Lorentey <klorentey@apple.com> Co-authored-by: 3405691582 <dsk@google.com> Co-authored-by: Vihan <vihan_bhargava@apple.com> Co-authored-by: Rodrigo Kreutz <8869678+rkreutz@users.noreply.github.com> Co-authored-by: Andrew Monshizadeh <1282845+amonshiz@users.noreply.github.com>

tomerd requested review from abertelrud, friedbunny and neonichu as code owners July 6, 2021 23:55

compnerd reviewed Jul 7, 2021

View reviewed changes

Utilities/bootstrap Outdated Show resolved Hide resolved

compnerd reviewed Jul 7, 2021

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

tomerd commented Jul 7, 2021

View reviewed changes

tomerd mentioned this pull request Jul 7, 2021

adopt swift-collections #3590

Closed

neonichu approved these changes Jul 7, 2021

View reviewed changes

WowbaggersLiquidLunch reviewed Jul 7, 2021

View reviewed changes

Utilities/bootstrap Outdated Show resolved Hide resolved

lorentey added a commit to apple/swift-collections that referenced this pull request Jul 7, 2021

[CMake] swift-collections → SwiftCollections

b0402d3

This is an integration FIXME for SwiftPM: swiftlang/swift-package-manager#3595 (comment)

lorentey mentioned this pull request Jul 7, 2021

[CMake] Update config to prepare for SwiftPM adoption apple/swift-collections#60

Merged

7 tasks

tomerd self-assigned this Jul 7, 2021

WowbaggersLiquidLunch mentioned this pull request Jul 8, 2021

[Gardening] Add missing "clone TSC" step to bootstrap instructions and reformat existing steps #3598

Merged

tomerd force-pushed the refactor/swift-colections-2 branch from 82b24ca to 0b55ada Compare July 9, 2021 02:25

adopt swift-collections

ca93005

motivation: replace TSC versions of ordered collections with the new ones from swift-collections changes: * pull in swift-collections as a dependency * update bootstrap script * adapt call-sites to swift-collections

tomerd force-pushed the refactor/swift-colections-2 branch from 0b55ada to ca93005 Compare July 12, 2021 17:04

tomerd mentioned this pull request Jul 12, 2021

update swift-collections to 0.0.4 swiftlang/swift#38312

Merged

tomerd added the ready Author believes the PR is ready to be merged & any feedback has been addressed label Jul 13, 2021

tomerd mentioned this pull request Jul 15, 2021

Adopt Swift Collections' OrderedSet and OrderedDictionary swiftlang/swift-tools-support-core#222

Open

tomerd merged commit 0b0ac51 into swiftlang:main Jul 20, 2021

compnerd mentioned this pull request Jul 21, 2021

build: add the ability to pass in SwiftCollections swiftlang/sourcekit-lsp#419

Merged

tomerd added a commit that referenced this pull request Jul 21, 2021

Revert "adopt swift-collections (#3595)"

9dbb9a0

This reverts commit 0b0ac51.

tomerd mentioned this pull request Jul 21, 2021

Revert "adopt swift-collections" #3628

Merged

tomerd added a commit that referenced this pull request Jul 22, 2021

Revert "adopt swift-collections (#3595)" (#3628)

c61078f

This reverts commit 0b0ac51.

tomerd added a commit that referenced this pull request Jul 22, 2021

Revert "Revert "adopt swift-collections (#3595)" (#3628)"

b7515b8

This reverts commit c61078f.

tomerd mentioned this pull request Jul 22, 2021

adopt swift-collections #3632

Merged

shahmishal pushed a commit that referenced this pull request Aug 18, 2021

adopt swift-collections (#3632)

2019464

* Revert "Revert "adopt swift-collections (#3595)" (#3628)" This reverts commit c61078f. * Update CONTRIBUTING.md * Update Package.swift

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adopt swift-collections #3595

adopt swift-collections #3595

tomerd commented Jul 6, 2021

tomerd commented Jul 6, 2021

tomerd Jul 7, 2021

lorentey Jul 7, 2021

tomerd Jul 7, 2021

tomerd commented Jul 7, 2021

tomerd commented Jul 7, 2021

tomerd commented Jul 7, 2021

WowbaggersLiquidLunch Jul 7, 2021 •

edited

Loading

tomerd Jul 7, 2021

neonichu Jul 7, 2021

abertelrud Jul 12, 2021

abertelrud Jul 12, 2021

WowbaggersLiquidLunch Jul 13, 2021 •

edited

Loading

tomerd commented Jul 9, 2021

tomerd commented Jul 12, 2021

tomerd commented Jul 13, 2021

tomerd commented Jul 13, 2021

WowbaggersLiquidLunch commented Jul 13, 2021 •

edited

Loading

neonichu commented Jul 13, 2021

tomerd commented Jul 13, 2021

neonichu commented Jul 13, 2021

friedbunny commented Jul 19, 2021

adopt swift-collections #3595

adopt swift-collections #3595

Conversation

tomerd commented Jul 6, 2021

tomerd commented Jul 6, 2021

tomerd Jul 7, 2021

Choose a reason for hiding this comment

lorentey Jul 7, 2021

Choose a reason for hiding this comment

tomerd Jul 7, 2021

Choose a reason for hiding this comment

tomerd commented Jul 7, 2021

tomerd commented Jul 7, 2021

tomerd commented Jul 7, 2021

WowbaggersLiquidLunch Jul 7, 2021 • edited Loading

Choose a reason for hiding this comment

tomerd Jul 7, 2021

Choose a reason for hiding this comment

neonichu Jul 7, 2021

Choose a reason for hiding this comment

abertelrud Jul 12, 2021

Choose a reason for hiding this comment

abertelrud Jul 12, 2021

Choose a reason for hiding this comment

WowbaggersLiquidLunch Jul 13, 2021 • edited Loading

Choose a reason for hiding this comment

tomerd commented Jul 9, 2021

tomerd commented Jul 12, 2021

tomerd commented Jul 13, 2021

tomerd commented Jul 13, 2021

WowbaggersLiquidLunch commented Jul 13, 2021 • edited Loading

neonichu commented Jul 13, 2021

tomerd commented Jul 13, 2021

neonichu commented Jul 13, 2021

friedbunny commented Jul 19, 2021

WowbaggersLiquidLunch Jul 7, 2021 •

edited

Loading

WowbaggersLiquidLunch Jul 13, 2021 •

edited

Loading

WowbaggersLiquidLunch commented Jul 13, 2021 •

edited

Loading