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

Remove unnecessary into conversions #15042

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Jan 9, 2025

"identity" .into() calls where the base's type isn't changed is a future compatibility foot-gun (like the issue we had with time a few months ago) as new impl Into blocks can cause previously compiling code to start failing. I don't foresee these ones in particular causing problems anytime soon, but I noticed them and might as well clean them up as a drive-by.

@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2025

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-git Area: anything dealing with git S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 9, 2025
@estebank
Copy link
Contributor Author

estebank commented Jan 9, 2025

I noticed these while working on rust-lang/rust#129249. I am not confident that there's no platform specific shenanigans that make these .into() calls necessary, like they it is for:

error: this conversion is useless `u64` to `u64`
   --> src/tools/cargo/src/cargo/core/shell.rs:611:49
    |
611 |             if libc::ioctl(libc::STDERR_FILENO, libc::TIOCGWINSZ.into(), &mut winsize) < 0 {
    |
    = note: `#[deny(self_type_conversion)]` on by default

I am working on correctly handling that case (it shouldn't warn at all), but in the other two cases it does seem like the lint caught real cases of this.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks Esteban. Just want to confirm this doesn't really block rust-lang/rust#129249, right? I am collectin data for cargo-as-subtree in rust-lang/rust and wonder how painful it is not.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks Esteban. Just want to confirm this doesn't really block rust-lang/rust#129249, right? I am collectin data for cargo-as-subtree in rust-lang/rust and wonder how painful it is not.

@weihanglo weihanglo enabled auto-merge January 9, 2025 19:34
@weihanglo weihanglo added this pull request to the merge queue Jan 9, 2025
@estebank
Copy link
Contributor Author

estebank commented Jan 9, 2025

@weihanglo it doesn't block it, I worked around it on that PR for now (right now I only care about getting to the point of being able to run crater), but will become a nuisance at an indeterminate time in the future. I'm confident this can just ride the train without it becoming an issue for me (given my current speed on finishing that PR). Thanks for the concern! The PR in the end might not be landed (I'd push the new logic to clippy), or might be allow-by-default. Either way it wouldn't block progress for these to be present in our tooling. I just wanted to clean up some of the things I was seeing.

Merged via the queue into rust-lang:master with commit 9589831 Jan 9, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-git Area: anything dealing with git 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.

4 participants