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

make [manual_map] ignore types that contain dyn #12712

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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
94 changes: 81 additions & 13 deletions clippy_lints/src/matches/manual_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,17 @@ use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{is_copy, is_type_diagnostic_item, peel_mid_ty_refs_is_mutable, type_is_unsafe_function};
use clippy_utils::{
can_move_expr_to_closure, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res, path_to_local_id,
can_move_expr_to_closure, fn_def_id, is_else_clause, is_lint_allowed, is_res_lang_ctor, path_res, path_to_local_id,
peel_blocks, peel_hir_expr_refs, peel_hir_expr_while, CaptureKind,
};
use rustc_ast::util::parser::PREC_UNAMBIGUOUS;
use rustc_errors::Applicability;
use rustc_hir::def::Res;
use rustc_hir::LangItem::{OptionNone, OptionSome};
use rustc_hir::{BindingMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath};
use rustc_hir::{self as hir, BindingMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath};
use rustc_lint::LateContext;
use rustc_middle::ty::adjustment::Adjust;
use rustc_middle::ty::{TypeFlags, TypeVisitableExt};
use rustc_span::{sym, SyntaxContext};

#[expect(clippy::too_many_arguments)]
Expand Down Expand Up @@ -73,7 +75,7 @@ where
}

// `map` won't perform any adjustments.
if !cx.typeck_results().expr_adjustments(some_expr.expr).is_empty() {
if expr_has_type_coercion(cx, expr) {
return None;
}

Expand Down Expand Up @@ -124,6 +126,12 @@ where
};

let closure_expr_snip = some_expr.to_snippet_with_context(cx, expr_ctxt, &mut app);
let closure_body = if some_expr.needs_unsafe_block {
format!("unsafe {}", closure_expr_snip.blockify())
} else {
closure_expr_snip.to_string()
};

let body_str = if let PatKind::Binding(annotation, id, some_binding, None) = some_pat.kind {
if !some_expr.needs_unsafe_block
&& let Some(func) = can_pass_as_func(cx, id, some_expr.expr)
Expand All @@ -145,20 +153,12 @@ where
""
};

if some_expr.needs_unsafe_block {
format!("|{annotation}{some_binding}| unsafe {{ {closure_expr_snip} }}")
} else {
format!("|{annotation}{some_binding}| {closure_expr_snip}")
}
format!("|{annotation}{some_binding}| {closure_body}")
}
} else if !is_wild_none && explicit_ref.is_none() {
// TODO: handle explicit reference annotations.
let pat_snip = snippet_with_context(cx, some_pat.span, expr_ctxt, "..", &mut app).0;
if some_expr.needs_unsafe_block {
format!("|{pat_snip}| unsafe {{ {closure_expr_snip} }}")
} else {
format!("|{pat_snip}| {closure_expr_snip}")
}
format!("|{pat_snip}| {closure_body}")
} else {
// Refutable bindings and mixed reference annotations can't be handled by `map`.
return None;
Expand Down Expand Up @@ -274,3 +274,71 @@ pub(super) fn try_parse_pattern<'tcx>(
fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
is_res_lang_ctor(cx, path_res(cx, peel_blocks(expr)), OptionNone)
}

fn expr_ty_adjusted(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
cx.typeck_results()
.expr_adjustments(expr)
.iter()
// We do not care about exprs with `NeverToAny` adjustments, such as `panic!` call.
.any(|adj| !matches!(adj.kind, Adjust::NeverToAny))
}

fn expr_has_type_coercion<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
if expr.span.from_expansion() {
return false;
}
if expr_ty_adjusted(cx, expr) {
return true;
}

// Identify coercion sites and recursively check it those sites
// actually has type adjustments.
match expr.kind {
// Function/method calls, including enum initialization.
ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) if let Some(def_id) = fn_def_id(cx, expr) => {
let fn_sig = cx.tcx.fn_sig(def_id).instantiate_identity();
if !fn_sig.output().skip_binder().has_type_flags(TypeFlags::HAS_TY_PARAM) {
return false;
}
let mut args_with_ty_param = fn_sig
.inputs()
.skip_binder()
.iter()
.zip(args)
.filter_map(|(arg_ty, arg)| if arg_ty.has_type_flags(TypeFlags::HAS_TY_PARAM) {
Some(arg)
} else {
None
});
args_with_ty_param.any(|arg| expr_has_type_coercion(cx, arg))
},
// Struct/union initialization.
ExprKind::Struct(_, fields, _) => {
fields.iter().map(|expr_field| expr_field.expr).any(|ex| expr_has_type_coercion(cx, ex))
},
// those two `ref` keywords cannot be removed
#[allow(clippy::needless_borrow)]
// Function results, including the final line of a block or a `return` expression.
ExprKind::Block(hir::Block { expr: Some(ref ret_expr), .. }, _) |
ExprKind::Ret(Some(ref ret_expr)) => expr_has_type_coercion(cx, ret_expr),

// ===== Coercion-propagation expressions =====

// Array, where the type is `[U; n]`.
ExprKind::Array(elems) |
// Tuple, `(U_0, U_1, ..., U_n)`.
ExprKind::Tup(elems) => {
elems.iter().any(|elem| expr_has_type_coercion(cx, elem))
},
// Array but with repeating syntax.
ExprKind::Repeat(rep_elem, _) => expr_has_type_coercion(cx, rep_elem),
// Others that may contain coercion sites.
ExprKind::If(_, then, maybe_else) => {
expr_has_type_coercion(cx, then) || maybe_else.is_some_and(|e| expr_has_type_coercion(cx, e))
}
ExprKind::Match(_, arms, _) => {
arms.iter().map(|arm| arm.body).any(|body| expr_has_type_coercion(cx, body))
}
_ => false
}
}
93 changes: 92 additions & 1 deletion tests/ui/manual_map_option_2.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,14 @@ fn main() {
None => None,
};

// Lint. `s` is captured by reference, so no lifetime issues.
let s = Some(String::new());
// Lint. `s` is captured by reference, so no lifetime issues.
let _ = s.as_ref().map(|x| { if let Some(ref s) = s { (x.clone(), s) } else { panic!() } });
// Don't lint this, type of `s` is coercioned from `&String` to `&str`
let x: Option<(String, &str)> = match &s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
};

// Issue #7820
unsafe fn f(x: u32) -> u32 {
Expand All @@ -54,3 +59,89 @@ fn main() {
let _ = Some(0).map(|x| unsafe { f(x) });
let _ = Some(0).map(|x| unsafe { f(x) });
}

// issue #12659
mod with_type_coercion {
trait DummyTrait {}

fn foo<T: DummyTrait, F: Fn() -> Result<T, ()>>(f: F) {
// Don't lint
let _: Option<Result<Box<dyn DummyTrait>, ()>> = match Some(0) {
Some(_) => Some(match f() {
Ok(res) => Ok(Box::new(res)),
_ => Err(()),
}),
None => None,
};

let _: Option<Box<&[u8]>> = match Some(()) {
Some(_) => Some(Box::new(b"1234")),
None => None,
};

let x = String::new();
let _: Option<Box<&str>> = match Some(()) {
Some(_) => Some(Box::new(&x)),
None => None,
};

let _: Option<&str> = match Some(()) {
Some(_) => Some(&x),
None => None,
};

//~v ERROR: manual implementation of `Option::map`
let _ = Some(0).map(|_| match f() {
Ok(res) => Ok(Box::new(res)),
_ => Err(()),
});
}

#[allow(clippy::redundant_allocation)]
fn bar() {
fn f(_: Option<Box<&[u8]>>) {}
fn g(b: &[u8]) -> Box<&[u8]> {
Box::new(b)
}

let x: &[u8; 4] = b"1234";
f(match Some(()) {
Some(_) => Some(Box::new(x)),
None => None,
});

//~v ERROR: manual implementation of `Option::map`
let _: Option<Box<&[u8]>> = Some(0).map(|_| g(x));
}

fn with_fn_ret(s: &Option<String>) -> Option<(String, &str)> {
// Don't lint, `map` doesn't work as the return type is adjusted.
match s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
}
}

fn with_fn_ret_2(s: &Option<String>) -> Option<(String, &str)> {
if true {
// Don't lint, `map` doesn't work as the return type is adjusted.
return match s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
};
}
None
}

#[allow(clippy::needless_late_init)]
fn with_fn_ret_3<'a>(s: &'a Option<String>) -> Option<(String, &'a str)> {
let x: Option<(String, &'a str)>;
x = {
match s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
}
};
x
}
}
99 changes: 98 additions & 1 deletion tests/ui/manual_map_option_2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,17 @@ fn main() {
None => None,
};

// Lint. `s` is captured by reference, so no lifetime issues.
let s = Some(String::new());
// Lint. `s` is captured by reference, so no lifetime issues.
let _ = match &s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
};
// Don't lint this, type of `s` is coercioned from `&String` to `&str`
let x: Option<(String, &str)> = match &s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
};

// Issue #7820
unsafe fn f(x: u32) -> u32 {
Expand All @@ -69,3 +74,95 @@ fn main() {
None => None,
};
}

// issue #12659
mod with_type_coercion {
trait DummyTrait {}

fn foo<T: DummyTrait, F: Fn() -> Result<T, ()>>(f: F) {
// Don't lint
let _: Option<Result<Box<dyn DummyTrait>, ()>> = match Some(0) {
Some(_) => Some(match f() {
Ok(res) => Ok(Box::new(res)),
_ => Err(()),
}),
None => None,
};
Alexendoo marked this conversation as resolved.
Show resolved Hide resolved

let _: Option<Box<&[u8]>> = match Some(()) {
Some(_) => Some(Box::new(b"1234")),
None => None,
};

let x = String::new();
let _: Option<Box<&str>> = match Some(()) {
Some(_) => Some(Box::new(&x)),
None => None,
};

let _: Option<&str> = match Some(()) {
Some(_) => Some(&x),
None => None,
};

//~v ERROR: manual implementation of `Option::map`
let _ = match Some(0) {
Some(_) => Some(match f() {
Ok(res) => Ok(Box::new(res)),
_ => Err(()),
}),
None => None,
};
}

#[allow(clippy::redundant_allocation)]
fn bar() {
fn f(_: Option<Box<&[u8]>>) {}
fn g(b: &[u8]) -> Box<&[u8]> {
Box::new(b)
}

let x: &[u8; 4] = b"1234";
f(match Some(()) {
Some(_) => Some(Box::new(x)),
None => None,
});

//~v ERROR: manual implementation of `Option::map`
let _: Option<Box<&[u8]>> = match Some(0) {
Some(_) => Some(g(x)),
None => None,
};
}

fn with_fn_ret(s: &Option<String>) -> Option<(String, &str)> {
// Don't lint, `map` doesn't work as the return type is adjusted.
match s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
}
}

fn with_fn_ret_2(s: &Option<String>) -> Option<(String, &str)> {
if true {
// Don't lint, `map` doesn't work as the return type is adjusted.
return match s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
};
}
None
}

#[allow(clippy::needless_late_init)]
fn with_fn_ret_3<'a>(s: &'a Option<String>) -> Option<(String, &'a str)> {
let x: Option<(String, &'a str)>;
x = {
match s {
Some(x) => Some({ if let Some(ref s) = s { (x.clone(), s) } else { panic!() } }),
None => None,
}
};
x
}
}
Loading