Skip to content

Commit

Permalink
[Experimental] <T as Into<T>>::into lint
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
estebank committed Jan 8, 2025
1 parent a580b5c commit d420b82
Show file tree
Hide file tree
Showing 26 changed files with 219 additions and 29 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_hir_analysis/src/check/wfcheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1120,7 +1120,7 @@ fn check_type_defn<'tcx>(
} else {
// Evaluate the constant proactively, to emit an error if the constant has
// an unconditional error. We only do so if the const has no type params.
let _ = tcx.const_eval_poly(def_id.into());
let _ = tcx.const_eval_poly(def_id);
}
}
let field_id = field.did.expect_local();
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_lint/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,8 @@ lint_reserved_prefix = prefix `{$prefix}` is unknown
lint_reserved_string = will be parsed as a guarded string in Rust 2024
.suggestion = insert whitespace here to avoid this being parsed as a guarded string in Rust 2024
lint_self_type_conversion = this conversion is useless `{$source}` to `{$target}`
lint_shadowed_into_iter =
this method call resolves to `<&{$target} as IntoIterator>::into_iter` (due to backwards compatibility), but will resolve to `<{$target} as IntoIterator>::into_iter` in Rust {$edition}
.use_iter_suggestion = use `.iter()` instead of `.into_iter()` to avoid ambiguity
Expand Down
109 changes: 105 additions & 4 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ use crate::lints::{
BuiltinTrivialBounds, BuiltinTypeAliasBounds, BuiltinUngatedAsyncFnTrackCaller,
BuiltinUnpermittedTypeInit, BuiltinUnpermittedTypeInitSub, BuiltinUnreachablePub,
BuiltinUnsafe, BuiltinUnstableFeatures, BuiltinUnusedDocComment, BuiltinUnusedDocCommentSub,
BuiltinWhileTrue, InvalidAsmLabel,
BuiltinWhileTrue, InvalidAsmLabel, SelfTypeConversionDiag,
};
use crate::nonstandard_style::{MethodLateContext, method_context};
use crate::{
Expand Down Expand Up @@ -1304,9 +1304,9 @@ impl UnreachablePub {
cx.effective_visibilities.effective_vis(def_id).map(|effective_vis| {
effective_vis.at_level(rustc_middle::middle::privacy::Level::Reachable)
})
&& let parent_parent = cx.tcx.parent_module_from_def_id(
cx.tcx.parent_module_from_def_id(def_id.into()).into(),
)
&& let parent_parent = cx
.tcx
.parent_module_from_def_id(cx.tcx.parent_module_from_def_id(def_id).into())
&& *restricted_did == parent_parent.to_local_def_id()
&& !restricted_did.to_def_id().is_crate_root()
{
Expand Down Expand Up @@ -1589,6 +1589,7 @@ declare_lint_pass!(
SoftLints => [
WHILE_TRUE,
NON_SHORTHAND_FIELD_PATTERNS,
SELF_TYPE_CONVERSION,
UNSAFE_CODE,
MISSING_DOCS,
MISSING_COPY_IMPLEMENTATIONS,
Expand Down Expand Up @@ -3063,3 +3064,103 @@ impl EarlyLintPass for SpecialModuleName {
}
}
}

declare_lint! {
/// The `self_type_conversion` lint detects when a call to `.into()` does not have any effect.
///
/// ### Example
///
/// ```rust,compile_fail
/// fn main() {
/// let () = ().into();
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
pub SELF_TYPE_CONVERSION,
Deny,
"",
}

pub struct SelfTypeConversion<'tcx> {
ignored_types: Vec<Ty<'tcx>>,
}

impl_lint_pass!(SelfTypeConversion<'_> => [SELF_TYPE_CONVERSION]);

impl SelfTypeConversion<'_> {
pub fn new() -> Self {
Self { ignored_types: vec![] }
}
}

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 };
tracing::info!("{:#?}", item);
tracing::info!(?path, ?kind);
for res in &path.res {
let Res::Def(DefKind::TyAlias, def_id) = res else { continue };
let ty = cx.tcx.type_of(def_id).instantiate_identity();
let name = cx.tcx.item_name(*def_id);
// println!("{ty:?} {name:?}");
tracing::info!("ignoring {ty:?} {name:?}");
self.ignored_types.push(ty);
for stripped in cx.tcx.stripped_cfg_items(def_id.krate) {
if stripped.name.name == name {
tracing::info!("found stripped {name:#?}");
}
}
}
// FIXME: also look at conditional cfgd uses so to account for things like
// `use std::io::repr_bitpacked` and `std::io::repr_unpacked`.
}

fn check_expr_post(&mut self, cx: &LateContext<'tcx>, expr: &hir::Expr<'_>) {
let hir::ExprKind::MethodCall(_segment, rcvr, args, _) = expr.kind else { return };
if !args.is_empty() {
tracing::info!("non-empty args");
return;
}
let ty = cx.typeck_results().expr_ty(expr);
let rcvr_ty = cx.typeck_results().expr_ty(rcvr);
tracing::info!(?ty, ?rcvr_ty);

if ty != rcvr_ty {
tracing::info!("different types");
return;
}
if self.ignored_types.contains(&rcvr_ty) {
tracing::info!("ignored");
return;
}
let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id) else {
tracing::info!("no type dependent def id");
return;
};
tracing::info!(?def_id);
if Some(def_id) != cx.tcx.get_diagnostic_item(sym::into_fn) {
tracing::info!("not into_fn {:?}", cx.tcx.get_diagnostic_item(sym::into_fn));
return;
}
tracing::info!(?def_id);
tracing::info!(?expr);
if expr.span.macro_backtrace().next().is_some() {
return;
}
if cx.tcx.sess.source_map().span_to_embeddable_string(expr.span).contains("symbolize/gimli")
{
// HACK
return;
}
// println!("{:#?}", self.ignored_types);
cx.emit_span_lint(SELF_TYPE_CONVERSION, expr.span, SelfTypeConversionDiag {
source: rcvr_ty,
target: ty,
});
// bug!("asdf");
}
}
5 changes: 4 additions & 1 deletion compiler/rustc_lint/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ early_lint_methods!(
late_lint_methods!(
declare_combined_late_lint_pass,
[
BuiltinCombinedModuleLateLintPass,
BuiltinCombinedModuleLateLintPass<'tcx>,
[
ForLoopsOverFallibles: ForLoopsOverFallibles,
DefaultCouldBeDerived: DefaultCouldBeDerived::default(),
Expand All @@ -206,6 +206,7 @@ late_lint_methods!(
UnitBindings: UnitBindings,
NonUpperCaseGlobals: NonUpperCaseGlobals,
NonShorthandFieldPatterns: NonShorthandFieldPatterns,
SelfTypeConversion<'tcx>: SelfTypeConversion::new(),
UnusedAllocation: UnusedAllocation,
// Depends on types used in type definitions
MissingCopyImplementations: MissingCopyImplementations,
Expand Down Expand Up @@ -275,6 +276,7 @@ fn register_builtins(store: &mut LintStore) {
store.register_lints(&foreign_modules::get_lints());
store.register_lints(&HardwiredLints::lint_vec());

store.register_late_pass(move |_tcx| Box::new(SelfTypeConversion::new()));
add_lint_group!(
"nonstandard_style",
NON_CAMEL_CASE_TYPES,
Expand Down Expand Up @@ -305,6 +307,7 @@ fn register_builtins(store: &mut LintStore) {
UNUSED_PARENS,
UNUSED_BRACES,
REDUNDANT_SEMICOLONS,
// SELF_TYPE_CONVERSION,
MAP_UNIT_FN
);

Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_lint/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ pub(crate) struct BuiltinNonShorthandFieldPatterns {
pub prefix: &'static str,
}

#[derive(LintDiagnostic)]
#[diag(lint_self_type_conversion)]
pub(crate) struct SelfTypeConversionDiag<'t> {
pub source: Ty<'t>,
pub target: Ty<'t>,
}

#[derive(LintDiagnostic)]
pub(crate) enum BuiltinUnsafe {
#[diag(lint_builtin_allow_internal_unsafe)]
Expand Down
12 changes: 6 additions & 6 deletions compiler/rustc_lint/src/passes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,13 +95,13 @@ macro_rules! expand_combined_late_lint_pass_methods {
/// runtime.
#[macro_export]
macro_rules! declare_combined_late_lint_pass {
([$v:vis $name:ident, [$($pass:ident: $constructor:expr,)*]], $methods:tt) => (
([$v:vis $name:ident$(<$lt:lifetime>)?, [$($pass:ident$(<$inner_lt:lifetime>)?: $constructor:expr,)*]], $methods:tt) => (
#[allow(non_snake_case)]
$v struct $name {
$($pass: $pass,)*
$v struct $name$(<$lt>)? {
$($pass: $pass$(<$inner_lt>)?,)*
}

impl $name {
impl$(<$lt>)? $name$(<$lt>)? {
$v fn new() -> Self {
Self {
$($pass: $constructor,)*
Expand All @@ -115,12 +115,12 @@ macro_rules! declare_combined_late_lint_pass {
}
}

impl<'tcx> $crate::LateLintPass<'tcx> for $name {
impl<'tcx> $crate::LateLintPass<'tcx> for $name$(<$lt>)? {
$crate::expand_combined_late_lint_pass_methods!([$($pass),*], $methods);
}

#[allow(rustc::lint_pass_impl_without_macro)]
impl $crate::LintPass for $name {
impl$(<$lt>)? $crate::LintPass for $name$(<$lt>)? {
fn name(&self) -> &'static str {
panic!()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ impl Subdiagnostic for LocalLabel<'_> {
for dtor in self.destructors {
dtor.add_to_diag_with(diag, f);
}
let msg = f(diag, crate::fluent_generated::mir_transform_label_local_epilogue.into());
let msg = f(diag, crate::fluent_generated::mir_transform_label_local_epilogue);
diag.span_label(self.span, msg);
}
}
Expand Down
6 changes: 2 additions & 4 deletions compiler/rustc_parse/src/parser/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2366,8 +2366,7 @@ fn string_to_tts_1() {
token::Ident(sym::i32, IdentIsRaw::No),
sp(8, 11),
),
])
.into(),
]),
),
TokenTree::Delimited(
DelimSpan::from_pair(sp(13, 14), sp(18, 19)),
Expand All @@ -2383,8 +2382,7 @@ fn string_to_tts_1() {
),
// `Alone` because the `;` is followed by whitespace.
TokenTree::token_alone(token::Semi, sp(16, 17)),
])
.into(),
]),
),
]);

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1119,6 +1119,7 @@ symbols! {
integer_: "integer", // underscore to avoid clashing with the function `sym::integer` below
integral,
into_async_iter_into_iter,
into_fn,
into_future,
into_iter,
intra_doc_pointers,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ pub fn is_const_evaluatable<'tcx>(
Err(
EvaluateConstErr::EvaluationFailure(e)
| EvaluateConstErr::InvalidConstParamTy(e),
) => Err(NotConstEvaluatable::Error(e.into())),
) => Err(NotConstEvaluatable::Error(e)),
Ok(_) => Ok(()),
}
}
Expand Down Expand Up @@ -140,7 +140,7 @@ pub fn is_const_evaluatable<'tcx>(
}
Err(
EvaluateConstErr::EvaluationFailure(e) | EvaluateConstErr::InvalidConstParamTy(e),
) => Err(NotConstEvaluatable::Error(e.into())),
) => Err(NotConstEvaluatable::Error(e)),
Ok(_) => Ok(()),
}
}
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1789,7 +1789,7 @@ impl<T: ?Sized, A: Allocator> Rc<T, A> {
/// let x: Rc<&str> = Rc::new("Hello, world!");
/// {
/// let s = String::from("Oh, no!");
/// let mut y: Rc<&str> = x.clone().into();
/// let mut y: Rc<&str> = x.clone();
/// unsafe {
/// // this is Undefined Behavior, because x's inner type
/// // is &'long str, not &'short str
Expand Down
2 changes: 1 addition & 1 deletion library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2468,7 +2468,7 @@ impl<T: ?Sized, A: Allocator> Arc<T, A> {
/// let x: Arc<&str> = Arc::new("Hello, world!");
/// {
/// let s = String::from("Oh, no!");
/// let mut y: Arc<&str> = x.clone().into();
/// let mut y: Arc<&str> = x.clone();
/// unsafe {
/// // this is Undefined Behavior, because x's inner type
/// // is &'long str, not &'short str
Expand Down
1 change: 1 addition & 0 deletions library/core/src/convert/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ pub trait AsMut<T: ?Sized> {
#[doc(search_unbox)]
pub trait Into<T>: Sized {
/// Converts this type into the (usually inferred) input type.
#[rustc_diagnostic_item = "into_fn"]
#[must_use]
#[stable(feature = "rust1", since = "1.0.0")]
fn into(self) -> T;
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/os/fd/owned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#![stable(feature = "io_safety", since = "1.63.0")]
#![deny(unsafe_op_in_unsafe_fn)]

#![cfg_attr(not(bootstrap), allow(self_type_conversion))]
use super::raw::{AsRawFd, FromRawFd, IntoRawFd, RawFd};
use crate::marker::PhantomData;
use crate::mem::ManuallyDrop;
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys/pal/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ impl Command {
// have the drop glue anyway because this code never returns (the
// child will either exec() or invoke libc::exit)
#[cfg(not(any(target_os = "tvos", target_os = "watchos")))]
#[cfg_attr(not(bootstrap), allow(self_type_conversion))]
unsafe fn do_exec(
&mut self,
stdio: ChildPipes,
Expand Down
1 change: 1 addition & 0 deletions library/std/src/sys_common/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ impl fmt::Debug for CommandEnv {

impl CommandEnv {
// Capture the current environment with these changes applied
#[cfg_attr(not(bootstrap), allow(self_type_conversion))]
pub fn capture(&self) -> BTreeMap<EnvKey, OsString> {
let mut result = BTreeMap::<EnvKey, OsString>::new();
if !self.clear {
Expand Down
2 changes: 1 addition & 1 deletion library/std/src/thread/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ impl Builder {

let id = ThreadId::new();
let my_thread = match name {
Some(name) => Thread::new(id, name.into()),
Some(name) => Thread::new(id, name),
None => Thread::new_unnamed(id),
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fn main() -> Result<(), ()> {
return Ok::<(), ()>(());
Err(())?;
Ok::<(), ()>(());
return Err(().into());
return Err(());
external! {
return Err(())?;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ fn main() -> Result<(), ()> {
return Ok::<(), ()>(());
Err(())?;
Ok::<(), ()>(());
return Err(().into());
return Err(());
external! {
return Err(())?;
}
Expand Down
Loading

0 comments on commit d420b82

Please sign in to comment.