cc https://github.com/rust-lang/api-guidelines/issues/75, @dtolnay

From https://github.com/env-logger-rs/env_logger/issues/185:

The scenario where this goes wrong (assuming you're the author of env_logger and not using html_root_url) is:

1. Another downstream crate uses env_logger as a dependency
2. downstream re-exports or in some other way links to env_logger
3. A user or developer of downstream builds documentation locally with cargo doc --no-deps (without --extern-html-root-url, because in practice no one but docs.rs does that).

Then the links to env_logger will be broken. But there's a simple fix and the fix is to remove --no-deps. Is this really so common that it's worth recommending that library authors use html_root_url?

Note that I'm hoping to fix this properly in cargo instead: https://github.com/rust-lang/cargo/issues/8296

My understanding is that nothing should ever use description(). Most errors will have a significantly more helpful Display representation than what the signature of description() allows.

Nevertheless, description() is there and has to return something.

Closes #6

This is an attempt at a guideline for discouraging trait bounds on data structures. It looks like the discussion has run its course, but it might be a bit premature to capture this in a guideline. If so I guess we can move the discussion back to #6.

I thought I'd open this as a PR and get some general feedback on what we expect guidelines to look like, so I can align them better in the future. Some things I'm not sure about:

• Should all guidelines have an advantages and disadvantages section?
• Should all examples be self-contained or can they depend on previous ones?
• Should we keep discussion points succinct or expand on them a bit?
• How strict should our wording be about conforming to the guideline?

reqwest::Error::get_ref returns Option<&(Error + 'static)> which is an error that is not Send and Sync. This is easy to miss and should be noted in C-SEND-SYNC-ERR.

I don't think there's a strong consensus here yet.

However, there seems to be a weak consensus, and also de-facto practice for many popular crates (cfg-if, lazy-static). Additionally, there's a lot of continuous discussion and re-litigation across various forums and issue trackers.

So it seems like tentatively documenting current practice might make sense?

I think it is better to add a where clause to the struct/enum/union definition which is required to be Send/Sync/any other trait. First, of all, it becomes part of the item declaration and second, it doesn't require compiling the crate with cfg(test) for verifying the implementation of the auto traits.

Compare this (taken from this paragraph):

#[test]
fn test_send() {
    fn assert_send<T: Send>() {}
    assert_send::<MyStrangeType>();
}

#[test]
fn test_sync() {
    fn assert_sync<T: Sync>() {}
    assert_sync::<MyStrangeType>();
}

To this:

struct MyStrangeType
where
    Self: Send + Sync
{
    // ...
}

This style doesn't conflict with clippy and rustc lints. So how about amending that paragraph with the new guideline?

I noticed that the guidelines don't recommend a way to name getters.

fn get_member(&self) -> &Member

vs

fn member(&self) -> &Member

Might be nice to recommend something here.

I'd be strongly in favor of the second option without get_.

The main one I know of is docs.rs only supports documenting the default feature set: https://github.com/onur/docs.rs/issues/29.

Dependency on system libraries may also be a problem.

Example from flate2:

pub struct EncoderReader<R: Read> {
    inner: EncoderReaderBuf<BufReader<R>>,
}

Bounds like this are annoying because they transitively infect any structs containing this type. Usually trait bounds should go just on impl blocks, not on data structures.

There are two exceptions where trait bounds on data structures are required.

1. The data structure refers to an associated type of the trait. Cow is an example of this.
2. The data structure has a Drop impl that requires trait bounds. Rust currently requires all bounds on the Drop impl to also be present on the data structure.

Relevant flate2 issue: https://github.com/alexcrichton/flate2-rs/issues/88

Generic reader/writer functions take R: Read and W: Write by value (C-RW-VALUE).

https://rust-lang-nursery.github.io/api-guidelines/interoperability.html#c-rw-value https://github.com/rust-lang-nursery/api-guidelines/blob/05a1d5e5568606442c770919d167513dbbf78024/src/interoperability.md#generic-readerwriter-functions-take-r-read-and-w-write-by-value-c-rw-value

Hyper has been using a bad html_root_url since 0.10.7 (https://github.com/hyperium/hyper/issues/1193). Let's explain or link to an explanation of how to pick the right html_root_url so users are not forced to guess.

There's no need to have a doc-hidden method to simulate private trait items. They can be placed in the supertrait that performs the sealing. The advantage here is that the compiler enforces that it's not used downstream. I personally use this pattern in a few places.

C-COMMON-TRAITS says that "crates that define new types should eagerly implement all applicable, common traits." The problem with this comes with the vague definition of applicable, which may indicate that one should simply implement as many common traits as it can, such as Eq and Org. The drawbacks of this:

• If a struct deriving Eq or Ord adds a private field which does not implement one of these, we have to drop that implementation, leading to a major breaking change in the API. It does not seem reasonable to fully impose this guideline and cause that certain future changes to public structs turn into API changes.
  • This has caught my attention because the recently added Clippy trait derive_partial_eq_without_eq is leaving potentially unwanted warnings on any project with a public type deriving PartialEq but not Eq (https://github.com/rust-lang/rust-clippy/issues/9063).
• See also #99: Deriving Ord might not have a meaningful implementation on enums, and it is unclear what benefits are attained from demanding these types to implement Ord and Default when there isn't a meaningful order between variants or a variant which could be considered the default one.

I suggest that the guideline is rephrased to drop the idea of "eagerly" implementing traits, and let developers understand that they should strike a balance between attaining interoperability with the ecosystem and writing a stable API that won't break due to a premature derive.

C-STABLE says:

A crate cannot be stable (>=1.0.0) without all of its public dependencies being stable.

However, Cargo treats unstable versions in a special way:

The version 0.0.x is not considered compatible with any other version.

Since this means that Cargo will not allow any updates of an unstable dependency specified as version 0.0.x because it might contain breaking changes, it seems to me that a stable crate can have an unstable dependency that is specified as version 0.0.x, and that C-STABLE should only apply to crates that have dependencies on the forms 0.x or 0.

Per RFC 3052, the authors field in Cargo.toml is now optional, and this fact should be widely communicated.

See also:

• https://github.com/rust-lang/rfcs/pull/3052
• https://github.com/rust-lang/rust/issues/83227

The motivation for the C-DEREF guideline and the potential problems resulting from ignoring it are (imho) not self explaning. The strongest clue I can find is from the guideline being filed in the Predictability section.

Neither the guideline, the doc or the smart pointer section of The Book explicitly define the difference between smart pointers and general applications of the newtype pattern. Outside the std library contributors there does seem to be confusion (see this old reddit thread). Maybe just referencing C-SMART-PTR would suffice.

Ideally the guideline shouldn't just describe what not to do, but what to do instead: When to impl AsRef/AsMut? What other options are there?