Repository navigation
<T as Into<T>>::into lint - #129249
Open
estebank wants to merge 3 commits into
Open
<T as Into<T>>::into lint#129249estebank wants to merge 3 commits into
<T as Into<T>>::into lint#129249estebank wants to merge 3 commits into
Conversation
Collaborator
|
r? @chenyukang rustbot has assigned @chenyukang. Use |
rustbot
added
O-unix
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
estebank
commented
Aug 18, 2024
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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.
This comment was marked as outdated.
This comment was marked as outdated.
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 has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
@bors try |
This comment has been minimized.
This comment has been minimized.
Contributor
Contributor
Author
|
@craterbot check |
Collaborator
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
Member
|
I haven't been clear on the status of this PR. It sounds like it's ready for my review now? |
Contributor
Author
|
@camelid I am confident that the code is now in shape to avoid common false positives, but wanted to wait until after the crater run to request a review, in case I found any show-stopper unexpected breakage in the wild. The only iffy thing I might have to modify is the |
Draft
Contributor
|
Crater rollup in #164148 @craterbot abort |
Contributor
|
@craterbot abort |
Collaborator
|
🗑️ Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
This comment has been minimized.
This comment has been minimized.
estebank
commented
Oct 13, 2026
Contributor
Author
There was a problem hiding this comment.
I believe the code is ready for review. Other than one explicit skip for crates.io and cargo that I'll remove after crater comes clean, I stand by the filtering logic. I haven't expanded to also support From::from calls: adding it shouldn't be too hard, but also I am less convinced those are as problematic...
Comment on lines
+3512
to
+3514
| if ["crates_io", "cargo"].contains(&cx.tcx.crate_name(hir::def_id::LOCAL_CRATE).as_str()) { | ||
| return; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Feel free to ignore this, it'll have to be removed before landing (as warn-by-default). I'm pretty certain that the triggers were valid, but doing this was the easiest way of getting the crater run to go.
Comment on lines
+3433
to
+3497
| if let hir::ExprKind::Field(base, field) = rcvr.kind { | ||
| // Look at the original type. If the field being accessed or its type is behind a `cfg` | ||
| // attribute, we don't trigger the lint, as it's likely that field has different types | ||
| // in different configurations. | ||
| // | ||
| // #[cfg(true)] | ||
| // struct S { | ||
| // #[cfg(..)] field: u32, | ||
| // #[cfg(not(..))] field: u64, | ||
| // } | ||
| // | ||
| // let s = S { field: 0 }; | ||
| // let x: u32 = s.field.into(); | ||
| let ty = cx.typeck_results().expr_ty(base); | ||
| if let ty::Adt(def, _args) = ty.peel_refs().kind() | ||
| && struct_field_has_cfg(cx.tcx, def.did(), field.name) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
| if let hir::Node::ExprField(field) = cx.tcx.parent_hir_node(expr.hir_id) | ||
| && let hir::Node::Expr(parent) = cx.tcx.parent_hir_node(field.hir_id) | ||
| && let hir::ExprKind::Struct(hir::QPath::Resolved(_, path), _, _) = parent.kind | ||
| && let Res::Def(DefKind::Struct, def_id) = path.res | ||
| && struct_field_has_cfg(cx.tcx, def_id, field.ident.name) | ||
| { | ||
| // The target of the value being converted corresponds to a struct that is behind a | ||
| // `cfg`: | ||
| // | ||
| // #[cfg(true)] | ||
| // struct S { | ||
| // #[cfg(..)] field: u32, | ||
| // #[cfg(not(..))] field: u64, | ||
| // } | ||
| // | ||
| // S { field: 0u32.into() }; | ||
| return; | ||
| } | ||
| if let hir::ExprKind::Path(hir::QPath::Resolved(_, path)) = rcvr.kind | ||
| && let Res::Def(DefKind::Const { .. }, def_id) = path.res | ||
| && (find_attr!(cx.tcx, def_id, CfgTrace(..) | CfgAttrTrace(..)) | ||
| || find_attr!(cx.tcx, cx.tcx.parent(def_id), CfgTrace(..) | CfgAttrTrace(..))) | ||
| { | ||
| // We're accessing and converting a const that is either itself annotated with `cfg` or | ||
| // its parent `mod`. Common for things like `libc::TIOCGWINSZ`. | ||
| return; | ||
| } | ||
|
|
||
| if let hir::ExprKind::MethodCall(_segment, _rcvr, _args, _) = rcvr.kind | ||
| && let Some(def_id) = cx.typeck_results().type_dependent_def_id(rcvr.hir_id) | ||
| && let output = cx.tcx.fn_sig(def_id).skip_binder().output().skip_binder() | ||
| && let ty::Alias(_, ty::AliasTy { kind: ty::Projection { def_id }, .. }) = output.kind() | ||
| && let bounds = cx.tcx.item_bounds(*def_id).instantiate_identity().skip_norm_wip() | ||
| && bounds.iter().any(|bound| { | ||
| Some(true) | ||
| == bound | ||
| .as_trait_clause() | ||
| .map(|c| Some(c.def_id()) == cx.tcx.get_diagnostic_item(sym::Into)) | ||
| }) | ||
| { | ||
| // We called `.into()` in an assoc `fn foo() -> Self::Type` where `type Type: Into<_>;`. | ||
| // We can assume that calling `.into()` on this value is the *intended use*. | ||
| // We saw this in interaction of `object`+`gimli` crates. | ||
| return; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
There might be other common patterns that the ecosystem uses that we might want to add to this list of heuristics. A case I could foresee is more than one level of mod nesting where the outer-most is cfgd, but we didn't see that in the toolchain, so I didn't include that level of flexibility.
Comment on lines
+3499
to
+3510
| if let Some(expn) = expr.span.macro_backtrace().next() { | ||
| if expn.macro_def_id.map_or(false, |did| did.is_local()) { | ||
| cx.emit_span_lint( | ||
| SELF_TYPE_CONVERSION_IN_MACRO, | ||
| expr.span, | ||
| SelfTypeConversionInMacroDiag { ty }, | ||
| ); | ||
| } | ||
| // A macro that expands to this code isn't great, but end-users of a macro can't do | ||
| // anything about it. | ||
| return; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
I'd want confirmation that we actually want to lint macro expansions at all. My expectation is that it would be allow-by-default and people could opt-into it on their own crates...
Comment on lines
+197
to
+202
| #[suggestion( | ||
| "consider removing the conversion call", | ||
| style = "verbose", | ||
| code = "", | ||
| applicability = "machine-applicable" | ||
| )] |
Contributor
Author
There was a problem hiding this comment.
Should we add an alternative suggestion to expect the lint on the enclosing item too, with an explanation that the lint can sometimes be over-eager?
Comment on lines
+339
to
+342
| store.register_lints(&SelfTypeConversion::lint_vec()); | ||
| store.register_late_lint_pass(Box::new(|_| { | ||
| Box::new(SelfTypeConversion { ignored_types: Default::default() }) | ||
| })); |
Contributor
Author
There was a problem hiding this comment.
Registering the lint this way allowed for ConditionalTypeCollector to run once per crate.
Comment on lines
436
to
439
| fn into_raw(self) -> i32 { | ||
| #[allow(self_type_conversion)] | ||
| self.as_inner().into_raw().into() | ||
| } |
Contributor
Author
There was a problem hiding this comment.
The level of indirection I need to account for to correctly allow this case seems a bit more than what we'd want to encode in the lint, unless it is a common pattern out in the ecosystem.
estebank
commented
Oct 13, 2026
This comment has been minimized.
This comment has been minimized.
estebank
commented
Oct 13, 2026
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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. Ignore useless `.into()` calls on fields or structs that are behind a `cfg` attr. Ignore useless `.into()` calls on `const`s that are behind a cfg attr. This side-steps a lot of `libc` cases. Account for `foo.map(Into::into)` and `foo.map_err(Into::into)`. Account for macros in suggestions. Do not emit a suggestion to remove the `.into()` call when the `Span` context of the receiver is different to the Span context of the expression to avoid malformed suggestions. The lint still triggers regardless, just with less information. Account for `object`+`gimli` interaction where an assoc fn returns `type T: Into<u64>;`
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 has been minimized.
This comment has been minimized.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
This branch has not been deployed
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
17 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.CC #127343.