OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Add: option_manual_map lint

Browse Source
This commit is contained in:
Jason Newcomb 2021-01-10 09:46:03 -05:00
parent 728f3976f0
commit efe33f9fe4
No known key found for this signature in database
GPG Key ID: DA59E8643A37ED06
7 changed files with 626 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -2161,6 +2161,7 @@ Released 2018-09-13
[`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
[`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map
[`manual_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten
[`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map
[`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
[`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
[`manual_ok_or`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_ok_or

5
clippy_lints/src/lib.rs
View File

@ -241,6 +241,7 @@ mod loops;
mod macro_use;
mod main_recursion;
mod manual_async_fn;
mod manual_map;
mod manual_non_exhaustive;
mod manual_ok_or;
mod manual_strip;
@ -706,6 +707,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
&macro_use::MACRO_USE_IMPORTS,
&main_recursion::MAIN_RECURSION,
&manual_async_fn::MANUAL_ASYNC_FN,
&manual_map::MANUAL_MAP,
&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
&manual_ok_or::MANUAL_OK_OR,
&manual_strip::MANUAL_STRIP,
@ -1262,6 +1264,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|| box case_sensitive_file_extension_comparisons::CaseSensitiveFileExtensionComparisons);
store.register_late_pass(|| box redundant_slicing::RedundantSlicing);
store.register_late_pass(|| box from_str_radix_10::FromStrRadix10);
store.register_late_pass(|| box manual_map::ManualMap);
store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
@ -1521,6 +1524,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&loops::WHILE_LET_ON_ITERATOR),
LintId::of(&main_recursion::MAIN_RECURSION),
LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
LintId::of(&manual_map::MANUAL_MAP),
LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
LintId::of(&manual_strip::MANUAL_STRIP),
LintId::of(&manual_unwrap_or::MANUAL_UNWRAP_OR),
@ -1750,6 +1754,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(&loops::WHILE_LET_ON_ITERATOR),
LintId::of(&main_recursion::MAIN_RECURSION),
LintId::of(&manual_async_fn::MANUAL_ASYNC_FN),
LintId::of(&manual_map::MANUAL_MAP),
LintId::of(&manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE),
LintId::of(&map_clone::MAP_CLONE),
LintId::of(&matches::INFALLIBLE_DESTRUCTURING_MATCH),

262
clippy_lints/src/manual_map.rs Normal file
View File

@ -0,0 +1,262 @@
use crate::utils::{
is_type_diagnostic_item, match_def_path, paths, peel_hir_expr_refs, peel_mid_ty_refs_is_mutable,
snippet_with_applicability, span_lint_and_sugg,
};
use rustc_ast::util::parser::PREC_POSTFIX;
use rustc_errors::Applicability;
use rustc_hir::{Arm, BindingAnnotation, Block, Expr, ExprKind, Mutability, Pat, PatKind, Path, QPath};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::symbol::{sym, Ident};
declare_clippy_lint! {
/// **What it does:** Checks for usages of `match` which could be implemented using `map`
///
/// **Why is this bad?** Using the `map` method is clearer and more concise.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust
/// match Some(0) {
/// Some(x) => Some(x + 1),
/// None => None,
/// };
/// ```
/// Use instead:
/// ```rust
/// Some(0).map(|x| x + 1);
/// ```
pub MANUAL_MAP,
style,
"reimplementation of `map`"
}
declare_lint_pass!(ManualMap => [MANUAL_MAP]);
impl LateLintPass<'_> for ManualMap {
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
if in_external_macro(cx.sess(), expr.span) {
return;
}
if let ExprKind::Match(scrutinee, [arm1 @ Arm { guard: None, .. }, arm2 @ Arm { guard: None, .. }], _) =
expr.kind
{
let (scrutinee_ty, ty_ref_count, ty_mutability) =
peel_mid_ty_refs_is_mutable(cx.typeck_results().expr_ty(scrutinee));
if !is_type_diagnostic_item(cx, scrutinee_ty, sym::option_type)
|| !is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(expr), sym::option_type)
{
return;
}
let (some_expr, some_pat, pat_ref_count, is_wild_none) =
match (try_parse_pattern(cx, arm1.pat), try_parse_pattern(cx, arm2.pat)) {
(Some(OptionPat::Wild), Some(OptionPat::Some { pattern, ref_count }))
if is_none_expr(cx, arm1.body) =>
{
(arm2.body, pattern, ref_count, true)
},
(Some(OptionPat::None), Some(OptionPat::Some { pattern, ref_count }))
if is_none_expr(cx, arm1.body) =>
{
(arm2.body, pattern, ref_count, false)
},
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::Wild))
if is_none_expr(cx, arm2.body) =>
{
(arm1.body, pattern, ref_count, true)
},
(Some(OptionPat::Some { pattern, ref_count }), Some(OptionPat::None))
if is_none_expr(cx, arm2.body) =>
{
(arm1.body, pattern, ref_count, false)
},
_ => return,
};
// Top level or patterns aren't allowed in closures.
if matches!(some_pat.kind, PatKind::Or(_)) {
return;
}
let some_expr = match get_some_expr(cx, some_expr) {
Some(expr) => expr,
None => return,
};
// Determine which binding mode to use.
let explicit_ref = some_pat.contains_explicit_ref_binding();
let binding_mutability = explicit_ref.or(if ty_ref_count != pat_ref_count {
Some(ty_mutability)
} else {
None
});
let as_ref_str = match binding_mutability {
Some(Mutability::Mut) => ".as_mut()",
Some(Mutability::Not) => ".as_ref()",
None => "",
};
let mut app = Applicability::MachineApplicable;
// Remove address-of expressions from the scrutinee. `as_ref` will be called,
// the type is copyable, or the option is being passed by value.
let scrutinee = peel_hir_expr_refs(scrutinee).0;
let scrutinee_str = snippet_with_applicability(cx, scrutinee.span, "_", &mut app);
let scrutinee_str = if expr.precedence().order() < PREC_POSTFIX {
// Parens are needed to chain method calls.
format!("({})", scrutinee_str)
} else {
scrutinee_str.into()
};
let body_str = if let PatKind::Binding(annotation, _, some_binding, None) = some_pat.kind {
if let Some(func) = can_pass_as_func(cx, some_binding, some_expr) {
snippet_with_applicability(cx, func.span, "..", &mut app).into_owned()
} else {
// `ref` and `ref mut` annotations were handled earlier.
let annotation = if matches!(annotation, BindingAnnotation::Mutable) {
"mut "
} else {
""
};
format!(
"|{}{}| {}",
annotation,
some_binding,
snippet_with_applicability(cx, some_expr.span, "..", &mut app)
)
}
} else if !is_wild_none && explicit_ref.is_none() {
// TODO: handle explicit reference annotations.
format!(
"|{}| {}",
snippet_with_applicability(cx, some_pat.span, "..", &mut app),
snippet_with_applicability(cx, some_expr.span, "..", &mut app)
)
} else {
// Refutable bindings and mixed reference annotations can't be handled by `map`.
return;
};
span_lint_and_sugg(
cx,
MANUAL_MAP,
expr.span,
"manual implementation of `Option::map`",
"try this",
format!("{}{}.map({})", scrutinee_str, as_ref_str, body_str),
app,
);
}
}
}
// Checks whether the expression could be passed as a function, or whether a closure is needed.
// Returns the function to be passed to `map` if it exists.
fn can_pass_as_func(cx: &LateContext<'tcx>, binding: Ident, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
match expr.kind {
ExprKind::Call(func, [arg])
if matches!(arg.kind,
ExprKind::Path(QPath::Resolved(None, Path { segments: [path], ..}))
if path.ident == binding
) && cx.typeck_results().expr_adjustments(arg).is_empty() =>
{
Some(func)
},
_ => None,
}
}
enum OptionPat<'a> {
Wild,
None,
Some {
// The pattern contained in the `Some` tuple.
pattern: &'a Pat<'a>,
// The number of references before the `Some` tuple.
// e.g. `&&Some(_)` has a ref count of 2.
ref_count: usize,
},
}
// Try to parse into a recognized `Option` pattern.
// i.e. `_`, `None`, `Some(..)`, or a reference to any of those.
fn try_parse_pattern(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) -> Option<OptionPat<'tcx>> {
fn f(cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, ref_count: usize) -> Option<OptionPat<'tcx>> {
match pat.kind {
PatKind::Wild => Some(OptionPat::Wild),
PatKind::Ref(pat, _) => f(cx, pat, ref_count + 1),
PatKind::Path(QPath::Resolved(None, path))
if path
.res
.opt_def_id()
.map_or(false, |id| match_def_path(cx, id, &paths::OPTION_NONE)) =>
{
Some(OptionPat::None)
},
PatKind::TupleStruct(QPath::Resolved(None, path), [pattern], _)
if path
.res
.opt_def_id()
.map_or(false, |id| match_def_path(cx, id, &paths::OPTION_SOME)) =>
{
Some(OptionPat::Some { pattern, ref_count })
},
_ => None,
}
}
f(cx, pat, 0)
}
// Checks for an expression wrapped by the `Some` constructor. Returns the contained expression.
fn get_some_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
// TODO: Allow more complex expressions.
match expr.kind {
ExprKind::Call(
Expr {
kind: ExprKind::Path(QPath::Resolved(None, path)),
..
},
[arg],
) => {
if match_def_path(cx, path.res.opt_def_id()?, &paths::OPTION_SOME) {
Some(arg)
} else {
None
}
},
ExprKind::Block(
Block {
stmts: [],
expr: Some(expr),
..
},
_,
) => get_some_expr(cx, expr),
_ => None,
}
}
// Checks for the `None` value.
fn is_none_expr(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> bool {
match expr.kind {
ExprKind::Path(QPath::Resolved(None, path)) => path
.res
.opt_def_id()
.map_or(false, |id| match_def_path(cx, id, &paths::OPTION_NONE)),
ExprKind::Block(
Block {
stmts: [],
expr: Some(expr),
..
},
_,
) => is_none_expr(cx, expr),
_ => false,
}
}

27
clippy_lints/src/utils/mod.rs
View File

@ -32,7 +32,7 @@ use std::collections::hash_map::Entry;
use std::hash::BuildHasherDefault;
use if_chain::if_chain;
use rustc_ast::ast::{self, Attribute, LitKind};
use rustc_ast::ast::{self, Attribute, BorrowKind, LitKind, Mutability};
use rustc_data_structures::fx::FxHashMap;
use rustc_errors::Applicability;
use rustc_hir as hir;
@ -1672,6 +1672,18 @@ pub fn peel_n_hir_expr_refs(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>,
f(expr, 0, count)
}
/// Peels off all references on the expression. Returns the underlying expression and the number of
/// references removed.
pub fn peel_hir_expr_refs(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) {
fn f(expr: &'a Expr<'a>, count: usize) -> (&'a Expr<'a>, usize) {
match expr.kind {
ExprKind::AddrOf(BorrowKind::Ref, _, expr) => f(expr, count + 1),
_ => (expr, count),
}
}
f(expr, 0)
}
/// Peels off all references on the type. Returns the underlying type and the number of references
/// removed.
pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) {
@ -1685,6 +1697,19 @@ pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) {
peel(ty, 0)
}
/// Peels off all references on the type.Returns the underlying type, the number of references
/// removed, and whether the pointer is ultimately mutable or not.
pub fn peel_mid_ty_refs_is_mutable(ty: Ty<'_>) -> (Ty<'_>, usize, Mutability) {
fn f(ty: Ty<'_>, count: usize, mutability: Mutability) -> (Ty<'_>, usize, Mutability) {
match ty.kind() {
ty::Ref(_, ty, Mutability::Mut) => f(ty, count + 1, mutability),
ty::Ref(_, ty, Mutability::Not) => f(ty, count + 1, Mutability::Not),
_ => (ty, count, mutability),
}
}
f(ty, 0, Mutability::Mut)
}
#[macro_export]
macro_rules! unwrap_cargo_metadata {
($cx: ident, $lint: ident, $deps: expr) => {{

61
tests/ui/manual_map_option.fixed Normal file
View File

@ -0,0 +1,61 @@
// run-rustfix
#![warn(clippy::manual_map)]
#![allow(clippy::no_effect, clippy::map_identity, clippy::unit_arg, clippy::match_ref_pats)]
fn main() {
Some(0).map(|_| 2);
Some(0).map(|x| x + 1);
Some("").map(|x| x.is_empty());
Some(0).map(|x| !x);
#[rustfmt::skip]
Some(0).map(std::convert::identity);
Some(&String::new()).map(|x| str::len(x));
match Some(0) {
Some(x) if false => Some(x + 1),
_ => None,
};
Some([0, 1]).as_ref().map(|x| x[0]);
Some(0).map(|x| x * 2);
Some(String::new()).as_ref().map(|x| x.is_empty());
Some(String::new()).as_ref().map(|x| x.len());
Some(0).map(|x| x + x);
Some(String::new()).as_mut().map(|x| x.push_str(""));
Some(String::new()).as_ref().map(|x| &**x);
Some(String::new()).as_ref().map(|x| x.is_empty());
Some((0, 1, 2)).map(|(x, y, z)| x + y + z);
Some([1, 2, 3]).map(|[first, ..]| first);
Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x));
match Some((String::new(), 0)) {
Some((ref x, y)) => Some((y, x)),
None => None,
};
match Some(Some(0)) {
Some(Some(_)) | Some(None) => Some(0),
None => None,
};
match Some(Some((0, 1))) {
Some(Some((x, 1))) => Some(x),
_ => None,
};
}

113
tests/ui/manual_map_option.rs Normal file
View File

@ -0,0 +1,113 @@
// run-rustfix
#![warn(clippy::manual_map)]
#![allow(clippy::no_effect, clippy::map_identity, clippy::unit_arg, clippy::match_ref_pats)]
fn main() {
match Some(0) {
Some(_) => Some(2),
None::<u32> => None,
};
match Some(0) {
Some(x) => Some(x + 1),
_ => None,
};
match Some("") {
Some(x) => Some(x.is_empty()),
None => None,
};
if let Some(x) = Some(0) {
Some(!x)
} else {
None
};
#[rustfmt::skip]
match Some(0) {
Some(x) => { Some(std::convert::identity(x)) }
None => { None }
};
match Some(&String::new()) {
Some(x) => Some(str::len(x)),
None => None,
};
match Some(0) {
Some(x) if false => Some(x + 1),
_ => None,
};
match &Some([0, 1]) {
Some(x) => Some(x[0]),
&None => None,
};
match &Some(0) {
&Some(x) => Some(x * 2),
None => None,
};
match Some(String::new()) {
Some(ref x) => Some(x.is_empty()),
_ => None,
};
match &&Some(String::new()) {
Some(x) => Some(x.len()),
_ => None,
};
match &&Some(0) {
&&Some(x) => Some(x + x),
&&_ => None,
};
match &mut Some(String::new()) {
Some(x) => Some(x.push_str("")),
None => None,
};
match &mut Some(String::new()) {
Some(ref x) => Some(&**x),
None => None,
};
match &mut &Some(String::new()) {
Some(x) => Some(x.is_empty()),
&mut _ => None,
};
match Some((0, 1, 2)) {
Some((x, y, z)) => Some(x + y + z),
None => None,
};
match Some([1, 2, 3]) {
Some([first, ..]) => Some(first),
None => None,
};
match &Some((String::new(), "test")) {
Some((x, y)) => Some((y, x)),
None => None,
};
match Some((String::new(), 0)) {
Some((ref x, y)) => Some((y, x)),
None => None,
};
match Some(Some(0)) {
Some(Some(_)) | Some(None) => Some(0),
None => None,
};
match Some(Some((0, 1))) {
Some(Some((x, 1))) => Some(x),
_ => None,
};
}

158
tests/ui/manual_map_option.stderr Normal file
View File

@ -0,0 +1,158 @@
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:7:5
|
LL | / match Some(0) {
LL | | Some(_) => Some(2),
LL | | None::<u32> => None,
LL | | };
| |_____^ help: try this: `Some(0).map(|_| 2)`
|
= note: `-D clippy::manual-map` implied by `-D warnings`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:12:5
|
LL | / match Some(0) {
LL | | Some(x) => Some(x + 1),
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(0).map(|x| x + 1)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:17:5
|
LL | / match Some("") {
LL | | Some(x) => Some(x.is_empty()),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some("").map(|x| x.is_empty())`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:22:5
|
LL | / if let Some(x) = Some(0) {
LL | | Some(!x)
LL | | } else {
LL | | None
LL | | };
| |_____^ help: try this: `Some(0).map(|x| !x)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:29:5
|
LL | / match Some(0) {
LL | | Some(x) => { Some(std::convert::identity(x)) }
LL | | None => { None }
LL | | };
| |_____^ help: try this: `Some(0).map(std::convert::identity)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:34:5
|
LL | / match Some(&String::new()) {
LL | | Some(x) => Some(str::len(x)),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(&String::new()).map(|x| str::len(x))`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:44:5
|
LL | / match &Some([0, 1]) {
LL | | Some(x) => Some(x[0]),
LL | | &None => None,
LL | | };
| |_____^ help: try this: `Some([0, 1]).as_ref().map(|x| x[0])`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:49:5
|
LL | / match &Some(0) {
LL | | &Some(x) => Some(x * 2),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(0).map(|x| x * 2)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:54:5
|
LL | / match Some(String::new()) {
LL | | Some(ref x) => Some(x.is_empty()),
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:59:5
|
LL | / match &&Some(String::new()) {
LL | | Some(x) => Some(x.len()),
LL | | _ => None,
LL | | };
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.len())`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:64:5
|
LL | / match &&Some(0) {
LL | | &&Some(x) => Some(x + x),
LL | | &&_ => None,
LL | | };
| |_____^ help: try this: `Some(0).map(|x| x + x)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:69:5
|
LL | / match &mut Some(String::new()) {
LL | | Some(x) => Some(x.push_str("")),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(String::new()).as_mut().map(|x| x.push_str(""))`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:74:5
|
LL | / match &mut Some(String::new()) {
LL | | Some(ref x) => Some(&**x),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| &**x)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:79:5
|
LL | / match &mut &Some(String::new()) {
LL | | Some(x) => Some(x.is_empty()),
LL | | &mut _ => None,
LL | | };
| |_____^ help: try this: `Some(String::new()).as_ref().map(|x| x.is_empty())`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:84:5
|
LL | / match Some((0, 1, 2)) {
LL | | Some((x, y, z)) => Some(x + y + z),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some((0, 1, 2)).map(|(x, y, z)| x + y + z)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:89:5
|
LL | / match Some([1, 2, 3]) {
LL | | Some([first, ..]) => Some(first),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some([1, 2, 3]).map(|[first, ..]| first)`
error: manual implementation of `Option::map`
--> $DIR/manual_map_option.rs:94:5
|
LL | / match &Some((String::new(), "test")) {
LL | | Some((x, y)) => Some((y, x)),
LL | | None => None,
LL | | };
| |_____^ help: try this: `Some((String::new(), "test")).as_ref().map(|(x, y)| (y, x))`
error: aborting due to 17 previous errors