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

use_self lint fires on code expanded from a macro #4887

Closed
Lokathor opened this issue Dec 7, 2019 · 5 comments · Fixed by #6179
Closed

use_self lint fires on code expanded from a macro #4887

Lokathor opened this issue Dec 7, 2019 · 5 comments · Fixed by #6179
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have T-macros Type: Issues with macros and macro expansion

Comments

@Lokathor
Copy link

Lokathor commented Dec 7, 2019

When using a macro that expands to a particular fixed type, if you happen to use that macro in a context where Self could have been used the use_self lint will trigger and tell you to fix the macro.

Any way to make it not trigger on macro expansions?

@basil-cow
Copy link
Contributor

basil-cow commented Dec 7, 2019

I believe it was an explicit change introduced here, it used to not trigger on macro expansions, after the change it only triggers in local macro expansions.

@flip1995
Copy link
Member

flip1995 commented Dec 11, 2019

@Areredify is right: Issue: #2098, PR: #3627

You can fix a macro like this:

macro_rules! mac {
    ($ty:ident) => {{
        impl $ty {
            fn new() -> $ty {
                $ty { /* Some fields */ };
            }
        }
    }}
};

with:

macro_rules! mac {
    ($ty:ident) => {{
        impl $ty {
            fn new() -> Self {
                Self { /* Some fields */ };
            }
        }
    }}
};

If you found a case, where this is not possible, feel free to report it here and I'll reopen this issue.

@Lokathor
Copy link
Author

I... absolutely don't follow your example code there. Those are very different. One is a trait impl and the other is an associated function impl.

Either way, my macro is this one here: https://github.com/Lokathor/wide/blob/master/src/m_f32x4.rs#L67 (as well as another similar one for i32x4, but let's focus on just one since a fix to this would fix the other)

And usages... all over the place in the module.

  • Some are in contexts where Self would correctly give f32x4.
  • Some are in contexts where Self would not give f32x4.
  • In fact to be generally useful the macro must be valid in module scope where there's no Self at all.

@flip1995
Copy link
Member

Oh, I simplified my example from a Trait impl to a simple impl, but forgot to edit a line. edited it now, sorry for the confusion.

Now I see your points. The easiest thing to do here would be just to disable it in macros again.

If we could distinguish if the self type came from $ty or just ty (was expanded or hard coded) we could disable the lint for types where the type is hard coded. We then had to address this

In fact to be generally useful the macro must be valid in module scope where there's no Self at all.

in some way. Maybe only lint in macros when we're inside an impl/trait definition.

@flip1995 flip1995 reopened this Dec 11, 2019
@flip1995 flip1995 added E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy T-macros Type: Issues with macros and macro expansion C-bug Category: Clippy is not doing the correct thing labels Dec 11, 2019
@ArekPiekarz
Copy link

I've stumbled upon this bug when trying to pass the type name of Self into panic!:

use std::any::type_name;

trait ToI32 {
    fn to_i32(&self) -> i32;
}

impl ToI32 for f64 {
    fn to_i32(&self) -> i32 {
        if !self.is_finite() {
            panic!("Cannot convert {}: {} into i32", type_name::<Self>(), self);
        }
        *self as i32
    }
}

fn main() {
}

which emitted a warning:

warning: unnecessary structure name repetition
  --> src/main.rs:10:13
   |
10 |             panic!("Cannot convert {}: {} into i32", type_name::<Self>(), self);
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |

@phansch phansch added the I-false-positive Issue: The lint was triggered on code it shouldn't have label Dec 18, 2020
bors added a commit that referenced this issue Jan 19, 2021
Rework use_self impl based on ty::Ty comparison #3410 | Take 2

This builds on top of #5531

I already reviewed and approved the commits by `@montrivo.` So only the review of my commits should be necessary.

I would also appreciate your review `@montrivo,` since you are familiar with the challenges here.

Fixes #3410 and Fixes #4143 (same problem)
Fixes #2843
Fixes #3859
Fixes #4734 and fixes #6221
Fixes #4305
Fixes #5078 (even at expression level now 🎉)
Fixes #3881 and Fixes #4887 (same problem)
Fixes #3909

Not yet: #4140 (test added)

All the credit for the fixes goes to `@montrivo.` I only refactored and copy and pasted his code.

changelog: rewrite [`use_self`] lint and fix multiple (8) FPs. One to go.
bors added a commit that referenced this issue Feb 10, 2021
Rework use_self impl based on ty::Ty comparison #3410 | Take 2

This builds on top of #5531

I already reviewed and approved the commits by `@montrivo.` So only the review of my commits should be necessary.

I would also appreciate your review `@montrivo,` since you are familiar with the challenges here.

Fixes #3410 and Fixes #4143 (same problem)
Fixes #2843
Fixes #3859
Fixes #4734 and fixes #6221
Fixes #4305
Fixes #5078 (even at expression level now 🎉)
Fixes #3881 and Fixes #4887 (same problem)
Fixes #3909

Not yet: #4140 (test added)

All the credit for the fixes goes to `@montrivo.` I only refactored and copy and pasted his code.

changelog: rewrite [`use_self`] lint and fix multiple (8) FPs. One to go.
@bors bors closed this as completed in 605e9ba Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing E-medium Call for participation: Medium difficulty level problem and requires some initial experience. good-first-issue These issues are a good way to get started with Clippy I-false-positive Issue: The lint was triggered on code it shouldn't have T-macros Type: Issues with macros and macro expansion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants