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

Add more cases to the useless_conversion lint #13756

Merged
merged 2 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions clippy_lints/src/methods/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4982,6 +4982,10 @@ impl Methods {
}
map_identity::check(cx, expr, recv, m_arg, name, span);
manual_inspect::check(cx, expr, m_arg, name, span, &self.msrv);
crate::useless_conversion::check_function_application(cx, expr, recv, m_arg);
},
("map_break" | "map_continue", [m_arg]) => {
crate::useless_conversion::check_function_application(cx, expr, recv, m_arg);
},
("map_or", [def, map]) => {
option_map_or_none::check(cx, expr, recv, def, map);
Expand Down
64 changes: 60 additions & 4 deletions clippy_lints/src/useless_conversion.rs
Original file line number Diff line number Diff line change
@@ -1,18 +1,20 @@
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::{snippet, snippet_with_applicability, snippet_with_context};
use clippy_utils::sugg::Sugg;
use clippy_utils::sugg::{DiagExt as _, Sugg};
use clippy_utils::ty::{is_copy, is_type_diagnostic_item, same_type_and_consts};
use clippy_utils::{get_parent_expr, is_trait_method, is_ty_alias, path_to_local};
use clippy_utils::{
get_parent_expr, is_inherent_method_call, is_trait_item, is_trait_method, is_ty_alias, path_to_local,
};
use rustc_errors::Applicability;
use rustc_hir::def_id::DefId;
use rustc_hir::{BindingMode, Expr, ExprKind, HirId, MatchSource, Node, PatKind};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_infer::traits::Obligation;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::traits::ObligationCause;
use rustc_middle::ty::{self, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt};
use rustc_middle::ty::{self, AdtDef, EarlyBinder, GenericArg, GenericArgsRef, Ty, TypeVisitableExt};
use rustc_session::impl_lint_pass;
use rustc_span::{Span, sym};
use rustc_span::{Span, Symbol, sym};
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;

declare_clippy_lint! {
Expand Down Expand Up @@ -382,3 +384,57 @@ impl<'tcx> LateLintPass<'tcx> for UselessConversion {
}
}
}

/// Check if `arg` is a `Into::into` or `From::from` applied to `receiver` to give `expr`, through a
/// higher-order mapping function.
pub fn check_function_application(cx: &LateContext<'_>, expr: &Expr<'_>, recv: &Expr<'_>, arg: &Expr<'_>) {
if has_eligible_receiver(cx, recv, expr)
&& (is_trait_item(cx, arg, sym::Into) || is_trait_item(cx, arg, sym::From))
&& let ty::FnDef(_, args) = cx.typeck_results().expr_ty(arg).kind()
&& let &[from_ty, to_ty] = args.into_type_list(cx.tcx).as_slice()
&& same_type_and_consts(from_ty, to_ty)
{
span_lint_and_then(
cx,
USELESS_CONVERSION,
expr.span.with_lo(recv.span.hi()),
format!("useless conversion to the same type: `{from_ty}`"),
|diag| {
diag.suggest_remove_item(
cx,
expr.span.with_lo(recv.span.hi()),
"consider removing",
Applicability::MachineApplicable,
);
},
);
}
}

fn has_eligible_receiver(cx: &LateContext<'_>, recv: &Expr<'_>, expr: &Expr<'_>) -> bool {
let recv_ty = cx.typeck_results().expr_ty(recv);
if is_inherent_method_call(cx, expr)
&& let Some(recv_ty_defid) = recv_ty.ty_adt_def().map(AdtDef::did)
{
if let Some(diag_name) = cx.tcx.get_diagnostic_name(recv_ty_defid)
&& matches!(diag_name, sym::Option | sym::Result)
{
return true;
}

// FIXME: Add ControlFlow diagnostic item
let def_path = cx.get_def_path(recv_ty_defid);
if def_path
.iter()
.map(Symbol::as_str)
.zip(["core", "ops", "control_flow", "ControlFlow"])
.all(|(sym, s)| sym == s)
{
return true;
}
}
if is_trait_method(cx, expr, sym::Iterator) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to lint for any methods of Iterator? What would the use case for this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only activates for the recognized methods (map_*), no other methods of Iterator will trigger that.

return true;
}
false
}
45 changes: 45 additions & 0 deletions tests/ui/useless_conversion.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// FIXME(static_mut_refs): Do not allow `static_mut_refs` lint
#![allow(static_mut_refs)]

use std::ops::ControlFlow;

fn test_generic<T: Copy>(val: T) -> T {
let _ = val;
val
Expand Down Expand Up @@ -297,3 +299,46 @@ impl From<Foo<'a'>> for Foo<'b'> {
Foo
}
}

fn direct_application() {
let _: Result<(), std::io::Error> = test_issue_3913();
//~^ useless_conversion
let _: Result<(), std::io::Error> = test_issue_3913();
//~^ useless_conversion
let _: Result<(), std::io::Error> = test_issue_3913();
//~^ useless_conversion
let _: Result<(), std::io::Error> = test_issue_3913();
//~^ useless_conversion

let c: ControlFlow<()> = ControlFlow::Continue(());
let _: ControlFlow<()> = c;
//~^ useless_conversion
let c: ControlFlow<()> = ControlFlow::Continue(());
let _: ControlFlow<()> = c;
//~^ useless_conversion

struct Absorb;
impl From<()> for Absorb {
fn from(_: ()) -> Self {
Self
}
}
impl From<std::io::Error> for Absorb {
fn from(_: std::io::Error) -> Self {
Self
}
}
let _: Vec<u32> = [1u32].into_iter().collect();
//~^ useless_conversion

// No lint for those
let _: Result<Absorb, std::io::Error> = test_issue_3913().map(Into::into);
let _: Result<(), Absorb> = test_issue_3913().map_err(Into::into);
let _: Result<Absorb, std::io::Error> = test_issue_3913().map(From::from);
let _: Result<(), Absorb> = test_issue_3913().map_err(From::from);
}

fn gen_identity<T>(x: [T; 3]) -> Vec<T> {
x.into_iter().collect()
//~^ useless_conversion
}
45 changes: 45 additions & 0 deletions tests/ui/useless_conversion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
// FIXME(static_mut_refs): Do not allow `static_mut_refs` lint
#![allow(static_mut_refs)]

use std::ops::ControlFlow;

fn test_generic<T: Copy>(val: T) -> T {
let _ = T::from(val);
val.into()
Expand Down Expand Up @@ -297,3 +299,46 @@ impl From<Foo<'a'>> for Foo<'b'> {
Foo
}
}

fn direct_application() {
let _: Result<(), std::io::Error> = test_issue_3913().map(Into::into);
//~^ useless_conversion
let _: Result<(), std::io::Error> = test_issue_3913().map_err(Into::into);
//~^ useless_conversion
let _: Result<(), std::io::Error> = test_issue_3913().map(From::from);
//~^ useless_conversion
let _: Result<(), std::io::Error> = test_issue_3913().map_err(From::from);
//~^ useless_conversion

let c: ControlFlow<()> = ControlFlow::Continue(());
let _: ControlFlow<()> = c.map_break(Into::into);
//~^ useless_conversion
let c: ControlFlow<()> = ControlFlow::Continue(());
let _: ControlFlow<()> = c.map_continue(Into::into);
//~^ useless_conversion

struct Absorb;
impl From<()> for Absorb {
fn from(_: ()) -> Self {
Self
}
}
impl From<std::io::Error> for Absorb {
fn from(_: std::io::Error) -> Self {
Self
}
}
let _: Vec<u32> = [1u32].into_iter().map(Into::into).collect();
//~^ useless_conversion

// No lint for those
let _: Result<Absorb, std::io::Error> = test_issue_3913().map(Into::into);
let _: Result<(), Absorb> = test_issue_3913().map_err(Into::into);
let _: Result<Absorb, std::io::Error> = test_issue_3913().map(From::from);
let _: Result<(), Absorb> = test_issue_3913().map_err(From::from);
}

fn gen_identity<T>(x: [T; 3]) -> Vec<T> {
x.into_iter().map(Into::into).collect()
//~^ useless_conversion
}
Loading