Add rc_buffer lint for checking Rc<String> and friends

[rschoon]

Fixes #2623

This is a bit different from the original PR attempting to implement this type of lint. Rather than linting against converting into the unwanted types, this PR lints against declaring the unwanted type in a struct or function definition.

I'm reasonably happy with what I have here, although I used the fully qualified type names for the Path and OsString suggestions, and I'm not sure if I should have just used the short versions instead, even if they might not have been declared via use.

Also, I don't know if "buffer type" is the best way to put it or not. Alternatively I could call it a "growable type" or "growable buffer type", but I was thinking of PathBuf when I started making the lint.

changelog: Add rc_buffer lint

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @yaahc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[yaahc]

This is a bit different from the original PR attempting to implement this type of lint. Rather than linting against converting into the unwanted types, this PR lints against declaring the unwanted type in a struct or function definition.

👍

I'm reasonably happy with what I have here, although I used the fully qualified type names for the Path and OsString suggestions, and I'm not sure if I should have just used the short versions instead, even if they might not have been declared via use.

I think using fully qualified paths is fine. It can always be changed later if it's causing issues.

[yaahc]

the changes look great btw, once we fix the dogfood issue I'll be happy to approve this.

[yaahc]

@bors r+

[bors]

📌 Commit 6c056d3 has been approved by yaahc

[bors]

⌛ Testing commit 6c056d3 with merge e636b88...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: yaahc
Pushing e636b88 to master...

[yoshuawuyts]

We've hit this lint in Surf and are moving to disable it: http-rs/surf#242. We use a similar structure for middleware in Tide as well. This may be an interesting case study for legitimate uses that are disallowed by this lint.

[Fishrock123]

So to elaborate more on Tide & Surf's usage:

The structure we have is Arc<Vec<Arc<dyn Middleware>>>, in both tide::Server and surf::Client.

The outer Arc exists to be able to handle things per-request, in both Tide and Surf, in both cases to get around a bunch of lifetime indirection I really don't understand. Both are constructed at runtime, so we need a Vec so people can add middleware (via .with(middleware)). The inner Arc exists due to an inner clone in Surf, and sharing between routes in Tide.

In both cases, the following information from the lint is not true:

    /// While mutating a buffer type would still be possible with Rc::get_mut(), it only
    /// works if there are no additional references yet, which defeats the purpose of
    /// enclosing it in a shared ownership type. Instead, additionally wrapping the inner
    /// type with an interior mutable container (such as RefCell or Mutex) would normally
    /// be used.

This is because the behavior described is useful to both Surf and Tide. Middleware needs to be mutable up-front, but once a Client or Server is executing on a request, messing with the middleware would lead to unexpected (and probably zalgo-esque timing bug) behavior, so having it panic is fine, because that is considered a programmer error. We could use an extra RefCell or Mutex but this brings us extra overhead and does less of what we'd want.