Skip to content

Gate collapsible_if let_chains lints on edition 2024 and MSRV #14723

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.

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
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
17 changes: 9 additions & 8 deletions clippy_lints/src/collapsible_if.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use clippy_config::Conf;
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::{IntoSpan as _, SpanRangeExt, snippet, snippet_block, snippet_block_with_applicability};
use rustc_ast::BinOpKind;
use rustc_errors::Applicability;
use rustc_hir::{Block, Expr, ExprKind, StmtKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty::TyCtxt;
use rustc_session::impl_lint_pass;
use rustc_span::Span;

Expand Down Expand Up @@ -78,14 +78,14 @@ declare_clippy_lint! {
}

pub struct CollapsibleIf {
let_chains_enabled: bool,
msrv: Msrv,
lint_commented_code: bool,
}

impl CollapsibleIf {
pub fn new(tcx: TyCtxt<'_>, conf: &'static Conf) -> Self {
pub fn new(conf: &'static Conf) -> Self {
Self {
let_chains_enabled: tcx.features().let_chains(),
msrv: conf.msrv,
lint_commented_code: conf.lint_commented_code,
}
}
Expand Down Expand Up @@ -127,7 +127,7 @@ impl CollapsibleIf {
if let Some(inner) = expr_block(then)
&& cx.tcx.hir_attrs(inner.hir_id).is_empty()
&& let ExprKind::If(check_inner, _, None) = &inner.kind
&& self.eligible_condition(check_inner)
&& self.eligible_condition(cx, check_inner)
&& let ctxt = expr.span.ctxt()
&& inner.span.ctxt() == ctxt
&& (self.lint_commented_code || !block_starts_with_comment(cx, then))
Expand Down Expand Up @@ -163,8 +163,9 @@ impl CollapsibleIf {
}
}

pub fn eligible_condition(&self, cond: &Expr<'_>) -> bool {
self.let_chains_enabled || !matches!(cond.kind, ExprKind::Let(..))
fn eligible_condition(&self, cx: &LateContext<'_>, cond: &Expr<'_>) -> bool {
!matches!(cond.kind, ExprKind::Let(..))
|| (cx.tcx.sess.edition().at_least_rust_2024() && self.msrv.meets(cx, msrvs::LET_CHAINS))
Comment on lines +166 to +168
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to prefer passing an extra argument every time (cx) and doing an edition comparison, while the edition being ≥2024 could instead be checked once from tcx at CollapsibleIf::new() time? The performance hit should be unnoticeable, but I wonder why you made this choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

cx is required for the MSRV check so doing so doesn't remove the extra arg

It could've been done once in new, it's not really something I actively considered. Perf wise it's an extra pointer dereference or two, doesn't seem worth splitting up the condition

}
}

Expand All @@ -180,7 +181,7 @@ impl LateLintPass<'_> for CollapsibleIf {
{
Self::check_collapsible_else_if(cx, then.span, else_);
} else if else_.is_none()
&& self.eligible_condition(cond)
&& self.eligible_condition(cx, cond)
&& let ExprKind::Block(then, None) = then.kind
{
self.check_collapsible_if_if(cx, expr, cond, then);
Expand Down
2 changes: 1 addition & 1 deletion clippy_lints/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -731,7 +731,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
store.register_early_pass(|| Box::new(unused_unit::UnusedUnit));
store.register_late_pass(|_| Box::new(unused_unit::UnusedUnit));
store.register_late_pass(|_| Box::new(returns::Return));
store.register_late_pass(move |tcx| Box::new(collapsible_if::CollapsibleIf::new(tcx, conf)));
store.register_late_pass(move |_| Box::new(collapsible_if::CollapsibleIf::new(conf)));
store.register_late_pass(|_| Box::new(items_after_statements::ItemsAfterStatements));
store.register_early_pass(|| Box::new(precedence::Precedence));
store.register_late_pass(|_| Box::new(needless_parens_on_range_literals::NeedlessParensOnRangeLiterals));
Expand Down
1 change: 1 addition & 0 deletions clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ macro_rules! msrv_aliases {

// names may refer to stabilized feature flags or library items
msrv_aliases! {
1,88,0 { LET_CHAINS }
1,87,0 { OS_STR_DISPLAY, INT_MIDPOINT }
1,85,0 { UINT_FLOAT_MIDPOINT }
1,84,0 { CONST_OPTION_AS_SLICE, MANUAL_DANGLING_PTR }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#![feature(let_chains)]
#![warn(clippy::collapsible_if)]

fn main() {
Expand Down
1 change: 0 additions & 1 deletion tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#![feature(let_chains)]
#![warn(clippy::collapsible_if)]

fn main() {
Expand Down
6 changes: 3 additions & 3 deletions tests/ui-toml/collapsible_if/collapsible_if_let_chains.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this `if` statement can be collapsed
--> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:5:5
--> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:4:5
|
LL | / if let Some(a) = Some(3) {
LL | | // with comment
Expand All @@ -21,7 +21,7 @@ LL ~ }
|

error: this `if` statement can be collapsed
--> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:13:5
--> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:12:5
|
LL | / if let Some(a) = Some(3) {
LL | | // with comment
Expand All @@ -41,7 +41,7 @@ LL ~ }
|

error: this `if` statement can be collapsed
--> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:21:5
--> tests/ui-toml/collapsible_if/collapsible_if_let_chains.rs:20:5
|
LL | / if Some(3) == Some(4).map(|x| x - 1) {
LL | | // with comment
Expand Down
79 changes: 39 additions & 40 deletions tests/ui/auxiliary/proc_macro_attr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,45 +51,44 @@ pub fn rename_my_lifetimes(_args: TokenStream, input: TokenStream) -> TokenStrea

fn mut_receiver_of(sig: &mut Signature) -> Option<&mut FnArg> {
let arg = sig.inputs.first_mut()?;
if let FnArg::Typed(PatType { pat, .. }) = arg {
if let Pat::Ident(PatIdent { ident, .. }) = &**pat {
if ident == "self" {
return Some(arg);
}
}
if let FnArg::Typed(PatType { pat, .. }) = arg
&& let Pat::Ident(PatIdent { ident, .. }) = &**pat
&& ident == "self"
{
Some(arg)
} else {
None
}
None
}

let mut elided = 0;
let mut item = parse_macro_input!(input as ItemImpl);

// Look for methods having arbitrary self type taken by &mut ref
for inner in &mut item.items {
if let ImplItem::Fn(method) = inner {
if let Some(FnArg::Typed(pat_type)) = mut_receiver_of(&mut method.sig) {
if let box Type::Reference(reference) = &mut pat_type.ty {
// Target only unnamed lifetimes
let name = match &reference.lifetime {
Some(lt) if lt.ident == "_" => make_name(elided),
None => make_name(elided),
_ => continue,
};
elided += 1;

// HACK: Syn uses `Span` from the proc_macro2 crate, and does not seem to reexport it.
// In order to avoid adding the dependency, get a default span from a nonexistent token.
// A default span is needed to mark the code as coming from expansion.
let span = Star::default().span();

// Replace old lifetime with the named one
let lifetime = Lifetime::new(&name, span);
reference.lifetime = Some(parse_quote!(#lifetime));

// Add lifetime to the generics of the method
method.sig.generics.params.push(parse_quote!(#lifetime));
}
}
if let ImplItem::Fn(method) = inner
&& let Some(FnArg::Typed(pat_type)) = mut_receiver_of(&mut method.sig)
&& let box Type::Reference(reference) = &mut pat_type.ty
{
// Target only unnamed lifetimes
let name = match &reference.lifetime {
Some(lt) if lt.ident == "_" => make_name(elided),
None => make_name(elided),
_ => continue,
};
elided += 1;

// HACK: Syn uses `Span` from the proc_macro2 crate, and does not seem to reexport it.
// In order to avoid adding the dependency, get a default span from a nonexistent token.
// A default span is needed to mark the code as coming from expansion.
let span = Star::default().span();

// Replace old lifetime with the named one
let lifetime = Lifetime::new(&name, span);
reference.lifetime = Some(parse_quote!(#lifetime));

// Add lifetime to the generics of the method
method.sig.generics.params.push(parse_quote!(#lifetime));
}
}

Expand Down Expand Up @@ -129,15 +128,15 @@ pub fn fake_desugar_await(_args: TokenStream, input: TokenStream) -> TokenStream
let mut async_fn = parse_macro_input!(input as syn::ItemFn);

for stmt in &mut async_fn.block.stmts {
if let syn::Stmt::Expr(syn::Expr::Match(syn::ExprMatch { expr: scrutinee, .. }), _) = stmt {
if let syn::Expr::Await(syn::ExprAwait { base, await_token, .. }) = scrutinee.as_mut() {
let blc = quote_spanned!( await_token.span => {
#[allow(clippy::let_and_return)]
let __pinned = #base;
__pinned
});
*scrutinee = parse_quote!(#blc);
}
if let syn::Stmt::Expr(syn::Expr::Match(syn::ExprMatch { expr: scrutinee, .. }), _) = stmt
&& let syn::Expr::Await(syn::ExprAwait { base, await_token, .. }) = scrutinee.as_mut()
{
let blc = quote_spanned!( await_token.span => {
#[allow(clippy::let_and_return)]
let __pinned = #base;
__pinned
});
*scrutinee = parse_quote!(#blc);
}
}

Expand Down
1 change: 0 additions & 1 deletion tests/ui/auxiliary/proc_macros.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
#![feature(let_chains)]
#![feature(proc_macro_span)]
#![allow(clippy::needless_if, dead_code)]

Expand Down
19 changes: 0 additions & 19 deletions tests/ui/collapsible_if.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -101,27 +101,8 @@ fn main() {
}
}

// Test behavior wrt. `let_chains`.
// None of the cases below should be collapsed.
fn truth() -> bool { true }

// Prefix:
if let 0 = 1 {
if truth() {}
}

// Suffix:
if truth() {
if let 0 = 1 {}
}

// Midfix:
if truth() {
if let 0 = 1 {
if truth() {}
}
}

// Fix #5962
if matches!(true, true)
&& matches!(true, true) {}
Expand Down
19 changes: 0 additions & 19 deletions tests/ui/collapsible_if.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,27 +108,8 @@ fn main() {
}
}

// Test behavior wrt. `let_chains`.
// None of the cases below should be collapsed.
fn truth() -> bool { true }

// Prefix:
if let 0 = 1 {
if truth() {}
}

// Suffix:
if truth() {
if let 0 = 1 {}
}

// Midfix:
if truth() {
if let 0 = 1 {
if truth() {}
}
}

// Fix #5962
if matches!(true, true) {
if matches!(true, true) {}
Expand Down
8 changes: 4 additions & 4 deletions tests/ui/collapsible_if.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ LL ~ }
|

error: this `if` statement can be collapsed
--> tests/ui/collapsible_if.rs:133:5
--> tests/ui/collapsible_if.rs:114:5
|
LL | / if matches!(true, true) {
LL | | if matches!(true, true) {}
Expand All @@ -141,7 +141,7 @@ LL ~ && matches!(true, true) {}
|

error: this `if` statement can be collapsed
--> tests/ui/collapsible_if.rs:139:5
--> tests/ui/collapsible_if.rs:120:5
|
LL | / if matches!(true, true) && truth() {
LL | | if matches!(true, true) {}
Expand All @@ -155,7 +155,7 @@ LL ~ && matches!(true, true) {}
|

error: this `if` statement can be collapsed
--> tests/ui/collapsible_if.rs:151:5
--> tests/ui/collapsible_if.rs:132:5
|
LL | / if true {
LL | | if true {
Expand All @@ -173,7 +173,7 @@ LL ~ }
|

error: this `if` statement can be collapsed
--> tests/ui/collapsible_if.rs:168:5
--> tests/ui/collapsible_if.rs:149:5
|
LL | / if true {
LL | | if true {
Expand Down
68 changes: 68 additions & 0 deletions tests/ui/collapsible_if_let_chains.edition2024.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
//@revisions: edition2021 edition2024
//@[edition2021] edition:2021
//@[edition2024] edition:2024
//@[edition2021] check-pass

#![warn(clippy::collapsible_if)]

fn main() {
if let Some(a) = Some(3) {
// with comment, so do not lint
if let Some(b) = Some(4) {
let _ = a + b;
}
}

//~[edition2024]v collapsible_if
if let Some(a) = Some(3)
&& let Some(b) = Some(4) {
let _ = a + b;
}

//~[edition2024]v collapsible_if
if let Some(a) = Some(3)
&& a + 1 == 4 {
let _ = a;
}

//~[edition2024]v collapsible_if
if Some(3) == Some(4).map(|x| x - 1)
&& let Some(b) = Some(4) {
let _ = b;
}

fn truth() -> bool {
true
}

// Prefix:
//~[edition2024]v collapsible_if
if let 0 = 1
&& truth() {}

// Suffix:
//~[edition2024]v collapsible_if
if truth()
&& let 0 = 1 {}

// Midfix:
//~[edition2024]vvv collapsible_if
//~[edition2024]v collapsible_if
if truth()
&& let 0 = 1
&& truth() {}
}

#[clippy::msrv = "1.87.0"]
fn msrv_1_87() {
if let 0 = 1 {
if true {}
}
}

#[clippy::msrv = "1.88.0"]
fn msrv_1_88() {
//~[edition2024]v collapsible_if
if let 0 = 1
&& true {}
}
Loading