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

Potential len method ambiguity in src/range.rs #1159

Closed
yhx-12243 opened this issue Apr 19, 2024 · 5 comments · Fixed by #1161
Closed

Potential len method ambiguity in src/range.rs #1159

yhx-12243 opened this issue Apr 19, 2024 · 5 comments · Fixed by #1161

Comments

@yhx-12243
Copy link

As we can see, in the following code we assume Range<u64> and Range<i64> does not implement ExactSizeIterator.

rayon/src/range.rs

Lines 292 to 296 in 8ccfda3

// other Range<T> with just Iterator
unindexed_range_impl! {u64, u64}
unindexed_range_impl! {i64, u64}
unindexed_range_impl! {u128, u128}
unindexed_range_impl! {i128, u128}

But in 64-bit platforms, they should implement and there are proposes in rust-lang/rust#124056 and rust-lang/libs-team#369.

However, it encounters barrier because this hardcoded logic. So we hope it can be improved.

@adamreichold
Copy link
Collaborator

we assume Range and Range does not implement ExactSizeIterator.

I would say it is the other way around: We do not assume that they do implement ExactSizeIterator.

I am not sure what you mean by "break" if such an implementation was added to the standard library? From my understanding, Rayon would just not automatically take advantage of it, but everything should continue to work as it does now, shouldn't it?

In any case, we should not move here before the std does, i.e. there is not just a decision, but the feature is published and available in Rayon's MSRV.

@yhx-12243
Copy link
Author

One can check this failed action once we add the implementation it will gives a compile error.

@adamreichold
Copy link
Collaborator

Understood, so we could rename UnindexedRangeLen::len to something else to avoid the ambiguity. However, this also suggests that the change would need to happen at an edition boundary, i.e. be opt-in and we should be fine as long as build against the current edition.

@cuviper
Copy link
Member

cuviper commented Apr 19, 2024

Ambiguous method resolution is a risk for any standard library addition. I don't think it's fair to call that "unrobust" here, because the only perfect way to defend against it is to use fully-qualified syntax for all trait method calls. This risk is acknowledged as a "minor change" in the API evolution RFC, and the Rust Project uses its crater tool to judge the practical impact -- though it's more obvious when it breaks a library used by the toolchain itself, as you found.

https://rust-lang.github.io/rfcs/1105-api-evolution.html#minor-change-implementing-any-non-fundamental-trait

However, this also suggests that the change would need to happen at an edition boundary,

AIUI, the compiler has no ability to consider editions during trait/method resolution. I don't know if that's a fundamental limitation, or just something that hasn't been implemented or would be very difficult. So in the current situation, the library team will weigh the benefit of such additions against the crater impact and decide all or nothing.

I'm not totally opposed to renaming internal implementation details like UnindexedRangeLen::len, but I'm pretty skeptical that the upstream API team will want to add these new ExactSizedIterator impls at all.

@cuviper cuviper changed the title Unrobust code in src/range.rs Potential len method ambiguity in src/range.rs Apr 19, 2024
@yhx-12243
Copy link
Author

I see. So this proposal is under discussion in rust-lang/libs-team#369 because I'm afraid of there are more example that "implicitly" "assume some trait are not implemented".

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 a pull request may close this issue.

3 participants