Skip to content

Commit

Permalink
Add: option_manual_map lint
Browse files Browse the repository at this point in the history
  • Loading branch information
Jarcho committed Feb 19, 2021
1 parent 877be18 commit 1c0ba16
Show file tree
Hide file tree
Showing 7 changed files with 626 additions and 1 deletion.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2160,6 +2160,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
Expand Down
5 changes: 5 additions & 0 deletions clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,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;
Expand Down Expand Up @@ -704,6 +705,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,
Expand Down Expand Up @@ -1259,6 +1261,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(move || box types::PtrAsPtr::new(msrv));
store.register_late_pass(|| box case_sensitive_file_extension_comparisons::CaseSensitiveFileExtensionComparisons);
store.register_late_pass(|| box redundant_slicing::RedundantSlicing);
store.register_late_pass(|| box manual_map::ManualMap);

store.register_group(true, "clippy::restriction", Some("clippy_restriction"), vec![
LintId::of(&arithmetic::FLOAT_ARITHMETIC),
Expand Down Expand Up @@ -1516,6 +1519,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),
Expand Down Expand Up @@ -1745,6 +1749,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),
Expand Down
262 changes: 262 additions & 0 deletions clippy_lints/src/manual_map.rs
Original file line number Diff line number Diff line change
@@ -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 changes: 26 additions & 1 deletion clippy_lints/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -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) => {{
Expand Down
Loading

0 comments on commit 1c0ba16

Please sign in to comment.