Skip to content

Commit

Permalink
fix [manual_map] not catching type adjustment
Browse files Browse the repository at this point in the history
  • Loading branch information
J-ZhengLi committed Jul 2, 2024
1 parent 06758d8 commit a5fcaea
Show file tree
Hide file tree
Showing 4 changed files with 306 additions and 19 deletions.
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,
};

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

0 comments on commit a5fcaea

Please sign in to comment.