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.

Sign up for GitHub

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

Jump to bottom

False positive for use_self when using core::mem::transmute in From impl #4734

Closed
IlyaSamokhin97 opened this issue Oct 26, 2019 · 0 comments · Fixed by #6179
Closed

False positive for use_self when using core::mem::transmute in From impl #4734

IlyaSamokhin97 opened this issue Oct 26, 2019 · 0 comments · Fixed by #6179
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@IlyaSamokhin97
Copy link

clippy 0.0.212 (3aea860 2019-09-03)

#![warn(clippy::use_self)]

#[repr(C, packed)]
pub struct X {
    pub x: u32,
}

impl From<X> for u32 {
    fn from(c: X) -> Self {
        unsafe {
            core::mem::transmute(c)
        }
    }
}

playground link
@flip1995 flip1995 added A-musing C-bug Category: Clippy is not doing the correct thing labels Oct 26, 2019
@flip1995 flip1995 mentioned this issue Nov 28, 2019
Closed
@flip1995 flip1995 mentioned this issue Jan 15, 2020
Closed
tnielens added a commit to tnielens/rust-clippy that referenced this issue Apr 29, 2020
@tnielens
use_self - add test case of rust-lang#4734
77c4a1f
@tnielens tnielens mentioned this issue Apr 29, 2020
Closed
tnielens added a commit to tnielens/rust-clippy that referenced this issue Apr 29, 2020
@tnielens
use_self - add test case of rust-lang#4734
a7a2bc7
bors added a commit that referenced this issue Oct 13, 2020
@bors
Auto merge of #5531 - montrivo:bugfix/3410-use_self_generic_false_pos…
a12ef13 
…itive, r=<try>

rework use_self impl based on ty::Ty comparison #3410

`use_self` lint refactoring. As suggested by `@eddyb` , implement the lint based on `ty::Ty` comparison instead of `hir::Path` checks.

This PR introduces negative regressions. The cases were covered in the previous implementation. See the test file.

It fixes #3410, #2843, #3859, #4734 and #4140. Should fix #4143 (example doesn't compile). #4305 false positive seems also fixed but the uitest fails to compile the unmodified test case in `use_self.rs.fixed`. Implements #5078.
Generally the implementation checks are done at mir level and offers higher guarantees against false positives. It is probably fixing other unreported false positives.

changelog: fix use_self generics false positives
@flip1995 flip1995 mentioned this issue Oct 16, 2020
Merged
@giraffate giraffate mentioned this issue Oct 26, 2020
Closed
@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
@bors
Auto merge of #6179 - flip1995:rewrite_use_self, r=phansch
a24fa49 
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.
@flip1995 flip1995 removed the A-musing label Feb 6, 2021
bors added a commit that referenced this issue Feb 10, 2021
@bors
Auto merge of #6179 - flip1995:rewrite_use_self, r=<try>
3ad10ff 
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
Assignees
No one assigned
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Rework use_self impl based on ty::Ty comparison #3410 | Take 2 flip1995/rust-clippy
3 participants
@phansch @flip1995 @IlyaSamokhin97