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

RangeInclusive<usize> shouldn't impl ExactSizeIterator #36386

Closed
durka opened this issue Sep 10, 2016 · 10 comments
Closed

RangeInclusive<usize> shouldn't impl ExactSizeIterator #36386

durka opened this issue Sep 10, 2016 · 10 comments

Comments

@durka
Copy link
Contributor

durka commented Sep 10, 2016

(0...usize::max_value()).len() panics:

$ cargo +nightly script -u inclusive_range_syntax -e '(0...usize::max_value()).len()'
thread 'main' panicked at 'assertion failed: `(left == right)` (left: `None`, right: `Some(18446744073709551615)`)', ../src/libcore/iter/traits.rs:521
note: Run with `RUST_BACKTRACE=1` for a backtrace.

cc @gnzlbg https://www.reddit.com/r/rust/comments/51mlth/how_should_we_represent_ranges_in_the_standard/d7gdx1v

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 11, 2016

As I mention in a followup comment in reddit, I think that RangeInclusive should implement ExactSizeIterator.

Penalizing RangeInclusive by not implementing ExactSizeIterator just because doing so panics for 0...usize::max() / 0...u64::max(), while ExactSizeIterator works fine for all other inputs might cause more trouble than it is worth (e.g. if users start going through workarounds for performance to recover the ExactSizeIterator properties).

I think that the right thing to do is to still panic, but do so earlier, on construction. Arguably, if somebody wants to do 0...usize::max() what they need to be doing is using a BigInt type (e.g. from the num crate). So the panic message should probably just tell them that.

@bluss
Copy link
Member

bluss commented Sep 11, 2016

Panic on construction is icky. RangeInclusive is supposed to be useful as an argument type (it's more than an iterator).

I think this PR is fine (it's unstable and we can add it back).

A change in the trait's contract could enable range inclusive to keep the implementation: #34433 (comment) ? I don't think it's too crazy, warrants discussion.

@gnzlbg
Copy link
Contributor

gnzlbg commented Sep 11, 2016

As long as we somehow manage to keep RangeInclusive implementing ExactSizeIterator any solution is fine with me.

@durka
Copy link
Contributor Author

durka commented Sep 11, 2016

I somewhat agree that it's lame for the whole type to be !ExactSizeIterator when the problem is only with one value of the type. However, there is precedent -- see Range<u64>, Chain, etc.

I'm not sure about @bluss's "saturating ExactSizeIterator" idea, but I'm willing to be convinced. It seems like it makes ExactSizeIterator only work for iterators of length up to usize::MAX - 1, if you get usize::MAX you can't trust the value (so it's a secret Option). Of course, if you're using the exact size to allocate memory, then values near usize::MAX aren't useful anyway, but there could be other uses.

@ollie27
Copy link
Member

ollie27 commented Sep 11, 2016

If we use the same logic as we do for the From for usize conversions, namely that we can only assume usize is 8 bits then we can't implement ExactSizeIterator for any of RangeInclusive. Even RangeInclusive<u8> requires that usize is 16 bits. I think at least for the time being we should just remove all the implementations of ExactSizeIterator for RangeInclusive.

Similarly as pointed out #34433 (comment) we shouldn't have implementations of ExactSizeIterator for Range<u32> and using the same logic as for From we can't even have it for Range<u16>.

@durka
Copy link
Contributor Author

durka commented Sep 11, 2016

Well it is already implemented for Range<u16> and Range<u32> and we can't really change that. Personally I would say let's assume that usize is at least 32 bits wide and when we go porting to 16-bit systems we can start making impls conditional on that. As I said on Reddit, some code would stop compiling on those systems but that is already the case with enum discriminants.

It seems that this decision has not been made as different parts of the standard library are following different guidelines (the From camp vs the ExactSizeIterator camp). Perhaps it needs a policy RFC?

@ollie27
Copy link
Member

ollie27 commented Sep 11, 2016

I personally think it would be fine to have impls gated on the size of usize.

Either way we'll need to remove the ExactSizeIterator impls for for RangeInclusive<i32> and RangeInclusive<u32> as they require usize to be larger than 32 bits.

@durka
Copy link
Contributor Author

durka commented Sep 11, 2016

I updated the PR to remove those. Perhaps I could put them back gated by #[cfg(target_pointer_width = "64")].

@bluss
Copy link
Member

bluss commented Sep 11, 2016

I realized ESI was introduced to support the .rposition() (index from the back) method, which will then return the wrong result for too long chains. Not that such a big index is that useful, but it would still be wrong with the saturating ESI contract.

@durka
Copy link
Contributor Author

durka commented Sep 12, 2016

See rust-lang/rfcs#1748.

bors added a commit that referenced this issue Sep 29, 2016
remove ExactSizeIterator from RangeInclusive<{u,i}{32,size}>

Fixes #36386.

This is a [breaking-change] for nightly users of `#![feature(inclusive_range_syntax)]` and/or `#![feature(inclusive_range)]`.
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

No branches or pull requests

5 participants