Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Unsafe Storage #26

Merged
merged 12 commits into from Mar 26, 2020
Merged

RFC: Unsafe Storage #26

merged 12 commits into from Mar 26, 2020

Conversation

stephencelis
Copy link
Member

Up to this point, swift-nonempty has compiler-proven, safe storage for its underlying collection. This has the ultimate benefit of not having to unsafely unwrap values anywhere, but it comes with a number of downsides:

  • We currently maintain a lot of logic around pretending a head element and a tail collection are a single, baked collection. This includes index math code, and it turns out it has some bugs!
  • We can't always vend out the raw collection type, since we hold onto the head element outside its collection and not every collection provides a conformance that allows re-insertion of that head element.
  • We pay a performance cost. While _modify semantics improves this, we ultimately can't be as performant as code that merely holds onto a single collection under the hood.

This PR converts the project to a simpler, less-safe storage of the underlying collection. A few notes:

  • This introduces a failable initializer init?(rawValue:).
  • This removes all logic we maintained around connecting the head and tail properties together. All conformance code merely forwards things on to the underlying collection, unsafely unwrapping elements where applicable. This means all this code is potentially a fatal error waiting to happen, but at least the behavior is more apt to be correct when it doesn't crash.
  • It is no longer possible to provide the "safe" init(head:tail:) initializer for every collection because it's not possible to prepend just any collection with an element. This seems like an acceptable loss.
  • NonEmpty is now raw-representable (using its failable initializer). This is nice for Point-Free's routing.

@@ -4,7 +4,7 @@ public protocol _DictionaryProtocol: Collection where Element == (key: Key, valu
associatedtype Key: Hashable
associatedtype Value
var keys: Dictionary<Key, Value>.Keys { get }
subscript(key: Key) -> Value? { get }
subscript(key: Key) -> Value? { get set }
Copy link
Member Author

Choose a reason for hiding this comment

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

NonEmpty itself doesn't conform to this protocol, but we need to know that this is settable to mutate the underlying dictionary in the convenience initializer below.

extension NonEmpty: ExpressibleByStringInterpolation where C: ExpressibleByStringInterpolation, C.StringLiteralType == DefaultStringInterpolation.StringLiteralType {}

#if swift(>=5.2)
extension NonEmpty: StringProtocol where C: StringProtocol {
Copy link
Member Author

Choose a reason for hiding this comment

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

Also added conformance to StringProtocol, which adds a bunch of functionality and was useful in Point-Free for using literals in tests. We could potentially add literal code for arrays/dictionaries, but we'd have to do that unsafe bit-cast dance that Joe told us not to do with Tagged (Slava thinks it's fine with ABI stability though).

@@ -1,21 +1,152 @@
@dynamicMemberLookup
Copy link
Member Author

Choose a reason for hiding this comment

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

Dynamic member lookup seems useful for this kind of thing.

Choose a reason for hiding this comment

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

This is a smart use of it, saves you having to forward all the computed properties manually.

public struct NonEmpty<C: Collection>: Collection {
public typealias Element = C.Element
public typealias Index = C.Index
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer need a custom index type.

@stephencelis
Copy link
Member Author

This PR also provides an alternative fix for #4 (and #17) and could get us closer to #19...

@Noobish1
Copy link

I'm glad you've gone down this route. I have my own version of a NonEmptyArray that I use and it's based off of https://github.com/khanlou/NonEmptyArray which uses the same kind of "unsafe" but realistically safe implementation.

I like your implementation more because it's more generic and you can use it for things other then arrays.

@kastiglione
Copy link

💯 for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants