<T as Into<T>>::into lint#129249
Open
estebank wants to merge 13 commits into
Open
Conversation
Collaborator
|
r? @chenyukang rustbot has assigned @chenyukang. Use |
rustbot
added
O-unix
Collaborator
|
Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
Contributor
Author
|
@bors try @craterbot check |
Contributor
Author
|
@bors try |
This comment was marked as outdated.
This comment was marked as outdated.
estebank
commented
Aug 18, 2024
Contributor
Author
|
@bors try |
Collaborator
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 18, 2024
[Experimental] `<T as Into<T>>::into` lint Running crater to see how common that pattern is. The Lint would have to be at most warn-by-default because there are a handful of cases detected that are actually perfectly reasonable (`type` aliases with per-platform `cfg`, or macros) which are now at best half-heartedly handled. I've detected a handful of cases where we're calling `.into()` unnecessarily in the `rustc` codebase as well, and changed those.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Collaborator
|
💔 Test failed - checks-actions |
bors
added
S-waiting-on-author
Member
|
🤔 looks like you are reimplementing a subset of https://rust-lang.github.io/rust-clippy/master/index.html#/useless_conversion ? edit: spoiler |
Member
|
looks a bit like a mix of |
This comment has been minimized.
This comment has been minimized.
traviscross
added
the
S-experimental
Contributor
Author
|
@matthiaskrgr I'm trying to gauge how common the root cause of the recent |
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
Author
|
@craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-157814-crater-rollup/retry-regressed-list.txt Rerunning crater with warn-by-default, to see what the real impact will be. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as duplicate.
This comment was marked as duplicate.
Collaborator
|
🎉 Experiment
Footnotes
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
|
By reading the recent comments history (linking them here for convenience, comment and this comment), I'll try to roll someone from t-compiler. r? compiler |
Contributor
Author
👍
I feel like at this point we'd like some t-lang input to verify that the current set of checks is what we want, t-compiler (for impl) and t-libs-api (for the new lint surface), with the go-no-go laying mostly on the latter. I know that @joshtriplett will likely want to take another pass at this. I know that we need to improve the wording of the lint. I also know that this code, as is, wouldn't have caught time's issue because that one was behind
I love the bot-added link with all the comments :) https://triagebot.infra.rust-lang.org/gh-comments/rust-lang/rust/pull/129249 It'd make sense to make some progress on #157218 to make this PR easier to land too. |
When annotating an item with `#[cfg]` we track both the item that got annotated (with an inert attr) and the names of items that got directly cfg'd out. Extend this mechanism to also work for items within a `cfg_select!`.
Add two lints, very similar to `clippy::useless_conversion`, detecting when `.into()` is being called unnecessarily. We present two separate lints, one that triggers inside of macros, because it is common for macros to have `.into()` calls to make the caller side easier to use. In those cases people should still use the fully-qualified path instead, but allow them to silence that lint without silencing the more general, more likely to be problematic case. This lint allows us to protect developers from the semver-hazard that time 0.3.34 encountered, where a std change caused a valid method chain to start producing inference errors. This is explicitly allowed by the Rust project's backwards compatibility guarantees (inference is not included in them), which means that relying on the blanket `impl Into` is a problem. The lint as implemented has false negatives: because of the way type aliases are handled by the type system, we can't know whether `let _: i32 = 0i32.into()` corresponds to a call on `i32` *or* a call on a type alias that is `i32` only on some platforms (like `#[cfg(..)] type Int = i32;`). To avoid false positives, we keep track of type aliases that have been imported in the local crate and mark their types for exclusion. This means that calling `let _: i32 = Int::into(0i32);` will not be linted against even though it should.
This side-steps a lot of `libc` cases.
This makes the visitor actually visit closure bodies, as expected.
…ents Account for `foo.map(Into::into)`, and modify the output to include structured suggestions.
Collaborator
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
chenyukang
reviewed
Jul 5, 2026
| added in the future causing type inference errors" | ||
| )] | ||
| #[note( | ||
| "you can instead use the fully-qualified path `<{$ty} as Into<{$ty}>::into(val) to avoid \ |
Member
There was a problem hiding this comment.
missing a > here in the note.
chenyukang
reviewed
Jul 5, 2026
| /// lint will not detect that. | ||
| pub SELF_TYPE_CONVERSION, | ||
| Warn, | ||
| "unnecessary call to `.into()`", |
Member
There was a problem hiding this comment.
seems there is no tests cover these two new unnecessary call to lints.
chenyukang
reviewed
Jul 5, 2026
|
|
||
| impl<'tcx> LateLintPass<'tcx> for SelfTypeConversion<'tcx> { | ||
| fn check_item_post(&mut self, cx: &LateContext<'tcx>, item: &hir::Item<'_>) { | ||
| let hir::ItemKind::Use(path, _kind) = item.kind else { return }; |
Member
There was a problem hiding this comment.
alias may happen in the same module?
such as:
#[cfg(target_arch = "avr")]
type Int = i16;
#[cfg(not(target_arch = "avr"))]
type Int = i32;
fn f() {
let x: Int = 1;
let _: i32 = x.into();
}the into may need for different arch, and it's without a use.
chenyukang
reviewed
Jul 5, 2026
| // of platform specific code. This kind of code often ends up with `.into()` method | ||
| // calls that are useless in some configurations, but *necessary* in others. As a | ||
| // first-order approximation, we ignore *all* `val_of_ty.into()` calls. | ||
| self.ignored_types.insert(ty); |
Member
There was a problem hiding this comment.
We adds types to ignored_types only when it visits a use item, while check_expr_post is also consults this set when it visits an .into() expression. This makes the lint result depend on item visitation order.
For example, if a use aliases::Int; item appears before a function, the alias type may already be in ignored_types, so x.into() is skipped. But if the same use item appears after the function, the function body can be linted before the alias is collected, causing a false positive.
Since rust item order should not affect this lint result, a more robust approach would be to collect cfg-sensitive aliases in an order-independent prepass before checking?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
16 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
Add two lints, very similar to
clippy::useless_conversion, detecting when.into()is being called unnecessarily. We present two separate lints, one that triggers inside of macros, because it is common for macros to have.into()calls to make the caller side easier to use. In those cases people should still use the fully-qualified path instead,but allow them to silence that lint without silencing the more general, more likely to be problematic case.
This lint allows us to protect developers from the semver-hazard that time 0.3.34 encountered, where a std change caused a valid method chain to start producing inference errors. This is explicitly allowed by the Rust project's backwards compatibility guarantees (inference is not included in them), which means that relying on the blanket
impl Intois a problem.The lint as implemented has false negatives: because of the way type aliases are handled by the type system, we can't know whether
let _: i32 = 0i32.into()corresponds to a call oni32or a call on a type alias that isi32only on some platforms (like#[cfg(..)] type Int = i32;). To avoid false positives, we keep track of type aliases that have been imported in the local crate and mark their types for exclusion. This means that callinglet _: i32 = Int::into(0i32);will not be linted against even though it should.Running crater to see how common that pattern is. The Lint would have to be at most warn-by-default because there are a handful of cases detected that are actually perfectly reasonable (
typealiases with per-platformcfg, or macros) which are now at best half-heartedly handled.I've detected a handful of cases where we're calling
.into()unnecessarily in therustccodebase as well, and changed those.CC #127343.