search

pointfreeco / swift-identified-collections

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

Unavailable position-based subscript setter crashes on runtime #14

Closed Thomvis closed 3 years ago

Thomvis commented 3 years ago

After upgrading from TCA's IdentifiedArray to the version in this repo (0.1.1), my app is crashing in places where it used the position-based subscript setter.

There's a @available annotation in the code, but that doesn't show up (at least not in Xcode 12.5). It's the fatalError in the setter that hits.

The weird thing is that the error did show up in another place in my code.

I think it's unfortunate that has become a runtime issue, but I'm not sure how to fix that. I'm also not sure why the setter can't be implemented. I can live without it (I only use it in tests), but it was convenient to have.

Thomvis commented 3 years ago

Looks like this comes from a restriction of the underlying OrderedDictionary offset-based subscript. That one doesn't have a setter either. However, the docs seems to imply it should be used for in-place mutation...

Thomvis commented 3 years ago

I wrote a very naive implementation that works in my app: https://github.com/Thomvis/swift-identified-collections/commit/f6561f9ef2feb179b64c8aed7a6ee0b7570d8949.

Do you think something like this could be added to the library?

stephencelis commented 3 years ago

@Thomvis Dang, yeah I hit that exact issue. It appears to be a compiler bug 😞

We eliminated the index-based setter because we think it opens up the opportunity to create invalid collections, e.g. if you have an element at 0 identifying as "deadbeef" and then you try to insert another element at 1 that also identifies as "deadbeef". Swift Collections' OrderedSet type has the same design.

So, instead of adding _modify to the subscript, can you use the id-based subscript instead? For example:

mage.resources[id: mage.resources[0].id]?.used = 1

I think what we'll probably want to do in the library is remove the deprecation entirely, so that the compiler can catch the issue. It's a bit of a bummer that we can't offer a better migration path, but this compiler bug is out of our control!

stephencelis commented 3 years ago

@Thomvis we plan on removing this setter in #15, but wanted to make sure you could still mutate things via the other subscript.

Thomvis commented 3 years ago

@stephencelis yes, that works. I’ll probably end up creating a “idPosition” subscript helper in my test target to make it a bit easier. In all/most other cases, using the mutable position-based subscript was a mediocre choice anyway. Thanks!

stephencelis commented 3 years ago

Fixed in #15.