Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 27 additions & 14 deletions clippy_lints/src/matches/manual_filter.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use clippy_utils::as_some_expr;
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
use clippy_utils::visitors::contains_unsafe_block;

Expand All @@ -9,7 +9,7 @@ use rustc_lint::LateContext;
use rustc_span::{SyntaxContext, sym};

use super::MANUAL_FILTER;
use super::manual_utils::{SomeExpr, check_with};
use super::manual_utils::{SomeExpr, SuggInfo, check_with};

// Function called on the <expr> of `[&+]Some((ref | ref mut) x) => <expr>`
// Need to check if it's of the form `<expr>=if <cond> {<then_expr>} else {<else_expr>}`
Expand Down Expand Up @@ -133,7 +133,15 @@ fn check<'tcx>(
else_pat: Option<&'tcx Pat<'_>>,
else_body: &'tcx Expr<'_>,
) {
if let Some(sugg_info) = check_with(
if let Some(SuggInfo {
needs_brackets,
scrutinee_impl_copy,
scrutinee_str,
as_ref_str,
body_str,
app,
requires_coercion,
}) = check_with(
cx,
expr,
scrutinee,
Expand All @@ -143,22 +151,27 @@ fn check<'tcx>(
else_body,
get_cond_expr,
) {
let body_str = add_ampersand_if_copy(sugg_info.body_str, sugg_info.scrutinee_impl_copy);
span_lint_and_sugg(
let body_str = add_ampersand_if_copy(body_str, scrutinee_impl_copy);
span_lint_and_then(
cx,
MANUAL_FILTER,
expr.span,
"manual implementation of `Option::filter`",
"try",
if sugg_info.needs_brackets {
format!(
"{{ {}{}.filter({body_str}) }}",
sugg_info.scrutinee_str, sugg_info.as_ref_str
)
} else {
format!("{}{}.filter({body_str})", sugg_info.scrutinee_str, sugg_info.as_ref_str)
|diag| {
diag.span_suggestion(
expr.span,
"try",
if needs_brackets {
format!("{{ {scrutinee_str}{as_ref_str}.filter({body_str}) }}")
} else {
format!("{scrutinee_str}{as_ref_str}.filter({body_str})")
},
app,
);
if requires_coercion {
diag.note("you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type");
}
},
sugg_info.app,
);
}
}
42 changes: 26 additions & 16 deletions clippy_lints/src/matches/manual_map.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use super::MANUAL_MAP;
use super::manual_utils::{SomeExpr, check_with};
use clippy_utils::diagnostics::span_lint_and_sugg;
use super::manual_utils::{SomeExpr, SuggInfo, check_with};
use clippy_utils::diagnostics::span_lint_and_then;

use clippy_utils::res::{MaybeDef, MaybeQPath};
use rustc_hir::LangItem::OptionSome;
Expand Down Expand Up @@ -42,7 +42,15 @@ fn check<'tcx>(
else_pat: Option<&'tcx Pat<'_>>,
else_body: &'tcx Expr<'_>,
) {
if let Some(sugg_info) = check_with(
if let Some(SuggInfo {
needs_brackets,
scrutinee_impl_copy: _,
scrutinee_str,
as_ref_str,
body_str,
app,
requires_coercion,
}) = check_with(
cx,
expr,
scrutinee,
Expand All @@ -52,24 +60,26 @@ fn check<'tcx>(
else_body,
get_some_expr,
) {
span_lint_and_sugg(
span_lint_and_then(
cx,
MANUAL_MAP,
expr.span,
"manual implementation of `Option::map`",
"try",
if sugg_info.needs_brackets {
format!(
"{{ {}{}.map({}) }}",
sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
)
} else {
format!(
"{}{}.map({})",
sugg_info.scrutinee_str, sugg_info.as_ref_str, sugg_info.body_str
)
|diag| {
diag.span_suggestion(
expr.span,
"try",
if needs_brackets {
format!("{{ {scrutinee_str}{as_ref_str}.map({body_str}) }}")
} else {
format!("{scrutinee_str}{as_ref_str}.map({body_str})")
},
app,
);
if requires_coercion {
diag.note("you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type");
}
},
sugg_info.app,
);
}
}
Expand Down
11 changes: 7 additions & 4 deletions clippy_lints/src/matches/manual_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,12 @@ where
return None;
}

let mut app = Applicability::MachineApplicable;

// `map` won't perform any adjustments.
if expr_requires_coercion(cx, expr) {
return None;
let requires_coercion = expr_requires_coercion(cx, expr);
if requires_coercion {
app = Applicability::MaybeIncorrect;
}

// Determine which binding mode to use.
Expand Down Expand Up @@ -111,8 +114,6 @@ where
None => return None,
}

let mut app = Applicability::MachineApplicable;

// Remove address-of expressions from the scrutinee. Either `as_ref` will be called, or
// it's being passed by value.
let scrutinee = peel_hir_expr_refs(scrutinee).0;
Expand Down Expand Up @@ -172,6 +173,7 @@ where
as_ref_str,
body_str,
app,
requires_coercion,
})
}

Expand All @@ -182,6 +184,7 @@ pub struct SuggInfo<'a> {
pub as_ref_str: &'a str,
pub body_str: String,
pub app: Applicability,
pub requires_coercion: bool,
}

// Checks whether the expression could be passed as a function, or whether a closure is needed.
Expand Down
17 changes: 17 additions & 0 deletions tests/ui/manual_filter_unfixable.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
//@no-rustfix
#![warn(clippy::manual_filter)]

// the suggestion is missing the necessary adjustments
fn issue16031() -> Option<&'static i32> {
match Some(&&0) {
//~^ manual_filter
None => None,
Some(x) => {
if **x > 0 {
None
} else {
Some(x)
}
},
}
}
18 changes: 18 additions & 0 deletions tests/ui/manual_filter_unfixable.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
error: manual implementation of `Option::filter`
--> tests/ui/manual_filter_unfixable.rs:6:5
|
LL | / match Some(&&0) {
LL | |
LL | | None => None,
LL | | Some(x) => {
... |
LL | | },
LL | | }
| |_____^ help: try: `Some(&&0).filter(|&x| **x <= 0)`
|
= note: you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type
= note: `-D clippy::manual-filter` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::manual_filter)]`

error: aborting due to 1 previous error

19 changes: 1 addition & 18 deletions tests/ui/manual_map_option.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -113,16 +113,7 @@ fn main() {
}

// #6811
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,
};
Some(0).map(|x| vec![x]);

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

Expand Down Expand Up @@ -154,12 +145,4 @@ fn main() {
None => None,
};
}

// #7077
let s = &String::new();
#[allow(clippy::needless_match)]
let _: Option<&str> = match Some(s) {
Some(s) => Some(s),
None => None,
};
}
15 changes: 1 addition & 14 deletions tests/ui/manual_map_option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,16 +183,11 @@ fn main() {

// #6811
match Some(0) {
//~^ manual_map
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,
};

match option_env!("") {
//~^ manual_map
Some(x) => Some(String::from(x)),
Expand Down Expand Up @@ -237,12 +232,4 @@ fn main() {
None => None,
};
}

// #7077
let s = &String::new();
#[allow(clippy::needless_match)]
let _: Option<&str> = match Some(s) {
Some(s) => Some(s),
None => None,
};
}
20 changes: 16 additions & 4 deletions tests/ui/manual_map_option.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,19 @@ 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:196:5
--> tests/ui/manual_map_option.rs:185:5
|
LL | / match Some(0) {
LL | |
LL | | Some(x) => Some(vec![x]),
LL | | None => None,
LL | | };
| |_____^ help: try: `Some(0).map(|x| vec![x])`
|
= note: you may need to add explicit `as` casts/dereferences if this `match` is coerced to another type

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

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

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

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

Loading