Skip to content

manual_let_else: support struct patterns #10866

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 3, 2023
Merged

manual_let_else: support struct patterns #10866

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a73668
manual_let_else: only add () around PatKind::Or at the top level
est31 Jun 1, 2023
86d57b7
Support multi-binding situations as well as tuple patterns
est31 Jun 1, 2023
f538402
Support struct patterns
est31 Jun 1, 2023
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
247 changes: 177 additions & 70 deletions clippy_lints/src/manual_let_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) => {
Expand Down Expand Up @@ -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);
},
}
};
Expand All @@ -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<'_>,
) {
Expand All @@ -146,8 +147,8 @@ fn emit_manual_let_else(
"this could be rewritten as `let...else`",
|diag| {
// This is far from perfect, for example there needs to be:
// * tracking for multi-binding cases: let (foo, bar) = if let (Some(foo), Ok(bar)) = ...
// * renamings of the bindings for many `PatKind`s like structs, slices, etc.
// * renamings of the bindings for many `PatKind`s like slices, etc.
// * limitations in the existing replacement algorithms
// * unused binding collision detection with existing ones
// for this to be machine applicable.
let mut app = Applicability::HasPlaceholders;
Expand All @@ -159,57 +160,126 @@ fn emit_manual_let_else(
} else {
format!("{{ {sn_else} }}")
};
let sn_bl = replace_in_pattern(cx, span, local, pat, &mut app);
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))
.collect::<Vec<_>>();
let or_pat = patterns.join(" | ");
return format!("({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))
.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})");
},
_ => {},
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;
},
PatKind::Struct(path, fields, has_dot_dot) => {
let fields = fields
.iter()
.map(|fld| {
if let PatKind::Binding(_, _, name, None) = fld.pat.kind &&
let Some(pat_to_put) = ident_map.get(&name.name)
{
let (sn_fld_name, _) = snippet_with_context(cx, fld.ident.span, span.ctxt(), "", app);
let (sn_ptp, _) = snippet_with_context(cx, pat_to_put.span, span.ctxt(), "", app);
// TODO: this is a bit of a hack, but it does its job. Ideally, we'd check if pat_to_put is
// a PatKind::Binding but that is also hard to get right.
if sn_fld_name == sn_ptp {
// Field init shorthand
return format!("{sn_fld_name}");
}
return format!("{sn_fld_name}: {sn_ptp}");
}
let (sn_fld, _) = snippet_with_context(cx, fld.span, span.ctxt(), "", app);
sn_fld.into_owned()
})
.collect::<Vec<_>>();
let fields_string = fields.join(", ");

let dot_dot_str = if has_dot_dot { " .." } else { "" };
let (sn_pth, _) = snippet_with_context(cx, path.span(), span.ctxt(), "", app);
return format!("{sn_pth} {{ {fields_string}{dot_dot_str} }}");
},
// 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()
Expand Down Expand Up @@ -353,37 +423,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.:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: probably possible to split into two lets, probably not worth it.

(Might be interesting to design a different lint that detects when let (a, b) = ... is splittable)

// 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)]
Expand Down
47 changes: 45 additions & 2 deletions tests/ui/manual_let_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -160,6 +160,30 @@ 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 };

// Tuple structs work
let w = if let Some(S { v: x }) = Some(S { v: 0 }) {
x
} else {
return;
};

// Field init shorthand is suggested
let v = if let Some(S { v: x }) = Some(S { v: 0 }) {
x
} else {
return;
};

// Multi-field structs also work
let (x, S { v }, w) = if let Some(U { v, w, x }) = None::<U<S<()>>> {
(x, v, w)
} else {
return;
};
}

fn not_fire() {
Expand Down Expand Up @@ -284,4 +308,23 @@ 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<T> {
v: T,
}

struct U<T> {
v: T,
w: T,
x: T,
}
Loading