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
J-ZhengLi committed Jul 1, 2024
1 parent 06758d8 commit b386427
Show file tree
Hide file tree
Showing 4 changed files with 297 additions and 16 deletions.
89 changes: 79 additions & 10 deletions clippy_lints/src/matches/manual_utils.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ 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_span::{sym, SyntaxContext};

#[expect(clippy::too_many_arguments)]
Expand Down Expand Up @@ -65,15 +66,7 @@ where

let some_expr = get_some_expr_fn(cx, some_pat, some_expr, expr_ctxt)?;

// These two lints will go back and forth with each other.
if cx.typeck_results().expr_ty(some_expr.expr) == cx.tcx.types.unit
&& !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, expr.hir_id)
{
return None;
}

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

Expand Down Expand Up @@ -274,3 +267,79 @@ 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.
.filter(|adj| !matches!(adj.kind, Adjust::NeverToAny))
.next()
.is_some()
}

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, _) => {
args.iter().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))
},
// 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.map(|e| expr_has_type_coercion(cx, e)).unwrap_or_default()
}
ExprKind::Match(_, arms, _) => {
arms.iter().map(|arm| arm.body).any(|body| expr_has_type_coercion(cx, body))
}
_ => false
}
}

fn should_lint<'tcx>(cx: &LateContext<'tcx>, match_expr: &Expr<'tcx>, some_expr: &Expr<'tcx>) -> bool {
let typeck = cx.typeck_results();
let some_expr_ty = typeck.expr_ty(some_expr);

// These two lints will go back and forth with each other.
if some_expr_ty.is_unit() && !is_lint_allowed(cx, OPTION_MAP_UNIT_FN, match_expr.hir_id) {
return false;
}

// `map` won't perform any adjustments.
// However, if the ty of `Some` arg is unit, type coercions should be fine.
if !some_expr_ty.is_unit()
// TODO: this needs to be tested
&& !some_expr_ty.is_scalar()
&& expr_has_type_coercion(cx, match_expr)
{
return false;
}

true
}
96 changes: 95 additions & 1 deletion tests/ui/manual_map_option_2.fixed
View file
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,92 @@ 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,
});

// FIXME: This is a known FN case.
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
}
}
99 changes: 98 additions & 1 deletion tests/ui/manual_map_option_2.rs
View file
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,
});

// FIXME: This is a known FN case.
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
}
}
29 changes: 25 additions & 4 deletions tests/ui/manual_map_option_2.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
| _________________^
Expand All @@ -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) {
| _____________^
Expand All @@ -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) {
| _____________^
Expand All @@ -61,5 +61,26 @@ 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: aborting due to 6 previous errors

0 comments on commit b386427

Please sign in to comment.