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

Remove redundant bounds #1207

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Conversation

steffahn
Copy link
Contributor

@steffahn steffahn commented Dec 26, 2021

There were 3 trait implementations that placed redundant AsRef/AsMut bounds on <R as BlockRngCore>::Results which were already implied by the bounds in the BlockRngCore trait definition.

Noticed those because they show up in the documentation, e.g. here.

Also fix 2 warnings (one clippy warning, one rustc warning).

Copy link
Member

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Looks fine if it passes CI.

@dhardy
Copy link
Member

dhardy commented Jan 10, 2022

Which it doesn't (this is using Rust 1.36.0):

error[E0277]: the trait bound `<R as block::BlockRngCore>::Results: core::convert::AsRef<[u32]>` is not satisfied
   --> rand_core/src/block.rs:181:1
    |
181 | / impl<R: BlockRngCore<Item = u32>> RngCore for BlockRng<R> {
182 | |     #[inline]
183 | |     fn next_u32(&mut self) -> u32 {
184 | |         if self.index >= self.results.as_ref().len() {
...   |
237 | |     }
238 | | }
    | |_^ the trait `core::convert::AsRef<[u32]>` is not implemented for `<R as block::BlockRngCore>::Results`
    |
    = help: consider adding a `where <R as block::BlockRngCore>::Results: core::convert::AsRef<[u32]>` bound
note: required by `block::BlockRngCore`
   --> rand_core/src/block.rs:68:1
    |
68  | pub trait BlockRngCore {
    | ^^^^^^^^^^^^^^^^^^^^^^

@steffahn
Copy link
Contributor Author

Oh, so Rust used to not support this? Interesting...

@steffahn
Copy link
Contributor Author

I'll investigate later what changed when, etc. Keeping these bounds really is only an issue of documentation after all, so it's not too bad. If we need to keep them, I might change this PR so that comments are added in the source code on these bounds, pointing out they're redundant but used to be needed and are kept for MSRV support.

@dhardy
Copy link
Member

dhardy commented Jan 10, 2022

1.36.0 is 2.5 years old by now; when we asked there was generally little interest in support beyond six months old, so an MSRV bump is not out of the question (even if this is only a minor thing).

@steffahn
Copy link
Contributor Author

The relevant fix in rustc was apparently rust-lang/rust#73905. The code without the extra bound (i.e. the code in this PR) works from Rust 1.49 (released December 2020).

@vks vks added the B-compiler Breakage: needs compiler upgrade label Jan 13, 2022
@vks
Copy link
Collaborator

vks commented Feb 22, 2022

This needs an update to the changelog and a bump to Rust 1.51 (see #1165).

@dhardy
Copy link
Member

dhardy commented Nov 14, 2022

@steffahn can you rebase? The MSRV was increased in #1246 already.

@dhardy dhardy removed the B-compiler Breakage: needs compiler upgrade label Nov 14, 2022
@steffahn
Copy link
Contributor Author

Rebased

@dhardy dhardy merged commit 21131af into rust-random:master Nov 14, 2022
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.

None yet

3 participants