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: unnecessary_fallible_conversions #11669

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

y21
Copy link
Member

@y21 y21 commented Oct 14, 2023

Closes #11577

A new lint that looks for calls such as i64::try_from(1i32) and suggests i64::from(1i32). See lint description (and linked issue) for more details for why.

There's a tiny bit of overlap with the useless_conversion lint, in that the other one warns T::try_from(T) (i.e., fallibly converting to the same type), so this lint ignores cases like i32::try_from(1i32) to avoid emitting two warnings for the same expression.

Also, funnily enough, with this one exception, this lint would warn on exactly every case in the useless_conversion_try ui test that useless_conversion didn't cover (but never two warnings at the same time), which is neat. I did add an #![allow] though since we don't want interleaved warnings from multiple lints in the same uitest.

changelog: new lint: unnecessary_fallible_conversions

@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2023

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 14, 2023
Copy link
Contributor

@sgued sgued left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the implementation of my suggested lint!

@bors
Copy link
Contributor

bors commented Oct 26, 2023

☔ The latest upstream changes (presumably #11698) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Contributor

@Jarcho Jarcho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. Just a couple of small nits. If you don't feel like rewording things it's workable the way it is.

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved
@Jarcho
Copy link
Contributor

Jarcho commented Oct 31, 2023

Thanks for the small cleanup. @bors r+

@bors
Copy link
Contributor

bors commented Oct 31, 2023

📌 Commit 69c3b9c has been approved by Jarcho

It is now in the queue for this repository.

@Jarcho
Copy link
Contributor

Jarcho commented Oct 31, 2023

@bors r-

@Jarcho
Copy link
Contributor

Jarcho commented Oct 31, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Oct 31, 2023

📌 Commit 69c3b9c has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 31, 2023

⌛ Testing commit 69c3b9c with merge 7d34406...

@bors
Copy link
Contributor

bors commented Oct 31, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 7d34406 to master...

@bors bors merged commit 7d34406 into rust-lang:master Oct 31, 2023
7 of 8 checks passed
@alexanderkjeldaas
Copy link

Just a maybe unrelated comment on this, but would a lint that finds unnecessary TryFrom implementations that instead could be just From be implemented alongside this one?

impl TryFrom<X> for Y {
    type Error = <Foo as FromStr>::Err;
    fn try_from(x: X) -> Result<Self, Self::Error> {
        Ok(Self {
            height: x.height.0.as_u64(),
            hash: x.hash.0,
        })
    }
}

Easy to detect when the return value is always Ok?

@flip1995
Copy link
Member

flip1995 commented Jan 4, 2024

Looks like a possible lint. If you want this, please open an issue or if you want to get into Clippy development consider opening a PR for it :)

arpad-m added a commit to neondatabase/neon that referenced this pull request Jan 9, 2024
This fixes the clippy lint firing on macOS on the conversion which
needed for portability. For some reason, the logic in
rust-lang/rust-clippy#11669 to avoid an overlap
is not working.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new lint: unnecessary_faillible_conversion
7 participants