Skip to content

Commit

Permalink
move expr_requires_coercion to clippy_utils & some other adjustments
Browse files Browse the repository at this point in the history
  • Loading branch information
J-ZhengLi committed Sep 13, 2024
1 parent 635c375 commit 18d1a59
Show file tree
Hide file tree
Showing 5 changed files with 112 additions and 89 deletions.
78 changes: 4 additions & 74 deletions clippy_lints/src/matches/manual_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,15 @@ 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, 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,
can_move_expr_to_closure, expr_requires_coercion, 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::{self as hir, BindingMode, Expr, ExprKind, HirId, Mutability, Pat, PatKind, Path, QPath};
use rustc_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 @@ -75,7 +73,7 @@ where
}

// `map` won't perform any adjustments.
if expr_has_type_coercion(cx, expr) {
if expr_requires_coercion(cx, expr) {
return None;
}

Expand Down Expand Up @@ -274,71 +272,3 @@ 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
}
}
89 changes: 88 additions & 1 deletion clippy_utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ use rustc_middle::ty::fast_reject::SimplifiedType;
use rustc_middle::ty::layout::IntegerExt;
use rustc_middle::ty::{
self as rustc_ty, Binder, BorrowKind, ClosureKind, EarlyBinder, FloatTy, GenericArgsRef, IntTy, ParamEnv,
ParamEnvAnd, Ty, TyCtxt, TypeVisitableExt, UintTy, UpvarCapture,
ParamEnvAnd, Ty, TyCtxt, TypeFlags, TypeVisitableExt, UintTy, UpvarCapture,
};
use rustc_span::hygiene::{ExpnKind, MacroKind};
use rustc_span::source_map::SourceMap;
Expand Down Expand Up @@ -3492,3 +3492,90 @@ pub fn is_receiver_of_method_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool
}
false
}

/// Returns true if the specified `expr` requires coercion,
/// meaning that it either has a coercion or propagates a coercion from one of its sub expressions.
///
/// Similar to [`is_adjusted`], this not only checks if an expression's type was adjusted,
/// but also going through extra steps to see if it fits the description of [coercion sites].
///
/// You should used this when you want to avoid suggesting replacing an expression that is currently
/// a coercion site or coercion propagating expression with one that is not.
///
/// [coercion sites]: https://doc.rust-lang.org/stable/reference/type-coercions.html#coercion-sites
pub fn expr_requires_coercion<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) -> bool {
let expr_ty_is_adjusted = cx
.typeck_results()
.expr_adjustments(expr)
.iter()
// ignore `NeverToAny` adjustments, such as `panic!` call.
.any(|adj| !matches!(adj.kind, Adjust::NeverToAny));
if expr_ty_is_adjusted {
return true;
}

// Identify coercion sites and recursively check if those sites
// actually have type adjustments.
match expr.kind {
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 self_arg_count = usize::from(matches!(expr.kind, ExprKind::MethodCall(..)));
let mut args_with_ty_param = {
fn_sig
.inputs()
.skip_binder()
.iter()
.skip(self_arg_count)
.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_requires_coercion(cx, arg))
},
// Struct/union initialization.
ExprKind::Struct(qpath, _, _) => {
let res = cx.typeck_results().qpath_res(qpath, expr.hir_id);
if let Some((_, v_def)) = adt_and_variant_of_res(cx, res) {
let generic_args = cx.typeck_results().node_args(expr.hir_id);
v_def
.fields
.iter()
.any(|field| field.ty(cx.tcx, generic_args).has_type_flags(TypeFlags::HAS_TY_PARAM))
} else {
false
}
},
// Function results, including the final line of a block or a `return` expression.
ExprKind::Block(
&Block {
expr: Some(ret_expr), ..
},
_,
)
| ExprKind::Ret(Some(ret_expr)) => expr_requires_coercion(cx, ret_expr),

// ===== Coercion-propagation expressions =====
ExprKind::Array(elems) | ExprKind::Tup(elems) => elems.iter().any(|elem| expr_requires_coercion(cx, elem)),
// Array but with repeating syntax.
ExprKind::Repeat(rep_elem, _) => expr_requires_coercion(cx, rep_elem),
// Others that may contain coercion sites.
ExprKind::If(_, then, maybe_else) => {
expr_requires_coercion(cx, then) || maybe_else.is_some_and(|e| expr_requires_coercion(cx, e))
},
ExprKind::Match(_, arms, _) => arms
.iter()
.map(|arm| arm.body)
.any(|body| expr_requires_coercion(cx, body)),
_ => false,
}
}
11 changes: 10 additions & 1 deletion tests/ui/manual_map_option.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,16 @@ fn main() {
}

// #6811
Some(0).map(|x| vec![x]);
match Some(0) {
Some(x) => Some(vec![x]),
None => None,
};

// Don't lint, coercion
let x: Option<Vec<&[u8]>> = match Some(()) {
Some(_) => Some(vec![b"1234"]),
None => None,
};

option_env!("").map(String::from);

Expand Down
6 changes: 6 additions & 0 deletions tests/ui/manual_map_option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ fn main() {
None => None,
};

// Don't lint, coercion
let x: Option<Vec<&[u8]>> = match Some(()) {
Some(_) => Some(vec![b"1234"]),
None => None,
};

match option_env!("") {
Some(x) => Some(String::from(x)),
None => None,
Expand Down
17 changes: 4 additions & 13 deletions tests/ui/manual_map_option.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -156,16 +156,7 @@ LL | | };
| |_____^ help: try: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`

error: manual implementation of `Option::map`
--> tests/ui/manual_map_option.rs:168:5
|
LL | / match Some(0) {
LL | | Some(x) => Some(vec![x]),
LL | | None => None,
LL | | };
| |_____^ help: try: `Some(0).map(|x| vec![x])`

error: manual implementation of `Option::map`
--> tests/ui/manual_map_option.rs:173:5
--> tests/ui/manual_map_option.rs:179:5
|
LL | / match option_env!("") {
LL | | Some(x) => Some(String::from(x)),
Expand All @@ -174,7 +165,7 @@ LL | | };
| |_____^ help: try: `option_env!("").map(String::from)`

error: manual implementation of `Option::map`
--> tests/ui/manual_map_option.rs:193:12
--> tests/ui/manual_map_option.rs:199:12
|
LL | } else if let Some(x) = Some(0) {
| ____________^
Expand All @@ -185,7 +176,7 @@ LL | | };
| |_____^ help: try: `{ Some(0).map(|x| x + 1) }`

error: manual implementation of `Option::map`
--> tests/ui/manual_map_option.rs:201:12
--> tests/ui/manual_map_option.rs:207:12
|
LL | } else if let Some(x) = Some(0) {
| ____________^
Expand All @@ -195,5 +186,5 @@ LL | | None
LL | | };
| |_____^ help: try: `{ Some(0).map(|x| x + 1) }`

error: aborting due to 21 previous errors
error: aborting due to 20 previous errors

0 comments on commit 18d1a59

Please sign in to comment.