pointfreeco / swift-identified-collections

A library of data structures for working with collections of identifiable elements in an ergonomic, performant way.
MIT License
531 stars 45 forks source link

Support OS versions below iOS 13, macOS 10.15 etc. #8

Closed mluisbrown closed 2 years ago

mluisbrown commented 3 years ago

Since this repo is now a dependency of TCA, and I have been updating RAS-TCA to match, it forces me to remove support for older Apple OSs for RAS-TCA as well 😢

I tried various different approaches to try and make IdentifiedCollections support the older OSes without success.

If you have any ideas on how that might work I'd be interested 😄

In the meantime, I might have to keep the "old" IdentifiedArray in RAS-TCA until a better solution can be found.

stephencelis commented 3 years ago

This is unfortunately due to how Swift Collections is distributed 😕. Its Package.swift file is locked to swift-package-tools: 5.3, which means older versions of SPM don't see it. I wonder if Apple would be open to relaxing their constraint and adding Package@swift-5.1.swift, etc., files in order to support older versions, and then we could totally support these versions, as well. Would you be up for filing an issue with them and share it here?

In the meantime, because you're maintaining RAS-TCA, and the dependency is at the level of TCA and not another downstream dependency, I think you could continue to ship the old IdentifiedArray directly inside the library. I know this isn't ideal, but it'd at least unblock you for now.

mluisbrown commented 3 years ago

Its Package.swift file is locked to swift-package-tools: 5.3, which means older versions of SPM don't see it

That's actually not the problem, as I'm using package tools 5.2, and the error I get is:

The package product 'IdentifiedCollections' requires minimum platform version 13.0 for the iOS platform, but this target supports 11.0

It's that Identifiable requires iOS 13+ / macOS 10.15+. Even if I add @available(iOS 13.0, OSX 10.15, tvOS 13.0, watchOS 6.0, *) everywhere that Identifiable is used in the repo, it doesn't help with 2 particular situations:

image

and

image

The Codable and ExpressableByStringLiteral protocols require that the initialisers support iOS 11, which conflicts with the @available restriction. For all the other places where Identifiable is used, just adding the @available annotation works fine.

stephencelis commented 3 years ago

@mluisbrown Is it possible to define our own Identifiable protocol for #if swift(<5.3)?

mluisbrown commented 3 years ago

@stephencelis it is possible to define our own Identifiable protocol, but it would need to be for iOS < 13 (etc). The problem isn't really the Swift language version so much as the OS version.

You can be using Swift 5.3 and still want to have a minimum deployment target of iOS 12, but you won't get Identifiable and a few other things. It's not possible to use compile time guards for OS version because the OS version is only known at runtime.

I think you could continue to ship the old IdentifiedArray directly inside the library. I know this isn't ideal, but it'd at least unblock you for now.

I'm going with this workaround for now, and so far it seems to be ok.

stephencelis commented 3 years ago

@mluisbrown Can you try using this branch? https://github.com/pointfreeco/swift-identified-collections/compare/relax-platform-requirements

Locally things seem to pass for me when targeting iOS 11.

mluisbrown commented 3 years ago

Locally things seem to pass for me when targeting iOS 11.

@stephencelis I get the same errors as before with the conformances for Decodable and ExpressableByArrayLiteral. I targeted a simulator with iOS 11.4:

image

I am using Xcode 12.5.1 for which AFAIK #if compiler(>=5.3) will always be true.

This change would probably work for using an older version of Xcode that only used Swift 5.2 but that wouldn't be all that useful.

stephencelis commented 3 years ago

Strangely enough it works on the Xcode 13 beta on an iOS 11 sim, which should definitely pass that compiler directive...I'll have to try 12 soon.

mluisbrown commented 3 years ago

Strangely enough it works on the Xcode 13 beta on an iOS 11 sim

Wow 😮 I guess it's possible that Swift 5.5 fixed the issue that checks the minimum OS availability for a protocol like Decodable if it's already been excluded by a previous @available directive. Because that does seem like kind of a bug to me.

lorentey commented 3 years ago

Swift Collections only builds with a recent compiler toolchain, and we expect to keep bumping the required compiler/SwiftPM version (i.e., swift-tools-version) to the ~latest Swift release. (Not out of spite, but because sticking with older compilers would mean sticking with old compiler bugs, some of which would be tricky/impossible to work around.)

However, Swift Collections does not specify a minimum deployment target -- it runs on every OS release that can run Swift. I think this package should do the same!

The compiler version check in the screenshot above seems wrong -- support for availability for protocol conformances did not land until after 5.3 was released, and I think it only matured enough to compile this code in Swift 5.5.

stephencelis commented 3 years ago

@mluisbrown With a compiler check of Swift 5.5 does everything work for you? If so, we'd be happy to take a PR to address this! We realize it's not a fix that can be used in production quite yet, but when Xcode 13 is released you should be able to fully switch off the old, vendored version :)

mluisbrown commented 3 years ago

Hi @stephencelis, it's strange. With Xcode beta 5 both the compiler check for Swift 5.3 and Swift 5.5 allow the code to build for an iOS 11 sim. However, when I run tests on an this sim (iPhone X, iOS 11.4) they crash:

TestCrash

The crash in swift_getWitnessTable seems to indicate that it is the protocol conformances to ExpressibleByArrayLiteral which is causing the issue.

Interestingly in Xcode 12.5.1, if I set the compile check to 5.5 (instead of 5.3 which I tried above without success), the library code compiles OK, but the test code does not:

image

That makes sense, since Xcode 12.5.1 is Swift 5.4, so the protocol conformance to ExpressibleByArrayLiteral is simply not being compiled.

With Xcode 13 beta 5 if I change the first test to not use an array literal then this test passes:

  func testIds() {
    let array: IdentifiedArray<Int, Int> = .init(uniqueElements: [1, 2, 3], id: \.id)
    XCTAssertEqual(array.ids, [1, 2, 3])
  }

So in summary: with Xcode 13 and a Swift 5.5 compiler check, the library compiles and can be used in with iOS 11 (and presumably the respective versions of the other OSes), but you it seems like a Swift and/or Xcode bug that it allows code that depends on the Swift 5.5 only protocol conformances compile without error and cause a runtime crash instead.

stephencelis commented 2 years ago

Going to move this to a discussion. We'd gladly take a PR for this but we've tried to move our repos to using issues exclusively for bug tracking, and this is more of a feature request.