Add trait implementation guidance #312
Conversation
rand-core/src/lib.rs
Outdated
| @@ -81,6 +81,14 @@ pub mod le; | |||
| /// in this trait directly, then use the helper functions from the [`impls`] | |||
| /// module to implement the other methods. | |||
| /// | |||
| /// It is recommended that implementations also implement: | |||
| /// | |||
| /// - `Debug` but such that no internal details are output | |||
There was a problem hiding this comment.
For CSPRNGs this seems like something to recommend to me, but I am starting to feel like it matters less for small rngs. There it may even be desirable to print the state.
There was a problem hiding this comment.
I doubt it matters much; the most that most people would want to do with the state is check whether it matches another state printed in the log, and it's not a very good way of checking states are equal in the first place (serialisation would be a better option).
rand-core/src/lib.rs
Outdated
| /// It is recommended that implementations also implement: | ||
| /// | ||
| /// - `Debug` but such that no internal details are output | ||
| /// - `Serialize` and `Deserialize` (from Serde), possibly behind a feature gate |
There was a problem hiding this comment.
Ah, something we talked a lot about 😄. Is behind a feature gate not mostly a practical matter, to keep the number of dependencies down?
There was a problem hiding this comment.
It seems good practice for a library to keep dependencies optional where possible. Maybe I should expand the comment.
There was a problem hiding this comment.
Or not mention it at all, seems like mixing concerns... Your choice.
rand-core/src/lib.rs
Outdated
| /// | ||
| /// - `Debug` but such that no internal details are output | ||
| /// - `Serialize` and `Deserialize` (from Serde), possibly behind a feature gate | ||
| /// - `Clone` if, and only if, the clone will have identical output to the original |
There was a problem hiding this comment.
We currently violate this with ThreadRng...
Do we want to guarantee the same output? Or do we want to guarantee the same generator?
I think we should just remove Clone from ThreadRng, but asking anyway. Would be another (small, I think) breaking change.
There was a problem hiding this comment.
ThreadRng is Clone because it's a Rc<..>. So maybe we should expand the doc to something like:
- if the clone would have identical output (i.e. be a true clone) or if the clone would be another handle to the same generator
There was a problem hiding this comment.
With that definition OsRng could implement clone also. I prefer the definition you wrote of identical output.
There was a problem hiding this comment.
if the clone would have identical output (i.e. be a true clone) or if the clone would be another handle to the same generator
Does this imply every RNG should document what its clone does? Or should we hide the Rc and make ThreadRng clone the generator instead of the handle?
There was a problem hiding this comment.
Technically ThreadRng meets the requirement: the clone has identical output to the original, except that the streams are not independent 😄
IMO ThreadRng supporting Clone causes no problems, and might be useful if someone wants to store it in a struct/tuple and then clone that.
There was a problem hiding this comment.
@vks didn't see your comment until after posting. No, we definitely shouldn't allow cloning the internal ThreadRng generator, it's supposed to be a secure way to generate random data, not one which risks returning cloned streams. I don't see much need to document the cloning in general, though I will for ThreadRng because it is an exception.
|
Could you add that RNGs should not implement |
dhardy
added
C-docs
rand-core/src/lib.rs
Outdated
| /// - *never* implement `Copy` (accidental copies may cause repeated values) | ||
| /// - also *do not* implement `Default`, but instead implement `SeedableRng` | ||
| /// thus allowing use of `rand::NewRng` (which is automatically implemented) |
There was a problem hiding this comment.
Why not use NewRng for Default?
There was a problem hiding this comment.
PRNGs will be implemented in various crates outside of the rand crate, which provides NewRng. Default comes from the standard library. Therefore the locality of implementation rules make this impossible. PRNGs could implement Default themselves, but they'd have to depend on rand to do so, which I don't think they should. Other than that I guess it's a good idea, though possibly a little confusing.
|
Looks good to me! Ready to merge? (The CI has some trouble with installing |
|
Failed on retry too. Never mind, I'll just merge. |
Add trait implementation guidance
Close dhardy#13