RxSwiftCommunity / RxDataSources

UITableView and UICollectionView Data Sources for RxSwift (sections, animated updates, editing ...)
MIT License
3.1k stars 496 forks source link

`model(at:) throws -> Any` method doesn't throw. #388

Closed freak4pc closed 3 years ago

freak4pc commented 3 years ago

This is a copy of https://github.com/ReactiveX/RxSwift/issues/2256 by @cameroncooke

Short description of the issue:

The model(at:) convenience method of CollectionViewSectionedDataSource is marked as throwing yet the implementation doesn't throw and can't throw as the underlying logic doesn't handle errors by throwing. There are no try statements within the body of the method.

https://github.com/ReactiveX/RxSwift/blob/ef8b0abe3334c5853001577125c82c86c55b41ce/RxExample/RxDataSources/RxDataSources/CollectionViewSectionedDataSource.swift#L78

The presence of the throws in the API contract caused us to introduce a production crash in our app because we believed that this method would throw when subscript was out of bounds which we handled at the callsite using try?. Instead as this method doesn't actually throw the underlying subscript implementation raises a fatalError instead.

Expected outcome:

Method to not be marked as throwing if it doesn't throw.

What actually happens:

Nothing directly, but in the case that a subscript out of bounds error is raised it's not handled by RxSwift as the throws method might suggest which can introduce bugs in the application using RxSwift.

Self contained code example that reproduces the issue:

func doSomething() {
    let outOfBoundsIndexPath = IndexPath(item: 1000, section: 0)
    guard let model = try? datasource.model(at: outOfBoundsIndexPath) else { return nil } // crashes instead of being handled
}

RxSwift/RxDataSources versions

5.1.1/4.0.1

Platform/Environment

How easy is to reproduce? (chances of successful reproduce after running the self contained code)

Xcode version:

Version 12.1 (12A7403)

Installation method:

I have multiple versions of Xcode installed: (so we can know if this is a potential cause of your issue)

Level of RxSwift knowledge: (this is so we can understand your level of knowledge and formulate the response in an appropriate manner)

cameroncooke commented 3 years ago

Thanks for moving this.

freak4pc commented 3 years ago

Fixed in 5.0.0 @cameroncooke