user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix [manual_map] not catching type adjustment

Browse source
This commit is contained in:
J-ZhengLi 2024-07-02 16:09:13 +08:00 • committed by J-ZhengLi
parent 75e3a2e905
commit a2f9861df8
4 changed files with 302 additions and 17 deletions
Download patch file Download diff file Expand all files Collapse all files

88
clippy_lints/src/matches/manual_utils.rs
View file

@ -73,7 +73,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;
}
@ -124,6 +124,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)
@ -145,20 +151,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;
@ -274,3 +272,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
tests/ui/manual_map_option_2.fixed
View file

@ -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 {
@ -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
tests/ui/manual_map_option_2.rs
View file

@ -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 {
@ -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
}
}

39
tests/ui/manual_map_option_2.stderr
View file

@ -32,7 +32,7 @@ LL | | };
| |_____^ help: try: `s.as_ref().map(|x| { if let Some(ref s) = s { (x.clone(), s) } else { panic!() } })`
error: manual implementation of `Option::map`
--> tests/ui/manual_map_option_2.rs:58:17
--> tests/ui/manual_map_option_2.rs:63:17
|
LL | let _ = match Some(0) {
| _________________^
@ -42,7 +42,7 @@ LL | | };
| |_________^ help: try: `Some(0).map(|x| f(x))`
error: manual implementation of `Option::map`
--> tests/ui/manual_map_option_2.rs:63:13
--> tests/ui/manual_map_option_2.rs:68:13
|
LL | let _ = match Some(0) {
| _____________^
@ -52,7 +52,7 @@ LL | | };
| |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })`
error: manual implementation of `Option::map`
--> tests/ui/manual_map_option_2.rs:67:13
--> tests/ui/manual_map_option_2.rs:72:13
|
LL | let _ = match Some(0) {
| _____________^
@ -61,5 +61,36 @@ LL | | None => None,
LL | | };
| |_____^ help: try: `Some(0).map(|x| unsafe { f(x) })`
error: aborting due to 5 previous errors
error: manual implementation of `Option::map`
--> tests/ui/manual_map_option_2.rs:109:17
|
LL | let _ = match Some(0) {
| _________________^
LL | | Some(_) => Some(match f() {
LL | | Ok(res) => Ok(Box::new(res)),
LL | | _ => Err(()),
LL | | }),
LL | | None => None,
LL | | };
| |_________^
|
help: try
|
LL ~ let _ = Some(0).map(|_| match f() {
LL + Ok(res) => Ok(Box::new(res)),
LL + _ => Err(()),
LL ~ });
|
error: manual implementation of `Option::map`
--> tests/ui/manual_map_option_2.rs:132:37
|
LL | let _: Option<Box<&[u8]>> = match Some(0) {
| _____________________________________^
LL | | Some(_) => Some(g(x)),
LL | | None => None,
LL | | };
| |_________^ help: try: `Some(0).map(|_| g(x))`
error: aborting due to 7 previous errors