From 1c277d1be896ad5499a464d92a3d427488df470e Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Sun, 21 May 2023 23:33:42 -0400 Subject: [PATCH 1/5] Unit tests highlighting unsafe match issue These unit tests generate non-compilable code. I did NOT `bless` them on purpose because the stderr output is not good. I'm surprised we don't auto-compile the suggestions here - is this something that can be easily enabled? See #10808 --- tests/ui/single_match.rs | 23 +++++++++- tests/ui/single_match_else.rs | 86 ++++++++++++++++++++++++++++++++++- 2 files changed, 107 insertions(+), 2 deletions(-) diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index d0c9b7b5663e..d474088aab66 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -1,5 +1,5 @@ #![warn(clippy::single_match)] -#![allow(clippy::uninlined_format_args)] +#![allow(unused, clippy::uninlined_format_args)] fn dummy() {} @@ -244,3 +244,24 @@ fn main() { _ => 0, }; } + +fn issue_10808(bar: Option) { + match bar { + Some(v) => unsafe { + let r = &v as *const i32; + println!("{}", *r); + }, + _ => {}, + } + + match bar { + Some(v) => { + // this comment prevents rustfmt from collapsing the block + unsafe { + let r = &v as *const i32; + println!("{}", *r); + } + }, + _ => {}, + } +} diff --git a/tests/ui/single_match_else.rs b/tests/ui/single_match_else.rs index c8ac768b60f6..768316edad53 100644 --- a/tests/ui/single_match_else.rs +++ b/tests/ui/single_match_else.rs @@ -1,6 +1,6 @@ //@aux-build: proc_macros.rs #![warn(clippy::single_match_else)] -#![allow(clippy::needless_return, clippy::no_effect, clippy::uninlined_format_args)] +#![allow(unused, clippy::needless_return, clippy::no_effect, clippy::uninlined_format_args)] extern crate proc_macros; use proc_macros::with_span; @@ -115,3 +115,87 @@ fn main() { } } } + +fn issue_10808(bar: Option) { + match bar { + Some(v) => unsafe { + let r = &v as *const i32; + println!("{}", *r); + }, + None => { + println!("None1"); + println!("None2"); + }, + } + + match bar { + Some(v) => { + println!("Some"); + println!("{v}"); + }, + None => unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + }, + } + + match bar { + Some(v) => unsafe { + let r = &v as *const i32; + println!("{}", *r); + }, + None => unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + }, + } + + match bar { + Some(v) => { + // this comment prevents rustfmt from collapsing the block + unsafe { + let r = &v as *const i32; + println!("{}", *r); + } + }, + None => { + println!("None"); + println!("None"); + }, + } + + match bar { + Some(v) => { + println!("Some"); + println!("{v}"); + }, + None => { + // this comment prevents rustfmt from collapsing the block + unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + } + }, + } + + match bar { + Some(v) => { + // this comment prevents rustfmt from collapsing the block + unsafe { + let r = &v as *const i32; + println!("{}", *r); + } + }, + None => { + // this comment prevents rustfmt from collapsing the block + unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + } + }, + } +} From e92614818879f24994c3890dcb958b8baa2d0678 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Mon, 22 May 2023 04:07:17 -0400 Subject: [PATCH 2/5] Fix unsafe blocks --- clippy_utils/src/source.rs | 14 +++-- tests/ui/single_match.stderr | 45 +++++++++++++- tests/ui/single_match_else.stderr | 99 ++++++++++++++++++++++++++++++- 3 files changed, 152 insertions(+), 6 deletions(-) diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 0f60290644a1..61e249b995e6 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -4,7 +4,7 @@ use rustc_data_structures::sync::Lrc; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{BlockCheckMode, Expr, ExprKind, UnsafeSource}; use rustc_lint::{LateContext, LintContext}; use rustc_session::Session; use rustc_span::source_map::{original_sp, SourceMap}; @@ -71,11 +71,17 @@ pub fn expr_block( app: &mut Applicability, ) -> String { let (code, from_macro) = snippet_block_with_context(cx, expr.span, outer, default, indent_relative_to, app); - if from_macro { - format!("{{ {code} }}") - } else if let ExprKind::Block(_, _) = expr.kind { + if !from_macro && + let ExprKind::Block(block, _) = expr.kind && + // TODO: Is this enough UnsafeSource::UserProvided, or should CompilerGenerated be also included? + block.rules != BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) + { format!("{code}") } else { + // FIXME: add extra indent for the unsafe blocks: + // original code: unsafe { ... } + // result code: { unsafe { ... } } + // desired code: {\n unsafe { ... }\n} format!("{{ {code} }}") } } diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index 7cecc1b73950..810f153fe621 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -155,5 +155,48 @@ LL | | (..) => {}, LL | | } | |_____^ help: try this: `if let (.., Some(E::V), _) = (Some(42), Some(E::V), Some(42)) {}` -error: aborting due to 16 previous errors +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:249:5 + | +LL | / match bar { +LL | | Some(v) => unsafe { +LL | | let r = &v as *const i32; +LL | | println!("{}", *r); +LL | | }, +LL | | _ => {}, +LL | | } + | |_____^ + | +help: try this + | +LL ~ if let Some(v) = bar { unsafe { +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } } + | + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match.rs:257:5 + | +LL | / match bar { +LL | | Some(v) => { +LL | | // this comment prevents rustfmt from collapsing the block +LL | | unsafe { +... | +LL | | _ => {}, +LL | | } + | |_____^ + | +help: try this + | +LL ~ if let Some(v) = bar { +LL + // this comment prevents rustfmt from collapsing the block +LL + unsafe { +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } +LL + } + | + +error: aborting due to 18 previous errors diff --git a/tests/ui/single_match_else.stderr b/tests/ui/single_match_else.stderr index 62876a55dc61..6537156d5158 100644 --- a/tests/ui/single_match_else.stderr +++ b/tests/ui/single_match_else.stderr @@ -100,5 +100,102 @@ LL + return; LL + } | -error: aborting due to 5 previous errors +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match_else.rs:120:5 + | +LL | / match bar { +LL | | Some(v) => unsafe { +LL | | let r = &v as *const i32; +LL | | println!("{}", *r); +... | +LL | | }, +LL | | } + | |_____^ + | +help: try this + | +LL ~ if let Some(v) = bar { unsafe { +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } } else { +LL + println!("None1"); +LL + println!("None2"); +LL + } + | + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match_else.rs:131:5 + | +LL | / match bar { +LL | | Some(v) => { +LL | | println!("Some"); +LL | | println!("{v}"); +... | +LL | | }, +LL | | } + | |_____^ + | +help: try this + | +LL ~ if let Some(v) = bar { +LL + println!("Some"); +LL + println!("{v}"); +LL + } else { unsafe { +LL + let v = 0; +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } } + | + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match_else.rs:143:5 + | +LL | / match bar { +LL | | Some(v) => unsafe { +LL | | let r = &v as *const i32; +LL | | println!("{}", *r); +... | +LL | | }, +LL | | } + | |_____^ + | +help: try this + | +LL ~ if let Some(v) = bar { unsafe { +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } } else { unsafe { +LL + let v = 0; +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } } + | + +error: you seem to be trying to use `match` for destructuring a single pattern. Consider using `if let` + --> $DIR/single_match_else.rs:155:5 + | +LL | / match bar { +LL | | Some(v) => { +LL | | // this comment prevents rustfmt from collapsing the block +LL | | unsafe { +... | +LL | | }, +LL | | } + | |_____^ + | +help: try this + | +LL ~ if let Some(v) = bar { +LL + // this comment prevents rustfmt from collapsing the block +LL + unsafe { +LL + let r = &v as *const i32; +LL + println!("{}", *r); +LL + } +LL + } else { +LL + println!("None"); +LL + println!("None"); +LL + } + | + +error: aborting due to 9 previous errors From 9fd34e0c7522fd52bf7d015de4ed78c24596498b Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Mon, 22 May 2023 20:02:45 -0400 Subject: [PATCH 3/5] Use #[rustfmt::skip] --- tests/ui/single_match.rs | 2 +- tests/ui/single_match.stderr | 3 +-- tests/ui/single_match_else.rs | 8 ++++---- tests/ui/single_match_else.stderr | 3 +-- 4 files changed, 7 insertions(+), 9 deletions(-) diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index d474088aab66..3455ca5537af 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -255,8 +255,8 @@ fn issue_10808(bar: Option) { } match bar { + #[rustfmt::skip] Some(v) => { - // this comment prevents rustfmt from collapsing the block unsafe { let r = &v as *const i32; println!("{}", *r); diff --git a/tests/ui/single_match.stderr b/tests/ui/single_match.stderr index 810f153fe621..dad66e2ab2ec 100644 --- a/tests/ui/single_match.stderr +++ b/tests/ui/single_match.stderr @@ -179,8 +179,8 @@ error: you seem to be trying to use `match` for destructuring a single pattern. --> $DIR/single_match.rs:257:5 | LL | / match bar { +LL | | #[rustfmt::skip] LL | | Some(v) => { -LL | | // this comment prevents rustfmt from collapsing the block LL | | unsafe { ... | LL | | _ => {}, @@ -190,7 +190,6 @@ LL | | } help: try this | LL ~ if let Some(v) = bar { -LL + // this comment prevents rustfmt from collapsing the block LL + unsafe { LL + let r = &v as *const i32; LL + println!("{}", *r); diff --git a/tests/ui/single_match_else.rs b/tests/ui/single_match_else.rs index 768316edad53..ec97bfc845f8 100644 --- a/tests/ui/single_match_else.rs +++ b/tests/ui/single_match_else.rs @@ -153,8 +153,8 @@ fn issue_10808(bar: Option) { } match bar { + #[rustfmt::skip] Some(v) => { - // this comment prevents rustfmt from collapsing the block unsafe { let r = &v as *const i32; println!("{}", *r); @@ -171,8 +171,8 @@ fn issue_10808(bar: Option) { println!("Some"); println!("{v}"); }, + #[rustfmt::skip] None => { - // this comment prevents rustfmt from collapsing the block unsafe { let v = 0; let r = &v as *const i32; @@ -182,15 +182,15 @@ fn issue_10808(bar: Option) { } match bar { + #[rustfmt::skip] Some(v) => { - // this comment prevents rustfmt from collapsing the block unsafe { let r = &v as *const i32; println!("{}", *r); } }, + #[rustfmt::skip] None => { - // this comment prevents rustfmt from collapsing the block unsafe { let v = 0; let r = &v as *const i32; diff --git a/tests/ui/single_match_else.stderr b/tests/ui/single_match_else.stderr index 6537156d5158..228236f3bb8e 100644 --- a/tests/ui/single_match_else.stderr +++ b/tests/ui/single_match_else.stderr @@ -175,8 +175,8 @@ error: you seem to be trying to use `match` for destructuring a single pattern. --> $DIR/single_match_else.rs:155:5 | LL | / match bar { +LL | | #[rustfmt::skip] LL | | Some(v) => { -LL | | // this comment prevents rustfmt from collapsing the block LL | | unsafe { ... | LL | | }, @@ -186,7 +186,6 @@ LL | | } help: try this | LL ~ if let Some(v) = bar { -LL + // this comment prevents rustfmt from collapsing the block LL + unsafe { LL + let r = &v as *const i32; LL + println!("{}", *r); From 68df61ebd9e0e6b75299185e545bd608369c4883 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Mon, 22 May 2023 20:06:58 -0400 Subject: [PATCH 4/5] remove todo --- clippy_utils/src/source.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs index 61e249b995e6..582337b47e81 100644 --- a/clippy_utils/src/source.rs +++ b/clippy_utils/src/source.rs @@ -73,7 +73,6 @@ pub fn expr_block( let (code, from_macro) = snippet_block_with_context(cx, expr.span, outer, default, indent_relative_to, app); if !from_macro && let ExprKind::Block(block, _) = expr.kind && - // TODO: Is this enough UnsafeSource::UserProvided, or should CompilerGenerated be also included? block.rules != BlockCheckMode::UnsafeBlock(UnsafeSource::UserProvided) { format!("{code}") From ed935de08761ef3d463b7770f9c5d8b7422bed82 Mon Sep 17 00:00:00 2001 From: Yuri Astrakhan Date: Mon, 22 May 2023 23:00:28 -0400 Subject: [PATCH 5/5] Run-rustfix --- tests/ui/single_match.fixed | 209 +++++++++++++++++++++++++++++++ tests/ui/single_match.rs | 4 +- tests/ui/single_match_else.fixed | 173 +++++++++++++++++++++++++ tests/ui/single_match_else.rs | 2 +- 4 files changed, 385 insertions(+), 3 deletions(-) create mode 100644 tests/ui/single_match.fixed create mode 100644 tests/ui/single_match_else.fixed diff --git a/tests/ui/single_match.fixed b/tests/ui/single_match.fixed new file mode 100644 index 000000000000..77a2cf3b991f --- /dev/null +++ b/tests/ui/single_match.fixed @@ -0,0 +1,209 @@ +//@run-rustfix +#![warn(clippy::single_match)] +#![allow(unused, clippy::uninlined_format_args, clippy::redundant_pattern_matching)] +fn dummy() {} + +fn single_match() { + let x = Some(1u8); + + if let Some(y) = x { + println!("{:?}", y); + }; + + let x = Some(1u8); + if let Some(y) = x { println!("{:?}", y) } + + let z = (1u8, 1u8); + if let (2..=3, 7..=9) = z { dummy() }; + + // Not linted (pattern guards used) + match x { + Some(y) if y == 0 => println!("{:?}", y), + _ => (), + } + + // Not linted (no block with statements in the single arm) + match z { + (2..=3, 7..=9) => println!("{:?}", z), + _ => println!("nope"), + } +} + +enum Foo { + Bar, + Baz(u8), +} +use std::borrow::Cow; +use Foo::*; + +fn single_match_know_enum() { + let x = Some(1u8); + let y: Result<_, i8> = Ok(1i8); + + if let Some(y) = x { dummy() }; + + if let Ok(y) = y { dummy() }; + + let c = Cow::Borrowed(""); + + if let Cow::Borrowed(..) = c { dummy() }; + + let z = Foo::Bar; + // no warning + match z { + Bar => println!("42"), + Baz(_) => (), + } + + match z { + Baz(_) => println!("42"), + Bar => (), + } +} + +// issue #173 +fn if_suggestion() { + let x = "test"; + if x == "test" { println!() } + + #[derive(PartialEq, Eq)] + enum Foo { + A, + B, + C(u32), + } + + let x = Foo::A; + if x == Foo::A { println!() } + + const FOO_C: Foo = Foo::C(0); + if x == FOO_C { println!() } + + if x == Foo::A { println!() } + + let x = &x; + if x == &Foo::A { println!() } + + enum Bar { + A, + B, + } + impl PartialEq for Bar { + fn eq(&self, rhs: &Self) -> bool { + matches!((self, rhs), (Self::A, Self::A) | (Self::B, Self::B)) + } + } + impl Eq for Bar {} + + let x = Bar::A; + if let Bar::A = x { println!() } + + // issue #7038 + struct X; + let x = Some(X); + if let None = x { println!() }; +} + +// See: issue #8282 +fn ranges() { + enum E { + V, + } + let x = (Some(E::V), Some(42)); + + // Don't lint, because the `E` enum can be extended with additional fields later. Thus, the + // proposed replacement to `if let Some(E::V)` may hide non-exhaustive warnings that appeared + // because of `match` construction. + match x { + (Some(E::V), _) => {}, + (None, _) => {}, + } + + // lint + if let (Some(_), _) = x {} + + // lint + if let (Some(E::V), _) = x { todo!() } + + // lint + if let (.., Some(E::V), _) = (Some(42), Some(E::V), Some(42)) {} + + // Don't lint, see above. + match (Some(E::V), Some(E::V), Some(E::V)) { + (.., Some(E::V), _) => {}, + (.., None, _) => {}, + } + + // Don't lint, see above. + match (Some(E::V), Some(E::V), Some(E::V)) { + (Some(E::V), ..) => {}, + (None, ..) => {}, + } + + // Don't lint, see above. + match (Some(E::V), Some(E::V), Some(E::V)) { + (_, Some(E::V), ..) => {}, + (_, None, ..) => {}, + } +} + +fn skip_type_aliases() { + enum OptionEx { + Some(i32), + None, + } + enum ResultEx { + Err(i32), + Ok(i32), + } + + use OptionEx::{None, Some}; + use ResultEx::{Err, Ok}; + + // don't lint + match Err(42) { + Ok(_) => dummy(), + Err(_) => (), + }; + + // don't lint + match Some(1i32) { + Some(_) => dummy(), + None => (), + }; +} + +macro_rules! single_match { + ($num:literal) => { + match $num { + 15 => println!("15"), + _ => (), + } + }; +} + +fn main() { + single_match!(5); + + // Don't lint + let _ = match Some(0) { + #[cfg(feature = "foo")] + Some(10) => 11, + Some(x) => x, + _ => 0, + }; +} + +fn issue_10808(bar: Option) { + if let Some(v) = bar { unsafe { + let r = &v as *const i32; + println!("{}", *r); + } } + + if let Some(v) = bar { + unsafe { + let r = &v as *const i32; + println!("{}", *r); + } + } +} diff --git a/tests/ui/single_match.rs b/tests/ui/single_match.rs index 3455ca5537af..8d0ab5b99ad5 100644 --- a/tests/ui/single_match.rs +++ b/tests/ui/single_match.rs @@ -1,6 +1,6 @@ +//@run-rustfix #![warn(clippy::single_match)] -#![allow(unused, clippy::uninlined_format_args)] - +#![allow(unused, clippy::uninlined_format_args, clippy::redundant_pattern_matching)] fn dummy() {} fn single_match() { diff --git a/tests/ui/single_match_else.fixed b/tests/ui/single_match_else.fixed new file mode 100644 index 000000000000..f88498655a41 --- /dev/null +++ b/tests/ui/single_match_else.fixed @@ -0,0 +1,173 @@ +//@run-rustfix +//@aux-build: proc_macros.rs +#![warn(clippy::single_match_else)] +#![allow(unused, clippy::needless_return, clippy::no_effect, clippy::uninlined_format_args)] +extern crate proc_macros; +use proc_macros::with_span; + +enum ExprNode { + ExprAddrOf, + Butterflies, + Unicorns, +} + +static NODE: ExprNode = ExprNode::Unicorns; + +fn unwrap_addr() -> Option<&'static ExprNode> { + let _ = if let ExprNode::ExprAddrOf = ExprNode::Butterflies { Some(&NODE) } else { + let x = 5; + None + }; + + // Don't lint + with_span!(span match ExprNode::Butterflies { + ExprNode::ExprAddrOf => Some(&NODE), + _ => { + let x = 5; + None + }, + }) +} + +macro_rules! unwrap_addr { + ($expression:expr) => { + match $expression { + ExprNode::ExprAddrOf => Some(&NODE), + _ => { + let x = 5; + None + }, + } + }; +} + +#[rustfmt::skip] +fn main() { + unwrap_addr!(ExprNode::Unicorns); + + // + // don't lint single exprs/statements + // + + // don't lint here + match Some(1) { + Some(a) => println!("${:?}", a), + None => return, + } + + // don't lint here + match Some(1) { + Some(a) => println!("${:?}", a), + None => { + return + }, + } + + // don't lint here + match Some(1) { + Some(a) => println!("${:?}", a), + None => { + return; + }, + } + + // + // lint multiple exprs/statements "else" blocks + // + + // lint here + if let Some(a) = Some(1) { println!("${:?}", a) } else { + println!("else block"); + return + } + + // lint here + if let Some(a) = Some(1) { println!("${:?}", a) } else { + println!("else block"); + return; + } + + // lint here + use std::convert::Infallible; + if let Ok(a) = Result::::Ok(1) { println!("${:?}", a) } else { + println!("else block"); + return; + } + + use std::borrow::Cow; + if let Cow::Owned(a) = Cow::from("moo") { println!("${:?}", a) } else { + println!("else block"); + return; + } +} + +fn issue_10808(bar: Option) { + if let Some(v) = bar { unsafe { + let r = &v as *const i32; + println!("{}", *r); + } } else { + println!("None1"); + println!("None2"); + } + + if let Some(v) = bar { + println!("Some"); + println!("{v}"); + } else { unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + } } + + if let Some(v) = bar { unsafe { + let r = &v as *const i32; + println!("{}", *r); + } } else { unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + } } + + if let Some(v) = bar { + unsafe { + let r = &v as *const i32; + println!("{}", *r); + } + } else { + println!("None"); + println!("None"); + } + + match bar { + Some(v) => { + println!("Some"); + println!("{v}"); + }, + #[rustfmt::skip] + None => { + unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + } + }, + } + + match bar { + #[rustfmt::skip] + Some(v) => { + unsafe { + let r = &v as *const i32; + println!("{}", *r); + } + }, + #[rustfmt::skip] + None => { + unsafe { + let v = 0; + let r = &v as *const i32; + println!("{}", *r); + } + }, + } +} diff --git a/tests/ui/single_match_else.rs b/tests/ui/single_match_else.rs index ec97bfc845f8..b34b95539190 100644 --- a/tests/ui/single_match_else.rs +++ b/tests/ui/single_match_else.rs @@ -1,7 +1,7 @@ +//@run-rustfix //@aux-build: proc_macros.rs #![warn(clippy::single_match_else)] #![allow(unused, clippy::needless_return, clippy::no_effect, clippy::uninlined_format_args)] - extern crate proc_macros; use proc_macros::with_span;