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

New lint suggestion: Avoid inherent methods on generic smart pointers #11820

Open
madsmtm opened this issue Nov 15, 2023 · 1 comment
Open

New lint suggestion: Avoid inherent methods on generic smart pointers #11820

madsmtm opened this issue Nov 15, 2023 · 1 comment
Labels
A-lint Area: New lints

Comments

@madsmtm
Copy link

madsmtm commented Nov 15, 2023

What it does

Generic smart pointers like Box<T>, Rc<T> and so on are recommended to use associated functions instead of inherent methods to avoid compatibility hazards with Deref. As an example, see Box::leak. This is also documented in the Rust API Guidelines.

This lint warns on every inherent method on types that implement Deref in a fully generic fashion.

(In particular, it should activate when type Target = T, but not when type Target = u8 or type Target = [T]).

Advantage

  • Avoid confusing overlapping methods.
  • Prevent SemVer issues with adding new inherent methods to smart pointers (which would potentially be shadowing user-defined methods).

Drawbacks

  • Using the new code is slightly more cumbersome for the user.
  • Changing the code as the lint suggests would be a breaking change for an already released crate.

Example

use std::ops::Deref;

pub struct MySmartPointer<T>(pub T);

impl<T> Deref for MySmartPointer<T> {
    type Target = T;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

impl<T> MySmartPointer<T> {
    pub fn foo(&self) {
        unimplemented!()
    }
}

Could be written as:

// ... snip

impl<T> MySmartPointer<T> {
    pub fn foo(this: &Self) {
        unimplemented!()
    }
}

Or, in the case where the user has read and understood the drawbacks, be explicitly allowed:

// ... snip

impl<T> MySmartPointer<T> {
    #[allow(clippy::bikeshed_lint_name)]
    pub fn foo(&self) {
        unimplemented!()
    }
}
@Centri3
Copy link
Member

Centri3 commented Nov 19, 2023

Some (sometimes unjustifiably) use Deref for newtypes, even large projects like bevy. This would afaik lint if it's generic, as an example, struct Meters<T: num::Float>(pub T)

In the case fully generic means no trait bounds, yeah this would work. Even if it's a newtype/marker type that would still be prone to issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

2 participants