Skip to content
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

Jump to bottom

Use original variable name in the suggestion #10175

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 3 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
60 changes: 35 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,20 @@ 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})")
},
PatKind::TupleStruct(ref w, ..) => {
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})")
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giraffate @est31 Thank you for your advice:)
Finally, it's addressed by separating the PatKind::Or case from the PatKind::TupleStruct case, which we should fixed in the PR.

Could you review it again? thanks

Copy link
Member

Choose a reason for hiding this comment

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

The code is absolutely not enough but it will improve the situation at least for the Some and Ok cases. The biggest issue I think is that tuple structs with multiple arguments are not supported, as in:

let foo = if let SomeEnum::Val(v, 0) = e() { v } else { _ };

Will be turned into:

let SomeEnum::Val(v) = e() else { _ };

Copy link
Member

Choose a reason for hiding this comment

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

But I think the PR should be accepted. 👍

Copy link
Contributor Author

@koka831 koka831 Feb 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@est31 Thank you for the quick review!
I see the example above should be handled like below:

let SomeEnum::Val(foo, 0) = e() { v } else { ... };
               // ^ replaced ideally

In this PR, I made a change to keep left if TupleStruct has more than two arguments. ref
I hope it's enough for now:pray:

_ => {
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
32 changes: 16 additions & 16 deletions tests/ui/manual_let_else.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:18: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`

Expand All @@ -18,7 +18,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + return;
LL + };
|
Expand Down Expand Up @@ -48,19 +48,19 @@ error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:38: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
|
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
|
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
Expand All @@ -74,7 +74,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + std::process::abort()
LL + };
|
Expand All @@ -91,7 +91,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + if true { return } else { panic!() }
LL + };
|
Expand All @@ -109,7 +109,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + if true {}
LL + panic!();
LL + };
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 @@ -141,7 +141,7 @@ error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:80: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
Expand All @@ -157,7 +157,7 @@ LL | | };
|
help: consider writing
|
LL ~ let Some(v_some) = g() else {
LL ~ let Some(v) = g() else {
LL + match panic!() {
LL + _ => {},
LL + }
Expand All @@ -178,7 +178,7 @@ 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");
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 @@ -226,7 +226,7 @@ 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 + };
|
Expand All @@ -252,7 +252,7 @@ error: this could be rewritten as `let...else`
--> $DIR/manual_let_else.rs:134: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
Expand All @@ -266,7 +266,7 @@ 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

4 changes: 2 additions & 2 deletions tests/ui/manual_let_else_match.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | / let v = match g() {
LL | | Some(v_some) => v_some,
LL | | None => return,
LL | | };
| |______^ 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`

Expand All @@ -16,7 +16,7 @@ LL | / let v = match g() {
LL | | Some(v_some) => v_some,
LL | | _ => return,
LL | | };
| |______^ help: consider writing: `let Some(v_some) = g() else { return };`
| |______^ help: consider writing: `let Some(v) = g() else { return };`

error: this could be rewritten as `let...else`
--> $DIR/manual_let_else_match.rs:44:9
Expand Down