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

Add a non-mutating lazy replaceSubrange #203

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

karwa
Copy link
Contributor

@karwa karwa commented Sep 9, 2023

Sometimes, you want to insert/remove elements from a collection without creating a copy and mutating it.

It's kind of a generalisation of chain. And maybe joined. I still need to do the guide document and implement BidirectionalCollection, but I'm creating the PR now for discussion purposes. In particular:

  • Is this a good candidate for inclusion in the project?

  • What should it be named?
    x.lazy.replacingSubrange(i..<j, with: y) isn't amazing, but draws strong parallels with replaceSubrange.
    I could also see "overlaying", as in x.overlaying(i..<j, with: y). Any other ideas?

  • Does anybody know of similar features in other languages?
    For the guide doc. I tried searching but couldn't find anything.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • [] I've updated the documentation if necessary

@karwa
Copy link
Contributor Author

karwa commented Sep 9, 2023

So, when checking this out using Godbolt, I find that the compiler produces shockingly good code. To the extent that, while I initially thought it might be a bit niche, I now wonder if it could be an interesting alternative to mutating operations for a lot of other applications - particularly SwiftUI applications.

Imagine I have some data, perhaps retrieved from the network or stored in some global datasource object. Let's say they're some kind of product, perhaps shoes, and I'm displaying them in a list/grid. I also have a boolean, telling me whether I should display a special call-to-action item in the list. This is a fairly standard thing to want.

The actual data that I want to use to drive my list/grid is composed from those two elements: the list of products and the boolean.

class ProductsListViewModel {
  var rawProductsList: [Product] = ...
  var showCallToAction: Bool = ...

  var viewItems: ??? // built by combining rawProductsList with showCallToAction
}

SwiftUI tends to emphasise computed properties for these kinds of things -- while I could make viewItems a stored Array-type property, I'd then need to make sure it's always up-to-date. Also, if the products list is a reference to non-owned data (such as a Binding), it may update even when this particular view isn't on-screen; in that case, we might be recomputing the viewItems unnecessarily.

Computed properties, on the other hand, are always up-to-date, taking account of the latest list of products and the latest state of the boolean. With @Observable, these kinds of data dependencies will even be tracked automatically.

However, within the implementation of the computed property, we must make a copy of the products list in order to inject elements in to it. The lack of lazy insertions also means we have to perform eager transformations such as map.

class ProductsListViewModel {
  var rawProductsList: [Product] = ...
  var showCallToAction: Bool = ...

  var viewItems: some Collection<ListItem> {
    var result = rawProductsList.map { .product($0) }
    if showCallToAction {
      result.insert(.callToAction, at: min(result.count, 4))
    }
    return result
  }
}

This is unfortunate, since viewItems will likely be called in a SwiftUI body property, meaning the map copy, and potential second copy and element shuffling caused by the insert, will happen in this performance-critical function.

With lazy insertions, we can avoid that:

class ProductsListViewModel {
  var rawProductsList: [Product] = ...
  var showCallToAction: Bool = ...

  var viewItems: some Collection<ListItem> {
    let result = rawProductsList.lazy.map { .product($0) }
    let ctaPosition = result.index(result.startIndex, offsetBy: 4, limitedBy: result.endIndex) ?? result.endIndex
    if showCallToAction {
      return result.replacingSubrange(ctaPosition..<ctaPosition, with: CollectionOfOne(.callToAction))
    } else {
      return result.replacingSubrange(result.startIndex..<result.startIndex, with: EmptyCollection()) // no-op.
    }
  }
}

viewItems has the same contents as before, but now we're able to use a lazy map transformation on the products list, and our insertion of the call-to-action prompt also won't copy or shuffle elements around. I'd expect it to be cheaper to call this from within a SwiftUI body function.

Iterating such a collection is more expensive than iterating an array, but because the compiler does such a good job with the generated code, there's a very good chance that it's still a net positive.

Unfortunately, it is also more awkward to use. Which is why another idea that I have WRT naming is to create a .overlay namespace struct (similar to .lazy), on which we can implement similar convenience methods to RangeReplaceableCollection. So people could write things like:

result.overlay.insert(.callToAction, at: ctaPosition)
// instead of
result.replacingSubrange(ctaPosition..<ctaPosition, with: CollectionOfOne(.callToAction))

(Array obviously has integer indexing, which is supremely convenient. We can't quite match that, but we could perhaps add some convenience APIs such as insert(_: Element, atOffset: Int))

@karwa
Copy link
Contributor Author

karwa commented Sep 17, 2023

One issue is that, if you're only conditionally applying a replacement while returning an opaque type, you need to perform a no-op replacement which ensures both branches have the same type. e.g:

if showCallToAction {
  return result.overlay.inserting(.callToAction, at: min(4, result.count))
} else {
  // Must return an OverlayCollection<Base, CollectionOfOne<ListItem>> which only contains the base elements.
  // That's not trivial.
}

What we can do instead is to (internally) make the overlay elements optional (i.e. the stored property overlay would become an Optional<Overlay>). That means we could always construct a no-op overlay wrapper with any overlay type.

To expose that feature, we could add a conditional overlay function:

extension Collection {

  @inlinable
  public func overlay<Overlay>(
    if condition: Bool,
    _ makeOverlay: (OverlayCollectionNamespace<Self>) -> OverlayCollection<Self, Overlay>
  ) -> OverlayCollection<Self, Overlay> {
    if condition {
      return makeOverlay(self.overlay)
    } else {
      return OverlayCollection(base: self, overlay: nil, replacedRange: startIndex..<startIndex)
    }
  }
}

Allowing usage like:

var viewItems: some Collection<ListItem> {
  let result = rawProductsList.lazy.map { .product($0) }
  return result.overlay(if: showCallToAction) {
    $0.inserting(.callToAction, at: min(4, $0.count))
  }
}

@lgreydev
Copy link

lgreydev commented Oct 5, 2023

replaceSubrange - Nice!

@CTMacUser
Copy link
Contributor

For the single-element removal method, isn't "removingSubrange(position..<position)" wrong? The range above specifies zero elements. You'll need "position ..< NEXT(position)" instead, where NEXT is replaced with the appropriate code.

@karwa
Copy link
Contributor Author

karwa commented Oct 7, 2023

@CTMacUser Yes, good catch!

@CTMacUser
Copy link
Contributor

OverlayCollection.makeIndex(_:) can't work if the source collections use the same Index type, because then the two overloads would have identical signatures. Possible fix: explicitly use the words "base" or "overlay" within the methods' names.

@CTMacUser
Copy link
Contributor

You could add an optimized .isEmpty

  @inlinable
  public var isEmpty: Bool {
    base[..<replacedRange.lowerBound].isEmpty && base[replacedRange.upperBound...].isEmpty && (overlay?.isEmpty ?? true)
  }

@karwa
Copy link
Contributor Author

karwa commented Oct 26, 2023

OverlayCollection.makeIndex(_:) can't work if the source collections use the same Index type, because then the two overloads would have identical signatures. Possible fix: explicitly use the words "base" or "overlay" within the methods' names.

Yes it can, because whether we're constructing a base or overlay index is determined based on unbound generics. Specialisation happens later, and does not affect overload resolution. This is one of the ways in which Swift generics are not like C++ templates.

For example, the very first test appends an Array<Int> to a Range<Int>. Both types use Int as their index.

- Shrunk OverlayCollection.Index
- Implemented Collection.isEmpty
- Added tests for single-element append/insert/removal methods
- Added separate removeSubrange tests
@karwa
Copy link
Contributor Author

karwa commented Oct 26, 2023

@natecook1000 Thoughts?

@CTMacUser
Copy link
Contributor

In my run through of 598946f, that last default of true in isEmpty was never triggered. That's because the trigger would be excluding an overlay on an already-empty collection. My quick test case, appended to testConditionalReplacement:

    func getNoNumbers(shouldInsert: Bool) -> OverlayCollection<Range<Int>, CollectionOfOne<Int>> {
      (0..<0).overlay(if: shouldInsert) { $0.inserting(42, at: 0) }
    }

    do {
      let result = getNoNumbers(shouldInsert: true)
      XCTAssertEqualCollections(result, [42])
      IndexValidator().validate(result, expectedCount: 1)
    }

    do {
      let result = getNoNumbers(shouldInsert: false)
      XCTAssertEqualCollections(result, [])
      IndexValidator().validate(result, expectedCount: 0)
    }

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.

3 participants