Skip to content

Use original variable name in the suggestion #10175

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 4 commits into from
May 18, 2023
Merged

Use original variable name in the suggestion #10175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a408e76
Suggest the correct variable name for `manual_let_else`
koka831 Feb 1, 2023
4931f45
Split if conditions by its semantics
koka831 Feb 1, 2023
20338fd
Remove old feature flag in code comment
koka831 Feb 1, 2023
07c8c50
Avoid renaming for TupleStruct with multiple arguments
koka831 Feb 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
61 changes: 36 additions & 25 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 @@ -37,7 +37,6 @@ declare_clippy_lint! {
/// Could be written:
///
/// ```rust
/// # #![feature(let_else)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO let_else has been stable since 1.65 so it can be removed?

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely, yes.

/// # fn main () {
/// # let w = Some(0);
/// let Some(v) = w else { return };
Expand Down Expand Up @@ -68,29 +67,23 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);

impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) {
let if_let_or_match = if_chain! {
if self.msrv.meets(msrvs::LET_ELSE);
if !in_external_macro(cx.sess(), stmt.span);
if let StmtKind::Local(local) = stmt.kind;
if let Some(init) = local.init;
if local.els.is_none();
if local.ty.is_none();
if init.span.ctxt() == stmt.span.ctxt();
if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init);
then {
if_let_or_match
} else {
return;
}
};
if !self.msrv.meets(msrvs::LET_ELSE) || in_external_macro(cx.sess(), stmt.span) {
return;
}

if let StmtKind::Local(local) = stmt.kind &&
let Some(init) = local.init &&
local.els.is_none() &&
local.ty.is_none() &&
init.span.ctxt() == stmt.span.ctxt() &&
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why rustfmt doesn't work for this case, but it would be better to deepen the indent.

Suggested change
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) {
let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init)
{
match if_let_or_match {

Copy link
Member

Choose a reason for hiding this comment

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

rustfmt doesn't support let chains right now: rust-lang/rustfmt#5203

Copy link
Contributor

Choose a reason for hiding this comment

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

@koka831 Sorry for the my late reviewing. Can this be addressed?

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(if_else) = if_else;
if expr_diverges(cx, if_else);
then {
emit_manual_let_else(cx, stmt.span, if_let_expr, let_pat, if_else);
emit_manual_let_else(cx, stmt.span, if_let_expr, local.pat, let_pat, if_else);
}
},
IfLetOrMatch::Match(match_expr, arms, source) => {
Expand Down Expand Up @@ -120,15 +113,23 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
return;
}

emit_manual_let_else(cx, stmt.span, match_expr, pat_arm.pat, diverging_arm.body);
emit_manual_let_else(cx, stmt.span, match_expr, local.pat, pat_arm.pat, diverging_arm.body);
},
}
};
}

extract_msrv_attr!(LateContext);
}

fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat: &Pat<'_>, else_body: &Expr<'_>) {
fn emit_manual_let_else(
cx: &LateContext<'_>,
span: Span,
expr: &Expr<'_>,
local: &Pat<'_>,
pat: &Pat<'_>,
else_body: &Expr<'_>,
) {
span_lint_and_then(
cx,
MANUAL_LET_ELSE,
Expand All @@ -137,12 +138,11 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat:
|diag| {
// This is far from perfect, for example there needs to be:
// * mut additions for the bindings
// * renamings of the bindings
// * renamings of the bindings for `PatKind::Or`
// * unused binding collision detection with existing ones
// * putting patterns with at the top level | inside ()
// for this to be machine applicable.
let mut app = Applicability::HasPlaceholders;
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
let (sn_expr, _) = snippet_with_context(cx, expr.span, span.ctxt(), "", &mut app);
let (sn_else, _) = snippet_with_context(cx, else_body.span, span.ctxt(), "", &mut app);

Expand All @@ -151,10 +151,21 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat:
} else {
format!("{{ {sn_else} }}")
};
let sn_bl = if matches!(pat.kind, PatKind::Or(..)) {
format!("({sn_pat})")
} else {
sn_pat.into_owned()
let sn_bl = match pat.kind {
PatKind::Or(..) => {
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
format!("({sn_pat})")
},
// Replace the variable name iff `TupleStruct` has one argument like `Variant(v)`.
PatKind::TupleStruct(ref w, args, ..) if args.len() == 1 => {
let sn_wrapper = cx.sess().source_map().span_to_snippet(w.span()).unwrap_or_default();
let (sn_inner, _) = snippet_with_context(cx, local.span, span.ctxt(), "", &mut app);
format!("{sn_wrapper}({sn_inner})")
},
_ => {
let (sn_pat, _) = snippet_with_context(cx, pat.span, span.ctxt(), "", &mut app);
sn_pat.into_owned()
},
};
let sugg = format!("let {sn_bl} = {sn_expr} else {else_bl};");
diag.span_suggestion(span, "consider writing", sugg, app);
Expand Down
15 changes: 15 additions & 0 deletions tests/ui/manual_let_else.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,12 @@
)]
#![warn(clippy::manual_let_else)]

enum Variant {
A(usize, usize),
B(usize),
C,
}

fn g() -> Option<()> {
None
}
Expand Down Expand Up @@ -135,6 +141,15 @@ fn fire() {
};
}
create_binding_if_some!(w, g());

fn e() -> Variant {
Variant::A(0, 0)
}

// Should not be renamed
let v = if let Variant::A(a, 0) = e() { a } else { return };
// Should be renamed
let v = if let Variant::B(b) = e() { b } else { return };
}

fn not_fire() {
Expand Down
82 changes: 47 additions & 35 deletions tests/ui/manual_let_else.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:18:5
--> $DIR/manual_let_else.rs:24:5
|
LL | let v = if let Some(v_some) = g() { v_some } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { return };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
|
= note: `-D clippy::manual-let-else` implied by `-D warnings`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:19:5
--> $DIR/manual_let_else.rs:25:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -18,13 +18,13 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + return;
LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:25:5
--> $DIR/manual_let_else.rs:31:5
|
LL | / let v = if let Some(v) = g() {
LL | | // Blocks around the identity should have no impact
Expand All @@ -45,25 +45,25 @@ LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:38:9
--> $DIR/manual_let_else.rs:44:9
|
LL | let v = if let Some(v_some) = g() { v_some } else { continue };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { continue };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { continue };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:39:9
--> $DIR/manual_let_else.rs:45:9
|
LL | let v = if let Some(v_some) = g() { v_some } else { break };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { break };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { break };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:43:5
--> $DIR/manual_let_else.rs:49:5
|
LL | let v = if let Some(v_some) = g() { v_some } else { panic!() };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { panic!() };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { panic!() };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:46:5
--> $DIR/manual_let_else.rs:52:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -74,13 +74,13 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + std::process::abort()
LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:53:5
--> $DIR/manual_let_else.rs:59:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -91,13 +91,13 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + if true { return } else { panic!() }
LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:60:5
--> $DIR/manual_let_else.rs:66:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -109,14 +109,14 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + if true {}
LL + panic!();
LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:70:5
--> $DIR/manual_let_else.rs:76:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -129,7 +129,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + match () {
LL + _ if panic!() => {},
LL + _ => panic!(),
Expand All @@ -138,13 +138,13 @@ LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:80:5
--> $DIR/manual_let_else.rs:86:5
|
LL | let v = if let Some(v_some) = g() { v_some } else { if panic!() {} };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v_some) = g() else { if panic!() {} };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { if panic!() {} };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:83:5
--> $DIR/manual_let_else.rs:89:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -157,15 +157,15 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + match panic!() {
LL + _ => {},
LL + }
LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:92:5
--> $DIR/manual_let_else.rs:98:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -178,15 +178,15 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else { if true {
LL ~ let Some(v) = g() else { if true {
LL + return;
LL + } else {
LL + panic!("diverge");
LL + } };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:101:5
--> $DIR/manual_let_else.rs:107:5
|
LL | / let v = if let Some(v_some) = g() {
LL | | v_some
Expand All @@ -199,7 +199,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + match (g(), g()) {
LL + (Some(_), None) => return,
LL + (None, Some(_)) => {
Expand All @@ -215,7 +215,7 @@ LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:118:5
--> $DIR/manual_let_else.rs:124:5
|
LL | / let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) {
LL | | v_some
Expand All @@ -226,13 +226,13 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g().map(|v| (v, 42)) else {
LL ~ let Some((v, w)) = g().map(|v| (v, 42)) else {
LL + return;
LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:125:5
--> $DIR/manual_let_else.rs:131:5
|
LL | / let v = if let (Some(v_some), w_some) = (g(), 0) {
LL | | (w_some, v_some)
Expand All @@ -249,24 +249,36 @@ LL + };
|

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:134:13
--> $DIR/manual_let_else.rs:140:13
|
LL | let $n = if let Some(v) = $e { v } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return };`
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some($n) = g() else { return };`
...
LL | create_binding_if_some!(w, g());
| ------------------------------- in this macro invocation
|
= note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:247:5
--> $DIR/manual_let_else.rs:150:5
|
LL | let v = if let Variant::A(a, 0) = e() { a } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::A(a, 0) = e() else { return };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:152:5
|
LL | let v = if let Variant::B(b) = e() { b } else { return };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Variant::B(v) = e() else { return };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:262:5
|
LL | / let _ = match ff {
LL | | Some(value) => value,
LL | | _ => macro_call!(),
LL | | };
| |______^ help: consider writing: `let Some(value) = ff else { macro_call!() };`
| |______^ help: consider writing: `let Some(_) = ff else { macro_call!() };`

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

Loading