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

Refine lints about field and method removals to account for Deref/DerefMut #766

Open
obi1kenobi opened this issue Apr 19, 2024 · 0 comments
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.

Comments

@obi1kenobi
Copy link
Owner

obi1kenobi commented Apr 19, 2024

This issue about trait associated consts and functions acting as a backstop for removed inherent items got me thinking, what about Deref? Is the following breaking if the // REMOVE lines are removed?

pub struct Foo {
    pub field: (), // REMOVE
    inner: Inner,
}

impl Foo {
    pub fn method(&self) {} // REMOVE

    pub fn new() -> Self {
        Self {
            field: (), // REMOVE
            inner: Inner { field: () },
        }
    }
}

pub struct Inner {
    pub field: (),
}

impl Inner {
    pub fn method(&self) {}
}

impl core::ops::Deref for Foo {
    type Target = Inner;
    fn deref(&self) -> &Self::Target {
        &self.inner
    }
}

impl core::ops::DerefMut for Foo {
    fn deref_mut(&mut self) -> &mut Self::Target {
        &mut self.inner
    }
}

fn test() {
    let mut foo = Foo::new();

    foo.field = ();
    foo.method();
}

Originally posted by @jw013 in #727 (comment)

The long-term solution we want here is similar to the case of clippy::correctness vs clippy::suspicious:

  • We want correctness-flavored lints (on by default, error level) that allow Deref/DerefMut to be used to replace the removed item.
  • We want suspicious-flavored lints (on by default, warning level) that do not allow the above, and never fire if their correctness sibling is already going to fire.

More discussion here: #727 (comment)

The suspicious-flavored lint is blocked on #58.

The correctness-flavored lints already exist and can be tweaked, though this is not a good onboarding task! It requires proficiency with the Trustfall query language, so highly recommend writing some other lints first!

@obi1kenobi obi1kenobi added A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue. labels Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: new or existing lint C-enhancement Category: raise the bar on expectations E-help-wanted Call for participation: Help is requested to fix this issue. E-mentor Call for participation: Mentorship is available for this issue.
Projects
None yet
Development

No branches or pull requests

1 participant