mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Support multi-binding situations as well as tuple patterns

Browse Source
This commit is contained in:
est31 2023-06-02 01:00:03 +02:00
parent 0a7366897d
commit 86d57b7bd4
3 changed files with 174 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

217
clippy_lints/src/manual_let_else.rs
View File

@ -6,17 +6,18 @@ use clippy_utils::source::snippet_with_context;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::visitors::{Descend, Visitable};
use if_chain::if_chain;
use rustc_data_structures::fx::FxHashSet;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::Applicability;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty};
use rustc_lint::{LateContext, LateLintPass, LintContext};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_tool_lint, impl_lint_pass};
use rustc_span::symbol::sym;
use rustc_span::symbol::{sym, Symbol};
use rustc_span::Span;
use serde::Deserialize;
use std::ops::ControlFlow;
use std::slice;
declare_clippy_lint! {
/// ### What it does
@ -81,11 +82,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
{
match if_let_or_match {
IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! {
if expr_is_simple_identity(let_pat, if_then);
if let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then);
if let Some(if_else) = if_else;
if expr_diverges(cx, if_else);
then {
emit_manual_let_else(cx, stmt.span, if_let_expr, local.pat, let_pat, if_else);
emit_manual_let_else(cx, stmt.span, if_let_expr, &ident_map, let_pat, if_else);
}
},
IfLetOrMatch::Match(match_expr, arms, source) => {
@ -118,11 +119,11 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
return;
}
let pat_arm = &arms[1 - idx];
if !expr_is_simple_identity(pat_arm.pat, pat_arm.body) {
return;
}
let Some(ident_map) = expr_simple_identity_map(local.pat, pat_arm.pat, pat_arm.body) else {
return
};
emit_manual_let_else(cx, stmt.span, match_expr, local.pat, pat_arm.pat, diverging_arm.body);
emit_manual_let_else(cx, stmt.span, match_expr, &ident_map, pat_arm.pat, diverging_arm.body);
},
}
};
@ -135,7 +136,7 @@ fn emit_manual_let_else(
cx: &LateContext<'_>,
span: Span,
expr: &Expr<'_>,
local: &Pat<'_>,
ident_map: &FxHashMap<Symbol, &Pat<'_>>,
pat: &Pat<'_>,
else_body: &Expr<'_>,
) {
@ -159,62 +160,99 @@ fn emit_manual_let_else(
} else {
format!("{{ {sn_else} }}")
};
let sn_bl = replace_in_pattern(cx, span, local, pat, &mut app, true);
let sn_bl = replace_in_pattern(cx, span, ident_map, pat, &mut app, true);
let sugg = format!("let {sn_bl} = {sn_expr} else {else_bl};");
diag.span_suggestion(span, "consider writing", sugg, app);
},
);
}
// replaces the locals in the pattern
/// Replaces the locals in the pattern
///
/// For this example:
///
/// ```ignore
/// let (a, FooBar { b, c }) = if let Bar { Some(a_i), b_i } = ex { (a_i, b_i) } else { return };
/// ```
///
/// We have:
///
/// ```ignore
/// pat: Bar { Some(a_i), b_i }
/// ident_map: (a_i) -> (a), (b_i) -> (FooBar { b, c })
/// ```
///
/// We return:
///
/// ```ignore
/// Bar { Some(a), b_i: FooBar { b, c } }
/// ```
fn replace_in_pattern(
cx: &LateContext<'_>,
span: Span,
local: &Pat<'_>,
ident_map: &FxHashMap<Symbol, &Pat<'_>>,
pat: &Pat<'_>,
app: &mut Applicability,
top_level: bool,
) -> String {
let mut bindings_count = 0;
pat.each_binding_or_first(&mut |_, _, _, _| bindings_count += 1);
// If the pattern creates multiple bindings, exit early,
// as otherwise we might paste the pattern to the positions of multiple bindings.
if bindings_count > 1 {
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
return sn_pat.into_owned();
}
// We put a labeled block here so that we can implement the fallback in this function.
// As the function has multiple call sites, implementing the fallback via an Option<T>
// return type and unwrap_or_else would cause repetition. Similarly, the function also
// invokes the fall back multiple times.
'a: {
// If the ident map is empty, there is no replacement to do.
// The code following this if assumes a non-empty ident_map.
if ident_map.is_empty() {
break 'a;
}
match pat.kind {
PatKind::Binding(..) => {
let (sn_bdg, _) = snippet_with_context(cx, local.span, span.ctxt(), "", app);
return sn_bdg.to_string();
},
PatKind::Or(pats) => {
let patterns = pats
.iter()
.map(|pat| replace_in_pattern(cx, span, local, pat, app, false))
.collect::<Vec<_>>();
let or_pat = patterns.join(" | ");
if top_level {
return format!("({or_pat})");
} else {
match pat.kind {
PatKind::Binding(_ann, _id, binding_name, opt_subpt) => {
let Some(pat_to_put) = ident_map.get(&binding_name.name) else { break 'a };
let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app);
if let Some(subpt) = opt_subpt {
let subpt = replace_in_pattern(cx, span, ident_map, subpt, app, false);
return format!("{sn_ptp} @ {subpt}");
}
return sn_ptp.to_string();
},
PatKind::Or(pats) => {
let patterns = pats
.iter()
.map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false))
.collect::<Vec<_>>();
let or_pat = patterns.join(" | ");
if top_level {
return format!("({or_pat})");
}
return or_pat;
}
},
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
PatKind::TupleStruct(ref w, args, dot_dot_pos) => {
let mut args = args
.iter()
.map(|pat| replace_in_pattern(cx, span, local, pat, app, false))
.collect::<Vec<_>>();
if let Some(pos) = dot_dot_pos.as_opt_usize() {
args.insert(pos, "..".to_owned());
}
let args = args.join(", ");
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
return format!("{sn_wrapper}({args})");
},
_ => {},
},
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
PatKind::TupleStruct(ref w, args, dot_dot_pos) => {
let mut args = args
.iter()
.map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false))
.collect::<Vec<_>>();
if let Some(pos) = dot_dot_pos.as_opt_usize() {
args.insert(pos, "..".to_owned());
}
let args = args.join(", ");
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
return format!("{sn_wrapper}({args})");
},
PatKind::Tuple(args, dot_dot_pos) => {
let mut args = args
.iter()
.map(|pat| replace_in_pattern(cx, span, ident_map, pat, app, false))
.collect::<Vec<_>>();
if let Some(pos) = dot_dot_pos.as_opt_usize() {
args.insert(pos, "..".to_owned());
}
let args = args.join(", ");
return format!("({args})");
},
_ => {},
}
}
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", app);
sn_pat.into_owned()
@ -358,37 +396,74 @@ fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>, check_types: boo
!has_disallowed
}
/// Checks if the passed block is a simple identity referring to bindings created by the pattern
fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool {
// We support patterns with multiple bindings and tuples, like:
// let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
/// Checks if the passed block is a simple identity referring to bindings created by the pattern,
/// and if yes, returns a mapping between the relevant sub-pattern and the identifier it corresponds
/// to.
///
/// We support patterns with multiple bindings and tuples, e.g.:
///
/// ```ignore
/// let (foo_o, bar_o) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
/// ```
///
/// The expected params would be:
///
/// ```ignore
/// local_pat: (foo_o, bar_o)
/// let_pat: (Some(foo), bar)
/// expr: (foo, bar)
/// ```
///
/// We build internal `sub_pats` so that it looks like `[foo_o, bar_o]` and `paths` so that it looks
/// like `[foo, bar]`. Then we turn that into `FxHashMap [(foo) -> (foo_o), (bar) -> (bar_o)]` which
/// we return.
fn expr_simple_identity_map<'a, 'hir>(
local_pat: &'a Pat<'hir>,
let_pat: &'_ Pat<'hir>,
expr: &'_ Expr<'hir>,
) -> Option<FxHashMap<Symbol, &'a Pat<'hir>>> {
let peeled = peel_blocks(expr);
let paths = match peeled.kind {
ExprKind::Tup(exprs) | ExprKind::Array(exprs) => exprs,
ExprKind::Path(_) => std::slice::from_ref(peeled),
_ => return false,
let (sub_pats, paths) = match (local_pat.kind, peeled.kind) {
(PatKind::Tuple(pats, _), ExprKind::Tup(exprs)) | (PatKind::Slice(pats, ..), ExprKind::Array(exprs)) => {
(pats, exprs)
},
(_, ExprKind::Path(_)) => (slice::from_ref(local_pat), slice::from_ref(peeled)),
_ => return None,
};
// There is some length mismatch, which indicates usage of .. in the patterns above e.g.:
// let (a, ..) = if let [a, b, _c] = ex { (a, b) } else { ... };
// We bail in these cases as they should be rare.
if paths.len() != sub_pats.len() {
return None;
}
let mut pat_bindings = FxHashSet::default();
pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
let_pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
pat_bindings.insert(ident);
});
if pat_bindings.len() < paths.len() {
return false;
// This rebinds some bindings from the outer scope, or it repeats some copy-able bindings multiple
// times. We don't support these cases so we bail here. E.g.:
// let foo = 0;
// let (new_foo, bar, bar_copied) = if let Some(bar) = Some(0) { (foo, bar, bar) } else { .. };
return None;
}
for path in paths {
if_chain! {
if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind;
if let [path_seg] = path.segments;
then {
if !pat_bindings.remove(&path_seg.ident) {
return false;
}
} else {
return false;
let mut ident_map = FxHashMap::default();
for (sub_pat, path) in sub_pats.iter().zip(paths.iter()) {
if let ExprKind::Path(QPath::Resolved(_ty, path)) = path.kind &&
let [path_seg] = path.segments
{
let ident = path_seg.ident;
if !pat_bindings.remove(&ident) {
return None;
}
ident_map.insert(ident.name, sub_pat);
} else {
return None;
}
}
true
Some(ident_map)
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Deserialize)]

20
tests/ui/manual_let_else.rs
View File

@ -127,8 +127,8 @@ fn fire() {
return;
};
// Tuples supported for the identity block and pattern
let v = if let (Some(v_some), w_some) = (g(), 0) {
// Tuples supported with multiple bindings
let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) {
(w_some, v_some)
} else {
return;
@ -160,6 +160,9 @@ fn fire() {
};
// dot dot works
let v = if let Variant::A(.., a) = e() { a } else { return };
// () is preserved: a bit of an edge case but make sure it stays around
let w = if let (Some(v), ()) = (g(), ()) { v } else { return };
}
fn not_fire() {
@ -284,4 +287,17 @@ fn not_fire() {
};
1
};
// This would require creation of a suggestion of the form
// let v @ (Some(_), _) = (...) else { return };
// Which is too advanced for our code, so we just bail.
let v = if let (Some(v_some), w_some) = (g(), 0) {
(w_some, v_some)
} else {
return;
};
}
struct S {
v: u32,
}

14
tests/ui/manual_let_else.stderr
View File

@ -234,7 +234,7 @@ LL + };
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:131:5
|
LL | / let v = if let (Some(v_some), w_some) = (g(), 0) {
LL | / let (w, S { v }) = if let (Some(v_some), w_some) = (g().map(|_| S { v: 0 }), 0) {
LL | | (w_some, v_some)
LL | | } else {
LL | | return;
@ -243,7 +243,7 @@ LL | | };
|
help: consider writing
|
LL ~ let (Some(v_some), w_some) = (g(), 0) else {
LL ~ let (Some(S { v }), w) = (g().map(|_| S { v: 0 }), 0) else {
LL + return;
LL + };
|
@ -295,7 +295,13 @@ LL | let v = if let Variant::A(.., a) = e() { a } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(.., v) = e() else { return };`
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:272:5
--> $DIR/manual_let_else.rs:165:5
|
LL | let w = if let (Some(v), ()) = (g(), ()) { v } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let (Some(w), ()) = (g(), ()) else { return };`
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:275:5
|
LL | / let _ = match ff {
LL | | Some(value) => value,
@ -303,5 +309,5 @@ LL | | _ => macro_call!(),
LL | | };
| |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`
error: aborting due to 22 previous errors
error: aborting due to 23 previous errors