From 78b7e8544bc961be55e896bbd6e42b1697963638 Mon Sep 17 00:00:00 2001 From: Philipp Hansch Date: Fri, 1 Nov 2019 20:12:08 +0100 Subject: [PATCH 01/19] Fix false positive in derive_hash_xor_eq This fixes a false positive in derive_hash_xor_eq where the lint was triggering on user-defined traits called `Hash`. --- clippy_lints/src/derive.rs | 1 + tests/ui/derive_hash_xor_eq.rs | 25 +++++++++++++++++++---- tests/ui/derive_hash_xor_eq.stderr | 32 +++++++++++++++++++++--------- 3 files changed, 45 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/derive.rs b/clippy_lints/src/derive.rs index fa981ce78d9..f60ed90cd7c 100644 --- a/clippy_lints/src/derive.rs +++ b/clippy_lints/src/derive.rs @@ -90,6 +90,7 @@ fn check_hash_peq<'a, 'tcx>( if_chain! { if match_path(&trait_ref.path, &paths::HASH); if let Some(peq_trait_def_id) = cx.tcx.lang_items().eq_trait(); + if !&trait_ref.trait_def_id().is_local(); then { // Look for the PartialEq implementations for `ty` cx.tcx.for_each_relevant_impl(peq_trait_def_id, ty, |impl_id| { diff --git a/tests/ui/derive_hash_xor_eq.rs b/tests/ui/derive_hash_xor_eq.rs index c0be787f5e4..10abe22d53e 100644 --- a/tests/ui/derive_hash_xor_eq.rs +++ b/tests/ui/derive_hash_xor_eq.rs @@ -1,5 +1,3 @@ -use std::hash::{Hash, Hasher}; - #[derive(PartialEq, Hash)] struct Foo; @@ -30,8 +28,27 @@ impl PartialEq for Baz { #[derive(PartialEq)] struct Bah; -impl Hash for Bah { - fn hash(&self, _: &mut H) {} +impl std::hash::Hash for Bah { + fn hash(&self, _: &mut H) {} +} + +#[derive(PartialEq)] +struct Foo2; + +trait Hash {} + +// We don't want to lint on user-defined traits called `Hash` +impl Hash for Foo2 {} + +mod use_hash { + use std::hash::{Hash, Hasher}; + + #[derive(PartialEq)] + struct Foo3; + + impl Hash for Foo3 { + fn hash(&self, _: &mut H) {} + } } fn main() {} diff --git a/tests/ui/derive_hash_xor_eq.stderr b/tests/ui/derive_hash_xor_eq.stderr index c3d451aaed6..10c38bb42e3 100644 --- a/tests/ui/derive_hash_xor_eq.stderr +++ b/tests/ui/derive_hash_xor_eq.stderr @@ -1,12 +1,12 @@ error: you are deriving `Hash` but have implemented `PartialEq` explicitly - --> $DIR/derive_hash_xor_eq.rs:12:10 + --> $DIR/derive_hash_xor_eq.rs:10:10 | LL | #[derive(Hash)] | ^^^^ | = note: `#[deny(clippy::derive_hash_xor_eq)]` on by default note: `PartialEq` implemented here - --> $DIR/derive_hash_xor_eq.rs:15:1 + --> $DIR/derive_hash_xor_eq.rs:13:1 | LL | / impl PartialEq for Bar { LL | | fn eq(&self, _: &Bar) -> bool { @@ -16,13 +16,13 @@ LL | | } | |_^ error: you are deriving `Hash` but have implemented `PartialEq` explicitly - --> $DIR/derive_hash_xor_eq.rs:21:10 + --> $DIR/derive_hash_xor_eq.rs:19:10 | LL | #[derive(Hash)] | ^^^^ | note: `PartialEq` implemented here - --> $DIR/derive_hash_xor_eq.rs:24:1 + --> $DIR/derive_hash_xor_eq.rs:22:1 | LL | / impl PartialEq for Baz { LL | | fn eq(&self, _: &Baz) -> bool { @@ -32,18 +32,32 @@ LL | | } | |_^ error: you are implementing `Hash` explicitly but have derived `PartialEq` - --> $DIR/derive_hash_xor_eq.rs:33:1 + --> $DIR/derive_hash_xor_eq.rs:31:1 | -LL | / impl Hash for Bah { -LL | | fn hash(&self, _: &mut H) {} +LL | / impl std::hash::Hash for Bah { +LL | | fn hash(&self, _: &mut H) {} LL | | } | |_^ | note: `PartialEq` implemented here - --> $DIR/derive_hash_xor_eq.rs:30:10 + --> $DIR/derive_hash_xor_eq.rs:28:10 | LL | #[derive(PartialEq)] | ^^^^^^^^^ -error: aborting due to 3 previous errors +error: you are implementing `Hash` explicitly but have derived `PartialEq` + --> $DIR/derive_hash_xor_eq.rs:49:5 + | +LL | / impl Hash for Foo3 { +LL | | fn hash(&self, _: &mut H) {} +LL | | } + | |_____^ + | +note: `PartialEq` implemented here + --> $DIR/derive_hash_xor_eq.rs:46:14 + | +LL | #[derive(PartialEq)] + | ^^^^^^^^^ + +error: aborting due to 4 previous errors From 8f5b4f3f5ca62ddb79a0337035919ef488acc9d4 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:26:52 +0200 Subject: [PATCH 02/19] literal representation restructure 1 Combine macro expansion checks. Indentation is a little strange to avoid rustfmt issue. --- clippy_lints/src/literal_representation.rs | 98 +++++++++++----------- 1 file changed, 47 insertions(+), 51 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 8ddd54bb7a3..8b978338922 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -358,67 +358,63 @@ impl EarlyLintPass for LiteralDigitGrouping { impl LiteralDigitGrouping { fn check_lit(cx: &EarlyContext<'_>, lit: &Lit) { let in_macro = in_macro(lit.span); + + if_chain! { + if let Some(src) = snippet_opt(cx, lit.span); + if let Some(firstch) = src.chars().next(); + if char::is_digit(firstch, 10); + then { + + match lit.kind { LitKind::Int(..) => { // Lint integral literals. - if_chain! { - if let Some(src) = snippet_opt(cx, lit.span); - if let Some(firstch) = src.chars().next(); - if char::is_digit(firstch, 10); - then { - let digit_info = DigitInfo::new(&src, false); - let _ = Self::do_lint(digit_info.digits, digit_info.suffix, in_macro).map_err(|warning_type| { - warning_type.display(&digit_info.grouping_hint(), cx, lit.span) - }); - } - } + let digit_info = DigitInfo::new(&src, false); + let _ = Self::do_lint(digit_info.digits, digit_info.suffix, in_macro).map_err(|warning_type| { + warning_type.display(&digit_info.grouping_hint(), cx, lit.span) + }); }, LitKind::Float(..) => { // Lint floating-point literals. - if_chain! { - if let Some(src) = snippet_opt(cx, lit.span); - if let Some(firstch) = src.chars().next(); - if char::is_digit(firstch, 10); - then { - let digit_info = DigitInfo::new(&src, true); - // Separate digits into integral and fractional parts. - let parts: Vec<&str> = digit_info - .digits - .split_terminator('.') - .collect(); + let digit_info = DigitInfo::new(&src, true); + // Separate digits into integral and fractional parts. + let parts: Vec<&str> = digit_info + .digits + .split_terminator('.') + .collect(); - // Lint integral and fractional parts separately, and then check consistency of digit - // groups if both pass. - let _ = Self::do_lint(parts[0], digit_info.suffix, in_macro) - .map(|integral_group_size| { - if parts.len() > 1 { - // Lint the fractional part of literal just like integral part, but reversed. - let fractional_part = &parts[1].chars().rev().collect::(); - let _ = Self::do_lint(fractional_part, None, in_macro) - .map(|fractional_group_size| { - let consistent = Self::parts_consistent(integral_group_size, - fractional_group_size, - parts[0].len(), - parts[1].len()); - if !consistent { - WarningType::InconsistentDigitGrouping.display( - &digit_info.grouping_hint(), - cx, - lit.span, - ); - } - }) - .map_err(|warning_type| warning_type.display(&digit_info.grouping_hint(), - cx, - lit.span)); - } - }) - .map_err(|warning_type| warning_type.display(&digit_info.grouping_hint(), cx, lit.span)); - } - } + // Lint integral and fractional parts separately, and then check consistency of digit + // groups if both pass. + let _ = Self::do_lint(parts[0], digit_info.suffix, in_macro) + .map(|integral_group_size| { + if parts.len() > 1 { + // Lint the fractional part of literal just like integral part, but reversed. + let fractional_part = &parts[1].chars().rev().collect::(); + let _ = Self::do_lint(fractional_part, None, in_macro) + .map(|fractional_group_size| { + let consistent = Self::parts_consistent(integral_group_size, + fractional_group_size, + parts[0].len(), + parts[1].len()); + if !consistent { + WarningType::InconsistentDigitGrouping.display( + &digit_info.grouping_hint(), + cx, + lit.span, + ); + } + }) + .map_err(|warning_type| warning_type.display(&digit_info.grouping_hint(), + cx, + lit.span)); + } + }) + .map_err(|warning_type| warning_type.display(&digit_info.grouping_hint(), cx, lit.span)); }, _ => (), } + } + } } /// Given the sizes of the digit groups of both integral and fractional From 2dbd34ffe814e92294992470ac74c57e33372407 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:05 +0200 Subject: [PATCH 03/19] literal representation restructure 2 Consolidate warning handling using "poor man's try". --- clippy_lints/src/literal_representation.rs | 88 +++++++++++----------- 1 file changed, 42 insertions(+), 46 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 8b978338922..6ea962f0ac3 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -365,54 +365,50 @@ impl LiteralDigitGrouping { if char::is_digit(firstch, 10); then { + let digit_info = match lit.kind { + LitKind::Int(..) => DigitInfo::new(&src, false), + LitKind::Float(..) => DigitInfo::new(&src, true), + _ => return, + }; - match lit.kind { - LitKind::Int(..) => { - // Lint integral literals. - let digit_info = DigitInfo::new(&src, false); - let _ = Self::do_lint(digit_info.digits, digit_info.suffix, in_macro).map_err(|warning_type| { + let result = (|| { + match lit.kind { + LitKind::Int(..) => { + Self::do_lint(digit_info.digits, digit_info.suffix, in_macro)?; + }, + LitKind::Float(..) => { + // Separate digits into integral and fractional parts. + let parts: Vec<&str> = digit_info + .digits + .split_terminator('.') + .collect(); + + // Lint integral and fractional parts separately, and then check consistency of digit + // groups if both pass. + let integral_group_size = Self::do_lint(parts[0], digit_info.suffix, in_macro)?; + if parts.len() > 1 { + // Lint the fractional part of literal just like integral part, but reversed. + let fractional_part = &parts[1].chars().rev().collect::(); + let fractional_group_size = Self::do_lint(fractional_part, None, in_macro)?; + let consistent = Self::parts_consistent(integral_group_size, + fractional_group_size, + parts[0].len(), + parts[1].len()); + if !consistent { + return Err(WarningType::InconsistentDigitGrouping); + }; + }; + }, + _ => (), + } + + Ok(()) + })(); + + + if let Err(warning_type) = result { warning_type.display(&digit_info.grouping_hint(), cx, lit.span) - }); - }, - LitKind::Float(..) => { - // Lint floating-point literals. - let digit_info = DigitInfo::new(&src, true); - // Separate digits into integral and fractional parts. - let parts: Vec<&str> = digit_info - .digits - .split_terminator('.') - .collect(); - - // Lint integral and fractional parts separately, and then check consistency of digit - // groups if both pass. - let _ = Self::do_lint(parts[0], digit_info.suffix, in_macro) - .map(|integral_group_size| { - if parts.len() > 1 { - // Lint the fractional part of literal just like integral part, but reversed. - let fractional_part = &parts[1].chars().rev().collect::(); - let _ = Self::do_lint(fractional_part, None, in_macro) - .map(|fractional_group_size| { - let consistent = Self::parts_consistent(integral_group_size, - fractional_group_size, - parts[0].len(), - parts[1].len()); - if !consistent { - WarningType::InconsistentDigitGrouping.display( - &digit_info.grouping_hint(), - cx, - lit.span, - ); - } - }) - .map_err(|warning_type| warning_type.display(&digit_info.grouping_hint(), - cx, - lit.span)); - } - }) - .map_err(|warning_type| warning_type.display(&digit_info.grouping_hint(), cx, lit.span)); - }, - _ => (), - } + } } } } From 2e8946a6de9dba222e1af8419bbb6f739b63eeea Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:14 +0200 Subject: [PATCH 04/19] literal representation restructure 3 Move suffix check into `check_lit` so that it isn't done repeatedly. --- clippy_lints/src/literal_representation.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 6ea962f0ac3..5b234948f24 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -372,9 +372,15 @@ impl LiteralDigitGrouping { }; let result = (|| { + if let Some(suffix) = digit_info.suffix { + if is_mistyped_suffix(suffix) { + return Err(WarningType::MistypedLiteralSuffix); + } + } + match lit.kind { LitKind::Int(..) => { - Self::do_lint(digit_info.digits, digit_info.suffix, in_macro)?; + Self::do_lint(digit_info.digits, in_macro)?; }, LitKind::Float(..) => { // Separate digits into integral and fractional parts. @@ -385,11 +391,11 @@ impl LiteralDigitGrouping { // Lint integral and fractional parts separately, and then check consistency of digit // groups if both pass. - let integral_group_size = Self::do_lint(parts[0], digit_info.suffix, in_macro)?; + let integral_group_size = Self::do_lint(parts[0], in_macro)?; if parts.len() > 1 { // Lint the fractional part of literal just like integral part, but reversed. let fractional_part = &parts[1].chars().rev().collect::(); - let fractional_group_size = Self::do_lint(fractional_part, None, in_macro)?; + let fractional_group_size = Self::do_lint(fractional_part, in_macro)?; let consistent = Self::parts_consistent(integral_group_size, fractional_group_size, parts[0].len(), @@ -432,12 +438,7 @@ impl LiteralDigitGrouping { /// Performs lint on `digits` (no decimal point) and returns the group /// size on success or `WarningType` when emitting a warning. - fn do_lint(digits: &str, suffix: Option<&str>, in_macro: bool) -> Result { - if let Some(suffix) = suffix { - if is_mistyped_suffix(suffix) { - return Err(WarningType::MistypedLiteralSuffix); - } - } + fn do_lint(digits: &str, in_macro: bool) -> Result { // Grab underscore indices with respect to the units digit. let underscore_positions: Vec = digits .chars() From 2d244d33588157257b4297b74276b5bc9b779a5f Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:19 +0200 Subject: [PATCH 05/19] literal representation restructure 4 Simplify `grouping_hint` by splitting digits into parts and handling one at a time. Fixes #4762 --- clippy_lints/src/literal_representation.rs | 166 +++++++++++--------- tests/ui/inconsistent_digit_grouping.fixed | 6 + tests/ui/inconsistent_digit_grouping.rs | 6 + tests/ui/inconsistent_digit_grouping.stderr | 28 +++- tests/ui/mistyped_literal_suffix.fixed | 2 + tests/ui/mistyped_literal_suffix.rs | 2 + tests/ui/mistyped_literal_suffix.stderr | 8 +- 7 files changed, 144 insertions(+), 74 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 5b234948f24..fe1ade60ca0 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -190,26 +190,70 @@ impl<'a> DigitInfo<'a> { } } + fn split_digit_parts(&self) -> (&str, Option<&str>, Option<(char, &str)>) { + let digits = self.digits; + + let mut integer = digits; + let mut fraction = None; + let mut exponent = None; + + if self.float { + for (i, c) in digits.char_indices() { + match c { + '.' => { + integer = &digits[..i]; + fraction = Some(&digits[i + 1..]); + }, + 'e' | 'E' => { + if integer.len() > i { + integer = &digits[..i]; + } else { + fraction = Some(&digits[integer.len() + 1..i]); + }; + exponent = Some((c, &digits[i + 1..])); + break; + }, + _ => {}, + } + } + } + + (integer, fraction, exponent) + } + /// Returns literal formatted in a sensible way. crate fn grouping_hint(&self) -> String { + let mut output = String::new(); + + if let Some(prefix) = self.prefix { + output.push_str(prefix); + } + let group_size = self.radix.suggest_grouping(); - if self.digits.contains('.') { - let mut parts = self.digits.split('.'); - let int_part_hint = parts - .next() - .expect("split always returns at least one element") - .chars() - .rev() - .filter(|&c| c != '_') - .collect::>() - .chunks(group_size) - .map(|chunk| chunk.iter().rev().collect()) - .rev() - .collect::>() - .join("_"); - let frac_part_hint = parts - .next() - .expect("already checked that there is a `.`") + + let (integer, fraction, exponent) = &self.split_digit_parts(); + + let int_digits: Vec<_> = integer.chars().rev().filter(|&c| c != '_').collect(); + let int_part_hint = int_digits + .chunks(group_size) + .map(|chunk| chunk.iter().rev().collect()) + .rev() + .collect::>() + .join("_"); + + // Pad leading hexidecimal group with zeros + if self.radix == Radix::Hexadecimal { + debug_assert!(group_size > 0); + let first_group_size = (int_digits.len() + group_size - 1) % group_size + 1; + for _ in 0..group_size - first_group_size { + output.push('0'); + } + } + + output.push_str(&int_part_hint); + + if let Some(fraction) = fraction { + let frac_part_hint = fraction .chars() .filter(|&c| c != '_') .collect::>() @@ -217,62 +261,40 @@ impl<'a> DigitInfo<'a> { .map(|chunk| chunk.iter().collect()) .collect::>() .join("_"); - let suffix_hint = match self.suffix { - Some(suffix) if is_mistyped_float_suffix(suffix) => format!("_f{}", &suffix[1..]), - Some(suffix) => suffix.to_string(), - None => String::new(), - }; - format!("{}.{}{}", int_part_hint, frac_part_hint, suffix_hint) - } else if self.float && (self.digits.contains('E') || self.digits.contains('e')) { - let which_e = if self.digits.contains('E') { 'E' } else { 'e' }; - let parts: Vec<&str> = self.digits.split(which_e).collect(); - let filtered_digits_vec_0 = parts[0].chars().filter(|&c| c != '_').rev().collect::>(); - let filtered_digits_vec_1 = parts[1].chars().filter(|&c| c != '_').rev().collect::>(); - let before_e_hint = filtered_digits_vec_0 - .chunks(group_size) - .map(|chunk| chunk.iter().rev().collect()) - .rev() - .collect::>() - .join("_"); - let after_e_hint = filtered_digits_vec_1 - .chunks(group_size) - .map(|chunk| chunk.iter().rev().collect()) - .rev() - .collect::>() - .join("_"); - let suffix_hint = match self.suffix { - Some(suffix) if is_mistyped_float_suffix(suffix) => format!("_f{}", &suffix[1..]), - Some(suffix) => suffix.to_string(), - None => String::new(), - }; - format!( - "{}{}{}{}{}", - self.prefix.unwrap_or(""), - before_e_hint, - which_e, - after_e_hint, - suffix_hint - ) - } else { - let filtered_digits_vec = self.digits.chars().filter(|&c| c != '_').rev().collect::>(); - let mut hint = filtered_digits_vec - .chunks(group_size) - .map(|chunk| chunk.iter().rev().collect()) - .rev() - .collect::>() - .join("_"); - // Forces hexadecimal values to be grouped by 4 being filled with zeroes (e.g 0x00ab_cdef) - let nb_digits_to_fill = filtered_digits_vec.len() % 4; - if self.radix == Radix::Hexadecimal && nb_digits_to_fill != 0 { - hint = format!("{:0>4}{}", &hint[..nb_digits_to_fill], &hint[nb_digits_to_fill..]); - } - let suffix_hint = match self.suffix { - Some(suffix) if is_mistyped_suffix(suffix) => format!("_i{}", &suffix[1..]), - Some(suffix) => suffix.to_string(), - None => String::new(), - }; - format!("{}{}{}", self.prefix.unwrap_or(""), hint, suffix_hint) + + output.push('.'); + output.push_str(&frac_part_hint); } + + if let Some((separator, exponent)) = exponent { + let after_e_hint = exponent + .chars() + .rev() + .filter(|&c| c != '_') + .collect::>() + .chunks(group_size) + .map(|chunk| chunk.iter().rev().collect()) + .rev() + .collect::>() + .join("_"); + + output.push(*separator); + output.push_str(&after_e_hint); + } + + if let Some(suffix) = self.suffix { + if self.float && is_mistyped_float_suffix(suffix) { + output.push_str("_f"); + output.push_str(&suffix[1..]); + } else if is_mistyped_suffix(suffix) { + output.push_str("_i"); + output.push_str(&suffix[1..]); + } else { + output.push_str(suffix); + } + } + + output } } diff --git a/tests/ui/inconsistent_digit_grouping.fixed b/tests/ui/inconsistent_digit_grouping.fixed index f25be70737b..a95b7e0392d 100644 --- a/tests/ui/inconsistent_digit_grouping.fixed +++ b/tests/ui/inconsistent_digit_grouping.fixed @@ -12,4 +12,10 @@ fn main() { 1.123_456_7_f32, ); let bad = (123_456, 12_345_678, 1_234_567, 1_234.567_8_f32, 1.234_567_8_f32); + + // Test padding + let _ = 0x0010_0000; + let _ = 0x0100_0000; + let _ = 0x1000_0000; + let _ = 0x0001_0000_0000_u64; } diff --git a/tests/ui/inconsistent_digit_grouping.rs b/tests/ui/inconsistent_digit_grouping.rs index 206fac8d3e3..e316e140de8 100644 --- a/tests/ui/inconsistent_digit_grouping.rs +++ b/tests/ui/inconsistent_digit_grouping.rs @@ -12,4 +12,10 @@ fn main() { 1.123_456_7_f32, ); let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); + + // Test padding + let _ = 0x100000; + let _ = 0x1000000; + let _ = 0x10000000; + let _ = 0x100000000_u64; } diff --git a/tests/ui/inconsistent_digit_grouping.stderr b/tests/ui/inconsistent_digit_grouping.stderr index 9fc1f424dc6..31acea2c5e0 100644 --- a/tests/ui/inconsistent_digit_grouping.stderr +++ b/tests/ui/inconsistent_digit_grouping.stderr @@ -30,5 +30,31 @@ error: digits grouped inconsistently by underscores LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); | ^^^^^^^^^^^^^^ help: consider: `1.234_567_8_f32` -error: aborting due to 5 previous errors +error: long literal lacking separators + --> $DIR/inconsistent_digit_grouping.rs:17:13 + | +LL | let _ = 0x100000; + | ^^^^^^^^ help: consider: `0x0010_0000` + | + = note: `-D clippy::unreadable-literal` implied by `-D warnings` + +error: long literal lacking separators + --> $DIR/inconsistent_digit_grouping.rs:18:13 + | +LL | let _ = 0x1000000; + | ^^^^^^^^^ help: consider: `0x0100_0000` + +error: long literal lacking separators + --> $DIR/inconsistent_digit_grouping.rs:19:13 + | +LL | let _ = 0x10000000; + | ^^^^^^^^^^ help: consider: `0x1000_0000` + +error: long literal lacking separators + --> $DIR/inconsistent_digit_grouping.rs:20:13 + | +LL | let _ = 0x100000000_u64; + | ^^^^^^^^^^^^^^^ help: consider: `0x0001_0000_0000_u64` + +error: aborting due to 9 previous errors diff --git a/tests/ui/mistyped_literal_suffix.fixed b/tests/ui/mistyped_literal_suffix.fixed index 531e44a781c..baee7735730 100644 --- a/tests/ui/mistyped_literal_suffix.fixed +++ b/tests/ui/mistyped_literal_suffix.fixed @@ -19,4 +19,6 @@ fn main() { #[allow(overflowing_literals)] let fail28 = 241_251_235E723_f64; let fail29 = 42_279.911_f32; + + let _ = 1.123_45E1_f32; } diff --git a/tests/ui/mistyped_literal_suffix.rs b/tests/ui/mistyped_literal_suffix.rs index d67c842b4af..6de447f4021 100644 --- a/tests/ui/mistyped_literal_suffix.rs +++ b/tests/ui/mistyped_literal_suffix.rs @@ -19,4 +19,6 @@ fn main() { #[allow(overflowing_literals)] let fail28 = 241251235E723_64; let fail29 = 42279.911_32; + + let _ = 1.12345E1_32; } diff --git a/tests/ui/mistyped_literal_suffix.stderr b/tests/ui/mistyped_literal_suffix.stderr index 17c8b323027..48a7ae90494 100644 --- a/tests/ui/mistyped_literal_suffix.stderr +++ b/tests/ui/mistyped_literal_suffix.stderr @@ -72,5 +72,11 @@ error: mistyped literal suffix LL | let fail29 = 42279.911_32; | ^^^^^^^^^^^^ help: did you mean to write: `42_279.911_f32` -error: aborting due to 12 previous errors +error: mistyped literal suffix + --> $DIR/mistyped_literal_suffix.rs:23:13 + | +LL | let _ = 1.12345E1_32; + | ^^^^^^^^^^^^ help: did you mean to write: `1.123_45E1_f32` + +error: aborting due to 13 previous errors From ec664e84bfe67e0835ec0bb04e63a1210007b598 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:27 +0200 Subject: [PATCH 06/19] literal representation restructure 5 Use `split_digit_parts` in `check_lit`. --- clippy_lints/src/literal_representation.rs | 41 +++++++--------------- 1 file changed, 13 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index fe1ade60ca0..eacba87073a 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -400,36 +400,21 @@ impl LiteralDigitGrouping { } } - match lit.kind { - LitKind::Int(..) => { - Self::do_lint(digit_info.digits, in_macro)?; - }, - LitKind::Float(..) => { - // Separate digits into integral and fractional parts. - let parts: Vec<&str> = digit_info - .digits - .split_terminator('.') - .collect(); + let (integer, fraction, _) = digit_info.split_digit_parts(); - // Lint integral and fractional parts separately, and then check consistency of digit - // groups if both pass. - let integral_group_size = Self::do_lint(parts[0], in_macro)?; - if parts.len() > 1 { - // Lint the fractional part of literal just like integral part, but reversed. - let fractional_part = &parts[1].chars().rev().collect::(); - let fractional_group_size = Self::do_lint(fractional_part, in_macro)?; - let consistent = Self::parts_consistent(integral_group_size, - fractional_group_size, - parts[0].len(), - parts[1].len()); - if !consistent { - return Err(WarningType::InconsistentDigitGrouping); - }; - }; - }, - _ => (), + let integral_group_size = Self::do_lint(integer, in_macro)?; + if let Some(fraction) = fraction { + let fractional_part = fraction.chars().rev().collect::(); + let fractional_group_size = Self::do_lint(&fractional_part, in_macro)?; + + let consistent = Self::parts_consistent(integral_group_size, + fractional_group_size, + integer.len(), + fraction.len()); + if !consistent { + return Err(WarningType::InconsistentDigitGrouping); + }; } - Ok(()) })(); From abf62d8011cb8a4c66f6f9b4fd156d04fcfeb8f2 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:37 +0200 Subject: [PATCH 07/19] literal representation restructure 6 Add `group_digits` helper function. --- clippy_lints/src/literal_representation.rs | 75 ++++++++++------------ 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index eacba87073a..bc23255da09 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -233,53 +233,16 @@ impl<'a> DigitInfo<'a> { let (integer, fraction, exponent) = &self.split_digit_parts(); - let int_digits: Vec<_> = integer.chars().rev().filter(|&c| c != '_').collect(); - let int_part_hint = int_digits - .chunks(group_size) - .map(|chunk| chunk.iter().rev().collect()) - .rev() - .collect::>() - .join("_"); - - // Pad leading hexidecimal group with zeros - if self.radix == Radix::Hexadecimal { - debug_assert!(group_size > 0); - let first_group_size = (int_digits.len() + group_size - 1) % group_size + 1; - for _ in 0..group_size - first_group_size { - output.push('0'); - } - } - - output.push_str(&int_part_hint); + Self::group_digits(&mut output, integer, group_size, true, self.radix == Radix::Hexadecimal); if let Some(fraction) = fraction { - let frac_part_hint = fraction - .chars() - .filter(|&c| c != '_') - .collect::>() - .chunks(group_size) - .map(|chunk| chunk.iter().collect()) - .collect::>() - .join("_"); - output.push('.'); - output.push_str(&frac_part_hint); + Self::group_digits(&mut output, fraction, group_size, false, false); } if let Some((separator, exponent)) = exponent { - let after_e_hint = exponent - .chars() - .rev() - .filter(|&c| c != '_') - .collect::>() - .chunks(group_size) - .map(|chunk| chunk.iter().rev().collect()) - .rev() - .collect::>() - .join("_"); - output.push(*separator); - output.push_str(&after_e_hint); + Self::group_digits(&mut output, exponent, group_size, true, false); } if let Some(suffix) = self.suffix { @@ -296,6 +259,38 @@ impl<'a> DigitInfo<'a> { output } + + fn group_digits(output: &mut String, input: &str, group_size: usize, partial_group_first: bool, pad: bool) { + debug_assert!(group_size > 0); + + let mut digits = input.chars().filter(|&c| c != '_'); + + let first_group_size; + + if partial_group_first { + first_group_size = (digits.clone().count() + group_size - 1) % group_size + 1; + if pad { + for _ in 0..group_size - first_group_size { + output.push('0'); + } + } + } else { + first_group_size = group_size; + } + + for _ in 0..first_group_size { + if let Some(digit) = digits.next() { + output.push(digit); + } + } + + for (c, i) in digits.zip((0..group_size).cycle()) { + if i == 0 { + output.push('_'); + } + output.push(c); + } + } } enum WarningType { From b62543f756bb6219dcc1025e21f5e209349b6ed6 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:42 +0200 Subject: [PATCH 08/19] literal representation restructure 7 Replace `do_lint` with `get_group_size`. Return `None` if there are no groups. --- clippy_lints/src/literal_representation.rs | 65 +++++++++------------- 1 file changed, 27 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index bc23255da09..42a75d1e300 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -397,10 +397,9 @@ impl LiteralDigitGrouping { let (integer, fraction, _) = digit_info.split_digit_parts(); - let integral_group_size = Self::do_lint(integer, in_macro)?; + let integral_group_size = Self::get_group_size(integer.split('_'), in_macro)?; if let Some(fraction) = fraction { - let fractional_part = fraction.chars().rev().collect::(); - let fractional_group_size = Self::do_lint(&fractional_part, in_macro)?; + let fractional_group_size = Self::get_group_size(fraction.rsplit('_'), in_macro)?; let consistent = Self::parts_consistent(integral_group_size, fractional_group_size, @@ -425,53 +424,43 @@ impl LiteralDigitGrouping { /// parts, and the length /// of both parts, determine if the digits have been grouped consistently. #[must_use] - fn parts_consistent(int_group_size: usize, frac_group_size: usize, int_size: usize, frac_size: usize) -> bool { + fn parts_consistent( + int_group_size: Option, + frac_group_size: Option, + int_size: usize, + frac_size: usize, + ) -> bool { match (int_group_size, frac_group_size) { // No groups on either side of decimal point - trivially consistent. - (0, 0) => true, + (None, None) => true, // Integral part has grouped digits, fractional part does not. - (_, 0) => frac_size <= int_group_size, + (Some(int_group_size), None) => frac_size <= int_group_size, // Fractional part has grouped digits, integral part does not. - (0, _) => int_size <= frac_group_size, + (None, Some(frac_group_size)) => int_size <= frac_group_size, // Both parts have grouped digits. Groups should be the same size. - (_, _) => int_group_size == frac_group_size, + (Some(int_group_size), Some(frac_group_size)) => int_group_size == frac_group_size, } } - /// Performs lint on `digits` (no decimal point) and returns the group - /// size on success or `WarningType` when emitting a warning. - fn do_lint(digits: &str, in_macro: bool) -> Result { - // Grab underscore indices with respect to the units digit. - let underscore_positions: Vec = digits - .chars() - .rev() - .enumerate() - .filter_map(|(idx, digit)| if digit == '_' { Some(idx) } else { None }) - .collect(); + /// Returns the size of the digit groups (or None if ungrouped) if successful, + /// otherwise returns a `WarningType` for linting. + fn get_group_size<'a>(groups: impl Iterator, in_macro: bool) -> Result, WarningType> { + let mut groups = groups.map(str::len); - if underscore_positions.is_empty() { - // Check if literal needs underscores. - if !in_macro && digits.len() > 5 { - Err(WarningType::UnreadableLiteral) + let first = groups.next().expect("At least one group"); + + if let Some(second) = groups.next() { + if !groups.all(|x| x == second) || first > second { + Err(WarningType::InconsistentDigitGrouping) + } else if second > 4 { + Err(WarningType::LargeDigitGroups) } else { - Ok(0) + Ok(Some(second)) } + } else if first > 5 && !in_macro { + Err(WarningType::UnreadableLiteral) } else { - // Check consistency and the sizes of the groups. - let group_size = underscore_positions[0]; - let consistent = underscore_positions - .windows(2) - .all(|ps| ps[1] - ps[0] == group_size + 1) - // number of digits to the left of the last group cannot be bigger than group size. - && (digits.len() - underscore_positions.last() - .expect("there's at least one element") <= group_size + 1); - - if !consistent { - return Err(WarningType::InconsistentDigitGrouping); - } else if group_size > 4 { - return Err(WarningType::LargeDigitGroups); - } - Ok(group_size) + Ok(None) } } } From a58b980bd88651bc3bb85ef465848e0c08253444 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:49 +0200 Subject: [PATCH 09/19] literal representation restructure 8 Store the digit parts directly in DigitInfo since we need them anyway. --- clippy_lints/src/literal_representation.rs | 66 ++++++++++++---------- 1 file changed, 37 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 42a75d1e300..2fe993624f4 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -124,12 +124,18 @@ impl Radix { #[derive(Debug)] pub(super) struct DigitInfo<'a> { - /// Characters of a literal between the radix prefix and type suffix. - crate digits: &'a str, /// Which radix the literal was represented in. crate radix: Radix, /// The radix prefix, if present. crate prefix: Option<&'a str>, + + /// The integer part of the number. + integer: &'a str, + /// The fraction part of the number. + fraction: Option<&'a str>, + /// The character used as exponent seperator (b'e' or b'E') and the exponent part. + exponent: Option<(char, &'a str)>, + /// The type suffix, including preceding underscore if present. crate suffix: Option<&'a str>, /// True for floating-point literals. @@ -158,6 +164,9 @@ impl<'a> DigitInfo<'a> { (Some(p), s) }; + let mut digits = sans_prefix; + let mut suffix = None; + let len = sans_prefix.len(); let mut last_d = '\0'; for (d_idx, d) in sans_prefix.char_indices() { @@ -168,36 +177,33 @@ impl<'a> DigitInfo<'a> { || ((d == 'E' || d == 'e') && !has_possible_float_suffix(&sans_prefix))) || !float && (d == 'i' || d == 'u' || is_possible_suffix_index(&sans_prefix, suffix_start, len)) { - let (digits, suffix) = sans_prefix.split_at(suffix_start); - return Self { - digits, - radix, - prefix, - suffix: Some(suffix), - float, - }; + let (d, s) = sans_prefix.split_at(suffix_start); + digits = d; + suffix = Some(s); + break; } last_d = d } - // No suffix found + let (integer, fraction, exponent) = Self::split_digit_parts(digits, float); + Self { - digits: sans_prefix, radix, prefix, - suffix: None, + integer, + fraction, + exponent, + suffix, float, } } - fn split_digit_parts(&self) -> (&str, Option<&str>, Option<(char, &str)>) { - let digits = self.digits; - + fn split_digit_parts(digits: &str, float: bool) -> (&str, Option<&str>, Option<(char, &str)>) { let mut integer = digits; let mut fraction = None; let mut exponent = None; - if self.float { + if float { for (i, c) in digits.char_indices() { match c { '.' => { @@ -231,17 +237,21 @@ impl<'a> DigitInfo<'a> { let group_size = self.radix.suggest_grouping(); - let (integer, fraction, exponent) = &self.split_digit_parts(); + Self::group_digits( + &mut output, + self.integer, + group_size, + true, + self.radix == Radix::Hexadecimal, + ); - Self::group_digits(&mut output, integer, group_size, true, self.radix == Radix::Hexadecimal); - - if let Some(fraction) = fraction { + if let Some(fraction) = self.fraction { output.push('.'); Self::group_digits(&mut output, fraction, group_size, false, false); } - if let Some((separator, exponent)) = exponent { - output.push(*separator); + if let Some((separator, exponent)) = self.exponent { + output.push(separator); Self::group_digits(&mut output, exponent, group_size, true, false); } @@ -395,15 +405,13 @@ impl LiteralDigitGrouping { } } - let (integer, fraction, _) = digit_info.split_digit_parts(); - - let integral_group_size = Self::get_group_size(integer.split('_'), in_macro)?; - if let Some(fraction) = fraction { + let integral_group_size = Self::get_group_size(digit_info.integer.split('_'), in_macro)?; + if let Some(fraction) = digit_info.fraction { let fractional_group_size = Self::get_group_size(fraction.rsplit('_'), in_macro)?; let consistent = Self::parts_consistent(integral_group_size, fractional_group_size, - integer.len(), + digit_info.integer.len(), fraction.len()); if !consistent { return Err(WarningType::InconsistentDigitGrouping); @@ -503,7 +511,7 @@ impl DecimalLiteralRepresentation { then { let hex = format!("{:#X}", val); let digit_info = DigitInfo::new(&hex, false); - let _ = Self::do_lint(digit_info.digits).map_err(|warning_type| { + let _ = Self::do_lint(digit_info.integer).map_err(|warning_type| { warning_type.display(&digit_info.grouping_hint(), cx, lit.span) }); } From a9c5a599e3f1cd52f9fd745aa75bdb7afe0bb4f4 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:27:54 +0200 Subject: [PATCH 10/19] literal representation restructure 9 Only store valid suffixes (and not mistyped suffixes) in DigitInfo. Check for mistyped suffixes later and not when DigitInfo is created. This opens the door to more sophisticated mistyped suffix checks later. --- clippy_lints/src/excessive_precision.rs | 2 +- clippy_lints/src/literal_representation.rs | 161 +++++++++++---------- 2 files changed, 86 insertions(+), 77 deletions(-) diff --git a/clippy_lints/src/excessive_precision.rs b/clippy_lints/src/excessive_precision.rs index 8027a736c6b..137261cab82 100644 --- a/clippy_lints/src/excessive_precision.rs +++ b/clippy_lints/src/excessive_precision.rs @@ -86,7 +86,7 @@ impl ExcessivePrecision { if sym_str == s { None } else { - let di = super::literal_representation::DigitInfo::new(&s, true); + let di = super::literal_representation::DigitInfo::new(&s, None, true); Some(di.grouping_hint()) } } else { diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 2fe993624f4..d2df2c731f4 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -138,13 +138,21 @@ pub(super) struct DigitInfo<'a> { /// The type suffix, including preceding underscore if present. crate suffix: Option<&'a str>, - /// True for floating-point literals. - crate float: bool, } impl<'a> DigitInfo<'a> { + fn from_lit(src: &'a str, lit: &Lit) -> Option> { + if lit.kind.is_numeric() && src.chars().next().map_or(false, |c| c.is_digit(10)) { + let (unsuffixed, suffix) = split_suffix(&src, &lit.kind); + let float = if let LitKind::Float(..) = lit.kind { true } else { false }; + Some(DigitInfo::new(unsuffixed, suffix, float)) + } else { + None + } + } + #[must_use] - crate fn new(lit: &'a str, float: bool) -> Self { + crate fn new(lit: &'a str, suffix: Option<&'a str>, float: bool) -> Self { // Determine delimiter for radix prefix, if present, and radix. let radix = if lit.starts_with("0x") { Radix::Hexadecimal @@ -157,35 +165,19 @@ impl<'a> DigitInfo<'a> { }; // Grab part of the literal after prefix, if present. - let (prefix, sans_prefix) = if let Radix::Decimal = radix { + let (prefix, mut sans_prefix) = if let Radix::Decimal = radix { (None, lit) } else { let (p, s) = lit.split_at(2); (Some(p), s) }; - let mut digits = sans_prefix; - let mut suffix = None; - - let len = sans_prefix.len(); - let mut last_d = '\0'; - for (d_idx, d) in sans_prefix.char_indices() { - let suffix_start = if last_d == '_' { d_idx - 1 } else { d_idx }; - if float - && (d == 'f' - || is_possible_float_suffix_index(&sans_prefix, suffix_start, len) - || ((d == 'E' || d == 'e') && !has_possible_float_suffix(&sans_prefix))) - || !float && (d == 'i' || d == 'u' || is_possible_suffix_index(&sans_prefix, suffix_start, len)) - { - let (d, s) = sans_prefix.split_at(suffix_start); - digits = d; - suffix = Some(s); - break; - } - last_d = d + if suffix.is_some() && sans_prefix.ends_with('_') { + // The '_' before the suffix isn't part of the digits + sans_prefix = &sans_prefix[..sans_prefix.len() - 1]; } - let (integer, fraction, exponent) = Self::split_digit_parts(digits, float); + let (integer, fraction, exponent) = Self::split_digit_parts(sans_prefix, float); Self { radix, @@ -194,7 +186,6 @@ impl<'a> DigitInfo<'a> { fraction, exponent, suffix, - float, } } @@ -256,15 +247,8 @@ impl<'a> DigitInfo<'a> { } if let Some(suffix) = self.suffix { - if self.float && is_mistyped_float_suffix(suffix) { - output.push_str("_f"); - output.push_str(&suffix[1..]); - } else if is_mistyped_suffix(suffix) { - output.push_str("_i"); - output.push_str(&suffix[1..]); - } else { - output.push_str(suffix); - } + output.push('_'); + output.push_str(suffix); } output @@ -303,6 +287,34 @@ impl<'a> DigitInfo<'a> { } } +fn split_suffix<'a>(src: &'a str, lit_kind: &LitKind) -> (&'a str, Option<&'a str>) { + debug_assert!(lit_kind.is_numeric()); + if let Some(suffix_length) = lit_suffix_length(lit_kind) { + let (unsuffixed, suffix) = src.split_at(src.len() - suffix_length); + (unsuffixed, Some(suffix)) + } else { + (src, None) + } +} + +fn lit_suffix_length(lit_kind: &LitKind) -> Option { + debug_assert!(lit_kind.is_numeric()); + let suffix = match lit_kind { + LitKind::Int(_, int_lit_kind) => match int_lit_kind { + LitIntType::Signed(int_ty) => Some(int_ty.name_str()), + LitIntType::Unsigned(uint_ty) => Some(uint_ty.name_str()), + LitIntType::Unsuffixed => None, + }, + LitKind::Float(_, float_lit_kind) => match float_lit_kind { + LitFloatType::Suffixed(float_ty) => Some(float_ty.name_str()), + LitFloatType::Unsuffixed => None, + }, + _ => None, + }; + + suffix.map(str::len) +} + enum WarningType { UnreadableLiteral, InconsistentDigitGrouping, @@ -388,22 +400,13 @@ impl LiteralDigitGrouping { if_chain! { if let Some(src) = snippet_opt(cx, lit.span); - if let Some(firstch) = src.chars().next(); - if char::is_digit(firstch, 10); + if let Some(mut digit_info) = DigitInfo::from_lit(&src, &lit); then { - - let digit_info = match lit.kind { - LitKind::Int(..) => DigitInfo::new(&src, false), - LitKind::Float(..) => DigitInfo::new(&src, true), - _ => return, - }; + if !Self::check_for_mistyped_suffix(cx, lit.span, &mut digit_info) { + return; + } let result = (|| { - if let Some(suffix) = digit_info.suffix { - if is_mistyped_suffix(suffix) { - return Err(WarningType::MistypedLiteralSuffix); - } - } let integral_group_size = Self::get_group_size(digit_info.integer.split('_'), in_macro)?; if let Some(fraction) = digit_info.fraction { @@ -428,6 +431,39 @@ impl LiteralDigitGrouping { } } + // Returns `false` if the check fails + fn check_for_mistyped_suffix( + cx: &EarlyContext<'_>, + span: syntax_pos::Span, + digit_info: &mut DigitInfo<'_>, + ) -> bool { + if digit_info.suffix.is_some() { + return true; + } + + let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut digit_info.exponent { + (exponent, &["32", "64"][..], 'f') + } else if let Some(fraction) = &mut digit_info.fraction { + (fraction, &["32", "64"][..], 'f') + } else { + (&mut digit_info.integer, &["8", "16", "32", "64"][..], 'i') + }; + + let mut split = part.rsplit('_'); + let last_group = split.next().expect("At least one group"); + if split.next().is_some() && mistyped_suffixes.contains(&last_group) { + *part = &part[..part.len() - last_group.len()]; + let mut hint = digit_info.grouping_hint(); + hint.push('_'); + hint.push(missing_char); + hint.push_str(last_group); + WarningType::MistypedLiteralSuffix.display(&hint, cx, span); + false + } else { + true + } + } + /// Given the sizes of the digit groups of both integral and fractional /// parts, and the length /// of both parts, determine if the digits have been grouped consistently. @@ -503,14 +539,12 @@ impl DecimalLiteralRepresentation { if_chain! { if let LitKind::Int(val, _) = lit.kind; if let Some(src) = snippet_opt(cx, lit.span); - if let Some(firstch) = src.chars().next(); - if char::is_digit(firstch, 10); - let digit_info = DigitInfo::new(&src, false); + if let Some(digit_info) = DigitInfo::from_lit(&src, &lit); if digit_info.radix == Radix::Decimal; if val >= u128::from(self.threshold); then { let hex = format!("{:#X}", val); - let digit_info = DigitInfo::new(&hex, false); + let digit_info = DigitInfo::new(&hex, None, false); let _ = Self::do_lint(digit_info.integer).map_err(|warning_type| { warning_type.display(&digit_info.grouping_hint(), cx, lit.span) }); @@ -563,28 +597,3 @@ impl DecimalLiteralRepresentation { Ok(()) } } - -#[must_use] -fn is_mistyped_suffix(suffix: &str) -> bool { - ["_8", "_16", "_32", "_64"].contains(&suffix) -} - -#[must_use] -fn is_possible_suffix_index(lit: &str, idx: usize, len: usize) -> bool { - ((len > 3 && idx == len - 3) || (len > 2 && idx == len - 2)) && is_mistyped_suffix(lit.split_at(idx).1) -} - -#[must_use] -fn is_mistyped_float_suffix(suffix: &str) -> bool { - ["_32", "_64"].contains(&suffix) -} - -#[must_use] -fn is_possible_float_suffix_index(lit: &str, idx: usize, len: usize) -> bool { - (len > 3 && idx == len - 3) && is_mistyped_float_suffix(lit.split_at(idx).1) -} - -#[must_use] -fn has_possible_float_suffix(lit: &str) -> bool { - lit.ends_with("_32") || lit.ends_with("_64") -} From a8ca8a21c1337edc460698baea56526f8e817f06 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:28:01 +0200 Subject: [PATCH 11/19] literal representation restructure 10 Rename DigitInfo to NumericLiteral --- clippy_lints/src/excessive_precision.rs | 4 +-- clippy_lints/src/literal_representation.rs | 42 +++++++++++----------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/excessive_precision.rs b/clippy_lints/src/excessive_precision.rs index 137261cab82..287c9b7b24e 100644 --- a/clippy_lints/src/excessive_precision.rs +++ b/clippy_lints/src/excessive_precision.rs @@ -86,8 +86,8 @@ impl ExcessivePrecision { if sym_str == s { None } else { - let di = super::literal_representation::DigitInfo::new(&s, None, true); - Some(di.grouping_hint()) + let num_lit = super::literal_representation::NumericLiteral::new(&s, None, true); + Some(num_lit.grouping_hint()) } } else { None diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index d2df2c731f4..3e278ff62fe 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -123,7 +123,7 @@ impl Radix { } #[derive(Debug)] -pub(super) struct DigitInfo<'a> { +pub(super) struct NumericLiteral<'a> { /// Which radix the literal was represented in. crate radix: Radix, /// The radix prefix, if present. @@ -140,12 +140,12 @@ pub(super) struct DigitInfo<'a> { crate suffix: Option<&'a str>, } -impl<'a> DigitInfo<'a> { - fn from_lit(src: &'a str, lit: &Lit) -> Option> { +impl<'a> NumericLiteral<'a> { + fn from_lit(src: &'a str, lit: &Lit) -> Option> { if lit.kind.is_numeric() && src.chars().next().map_or(false, |c| c.is_digit(10)) { let (unsuffixed, suffix) = split_suffix(&src, &lit.kind); let float = if let LitKind::Float(..) = lit.kind { true } else { false }; - Some(DigitInfo::new(unsuffixed, suffix, float)) + Some(NumericLiteral::new(unsuffixed, suffix, float)) } else { None } @@ -400,21 +400,21 @@ impl LiteralDigitGrouping { if_chain! { if let Some(src) = snippet_opt(cx, lit.span); - if let Some(mut digit_info) = DigitInfo::from_lit(&src, &lit); + if let Some(mut num_lit) = NumericLiteral::from_lit(&src, &lit); then { - if !Self::check_for_mistyped_suffix(cx, lit.span, &mut digit_info) { + if !Self::check_for_mistyped_suffix(cx, lit.span, &mut num_lit) { return; } let result = (|| { - let integral_group_size = Self::get_group_size(digit_info.integer.split('_'), in_macro)?; - if let Some(fraction) = digit_info.fraction { + let integral_group_size = Self::get_group_size(num_lit.integer.split('_'), in_macro)?; + if let Some(fraction) = num_lit.fraction { let fractional_group_size = Self::get_group_size(fraction.rsplit('_'), in_macro)?; let consistent = Self::parts_consistent(integral_group_size, fractional_group_size, - digit_info.integer.len(), + num_lit.integer.len(), fraction.len()); if !consistent { return Err(WarningType::InconsistentDigitGrouping); @@ -425,7 +425,7 @@ impl LiteralDigitGrouping { if let Err(warning_type) = result { - warning_type.display(&digit_info.grouping_hint(), cx, lit.span) + warning_type.display(&num_lit.grouping_hint(), cx, lit.span) } } } @@ -435,25 +435,25 @@ impl LiteralDigitGrouping { fn check_for_mistyped_suffix( cx: &EarlyContext<'_>, span: syntax_pos::Span, - digit_info: &mut DigitInfo<'_>, + num_lit: &mut NumericLiteral<'_>, ) -> bool { - if digit_info.suffix.is_some() { + if num_lit.suffix.is_some() { return true; } - let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut digit_info.exponent { + let (part, mistyped_suffixes, missing_char) = if let Some((_, exponent)) = &mut num_lit.exponent { (exponent, &["32", "64"][..], 'f') - } else if let Some(fraction) = &mut digit_info.fraction { + } else if let Some(fraction) = &mut num_lit.fraction { (fraction, &["32", "64"][..], 'f') } else { - (&mut digit_info.integer, &["8", "16", "32", "64"][..], 'i') + (&mut num_lit.integer, &["8", "16", "32", "64"][..], 'i') }; let mut split = part.rsplit('_'); let last_group = split.next().expect("At least one group"); if split.next().is_some() && mistyped_suffixes.contains(&last_group) { *part = &part[..part.len() - last_group.len()]; - let mut hint = digit_info.grouping_hint(); + let mut hint = num_lit.grouping_hint(); hint.push('_'); hint.push(missing_char); hint.push_str(last_group); @@ -539,14 +539,14 @@ impl DecimalLiteralRepresentation { if_chain! { if let LitKind::Int(val, _) = lit.kind; if let Some(src) = snippet_opt(cx, lit.span); - if let Some(digit_info) = DigitInfo::from_lit(&src, &lit); - if digit_info.radix == Radix::Decimal; + if let Some(num_lit) = NumericLiteral::from_lit(&src, &lit); + if num_lit.radix == Radix::Decimal; if val >= u128::from(self.threshold); then { let hex = format!("{:#X}", val); - let digit_info = DigitInfo::new(&hex, None, false); - let _ = Self::do_lint(digit_info.integer).map_err(|warning_type| { - warning_type.display(&digit_info.grouping_hint(), cx, lit.span) + let num_lit = NumericLiteral::new(&hex, None, false); + let _ = Self::do_lint(num_lit.integer).map_err(|warning_type| { + warning_type.display(&num_lit.grouping_hint(), cx, lit.span) }); } } From eb9caf3050d9502fb98c30356da433629b5fa78d Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:28:06 +0200 Subject: [PATCH 12/19] literal representation restructure 11 Rename `grouping_hint` to `format` and use the term consistently. --- clippy_lints/src/excessive_precision.rs | 2 +- clippy_lints/src/literal_representation.rs | 28 +++++++++++----------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/excessive_precision.rs b/clippy_lints/src/excessive_precision.rs index 287c9b7b24e..3ff9679b1f6 100644 --- a/clippy_lints/src/excessive_precision.rs +++ b/clippy_lints/src/excessive_precision.rs @@ -87,7 +87,7 @@ impl ExcessivePrecision { None } else { let num_lit = super::literal_representation::NumericLiteral::new(&s, None, true); - Some(num_lit.grouping_hint()) + Some(num_lit.format()) } } else { None diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index 3e278ff62fe..f261e5bfde1 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -219,7 +219,7 @@ impl<'a> NumericLiteral<'a> { } /// Returns literal formatted in a sensible way. - crate fn grouping_hint(&self) -> String { + crate fn format(&self) -> String { let mut output = String::new(); if let Some(prefix) = self.prefix { @@ -324,7 +324,7 @@ enum WarningType { } impl WarningType { - crate fn display(&self, grouping_hint: &str, cx: &EarlyContext<'_>, span: syntax_pos::Span) { + crate fn display(&self, suggested_format: String, cx: &EarlyContext<'_>, span: syntax_pos::Span) { match self { Self::MistypedLiteralSuffix => span_lint_and_sugg( cx, @@ -332,7 +332,7 @@ impl WarningType { span, "mistyped literal suffix", "did you mean to write", - grouping_hint.to_string(), + suggested_format, Applicability::MaybeIncorrect, ), Self::UnreadableLiteral => span_lint_and_sugg( @@ -341,7 +341,7 @@ impl WarningType { span, "long literal lacking separators", "consider", - grouping_hint.to_owned(), + suggested_format, Applicability::MachineApplicable, ), Self::LargeDigitGroups => span_lint_and_sugg( @@ -350,7 +350,7 @@ impl WarningType { span, "digit groups should be smaller", "consider", - grouping_hint.to_owned(), + suggested_format, Applicability::MachineApplicable, ), Self::InconsistentDigitGrouping => span_lint_and_sugg( @@ -359,7 +359,7 @@ impl WarningType { span, "digits grouped inconsistently by underscores", "consider", - grouping_hint.to_owned(), + suggested_format, Applicability::MachineApplicable, ), Self::DecimalRepresentation => span_lint_and_sugg( @@ -368,7 +368,7 @@ impl WarningType { span, "integer literal has a better hexadecimal representation", "consider", - grouping_hint.to_owned(), + suggested_format, Applicability::MachineApplicable, ), }; @@ -425,7 +425,7 @@ impl LiteralDigitGrouping { if let Err(warning_type) = result { - warning_type.display(&num_lit.grouping_hint(), cx, lit.span) + warning_type.display(num_lit.format(), cx, lit.span) } } } @@ -453,11 +453,11 @@ impl LiteralDigitGrouping { let last_group = split.next().expect("At least one group"); if split.next().is_some() && mistyped_suffixes.contains(&last_group) { *part = &part[..part.len() - last_group.len()]; - let mut hint = num_lit.grouping_hint(); - hint.push('_'); - hint.push(missing_char); - hint.push_str(last_group); - WarningType::MistypedLiteralSuffix.display(&hint, cx, span); + let mut sugg = num_lit.format(); + sugg.push('_'); + sugg.push(missing_char); + sugg.push_str(last_group); + WarningType::MistypedLiteralSuffix.display(sugg, cx, span); false } else { true @@ -546,7 +546,7 @@ impl DecimalLiteralRepresentation { let hex = format!("{:#X}", val); let num_lit = NumericLiteral::new(&hex, None, false); let _ = Self::do_lint(num_lit.integer).map_err(|warning_type| { - warning_type.display(&num_lit.grouping_hint(), cx, lit.span) + warning_type.display(num_lit.format(), cx, lit.span) }); } } From 2e9d173be1e5a7e1e4788c44dff8b9db268679c1 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Wed, 13 Nov 2019 08:28:50 +0200 Subject: [PATCH 13/19] literal representation restructure 12 Export function for formatting literals and remove crate visibility from other items. --- clippy_lints/src/excessive_precision.rs | 4 ++-- clippy_lints/src/literal_representation.rs | 20 +++++++++++++------- clippy_lints/src/utils/sugg.rs | 2 ++ 3 files changed, 17 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/excessive_precision.rs b/clippy_lints/src/excessive_precision.rs index 3ff9679b1f6..40dbaa17732 100644 --- a/clippy_lints/src/excessive_precision.rs +++ b/clippy_lints/src/excessive_precision.rs @@ -1,4 +1,5 @@ use crate::utils::span_lint_and_sugg; +use crate::utils::sugg::format_numeric_literal; use if_chain::if_chain; use rustc::hir; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; @@ -86,8 +87,7 @@ impl ExcessivePrecision { if sym_str == s { None } else { - let num_lit = super::literal_representation::NumericLiteral::new(&s, None, true); - Some(num_lit.format()) + Some(format_numeric_literal(&s, None, true)) } } else { None diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index f261e5bfde1..b6f641d1b8c 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -114,7 +114,7 @@ pub(super) enum Radix { impl Radix { /// Returns a reasonable digit group size for this radix. #[must_use] - crate fn suggest_grouping(&self) -> usize { + fn suggest_grouping(&self) -> usize { match *self { Self::Binary | Self::Hexadecimal => 4, Self::Octal | Self::Decimal => 3, @@ -122,12 +122,18 @@ impl Radix { } } +/// A helper method to format numeric literals with digit grouping. +/// `lit` must be a valid numeric literal without suffix. +pub fn format_numeric_literal(lit: &str, type_suffix: Option<&str>, float: bool) -> String { + NumericLiteral::new(lit, type_suffix, float).format() +} + #[derive(Debug)] pub(super) struct NumericLiteral<'a> { /// Which radix the literal was represented in. - crate radix: Radix, + radix: Radix, /// The radix prefix, if present. - crate prefix: Option<&'a str>, + prefix: Option<&'a str>, /// The integer part of the number. integer: &'a str, @@ -137,7 +143,7 @@ pub(super) struct NumericLiteral<'a> { exponent: Option<(char, &'a str)>, /// The type suffix, including preceding underscore if present. - crate suffix: Option<&'a str>, + suffix: Option<&'a str>, } impl<'a> NumericLiteral<'a> { @@ -152,7 +158,7 @@ impl<'a> NumericLiteral<'a> { } #[must_use] - crate fn new(lit: &'a str, suffix: Option<&'a str>, float: bool) -> Self { + fn new(lit: &'a str, suffix: Option<&'a str>, float: bool) -> Self { // Determine delimiter for radix prefix, if present, and radix. let radix = if lit.starts_with("0x") { Radix::Hexadecimal @@ -219,7 +225,7 @@ impl<'a> NumericLiteral<'a> { } /// Returns literal formatted in a sensible way. - crate fn format(&self) -> String { + fn format(&self) -> String { let mut output = String::new(); if let Some(prefix) = self.prefix { @@ -324,7 +330,7 @@ enum WarningType { } impl WarningType { - crate fn display(&self, suggested_format: String, cx: &EarlyContext<'_>, span: syntax_pos::Span) { + fn display(&self, suggested_format: String, cx: &EarlyContext<'_>, span: syntax_pos::Span) { match self { Self::MistypedLiteralSuffix => span_lint_and_sugg( cx, diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index ff874179cff..228fda9eec0 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -18,6 +18,8 @@ use syntax::token; use syntax::util::parser::AssocOp; use syntax_pos::{BytePos, Pos}; +pub use crate::literal_representation::format_numeric_literal; + /// A helper type to build suggestion correctly handling parenthesis. pub enum Sugg<'a> { /// An expression that never needs parenthesis such as `1337` or `[0; 42]`. From 75e2dcf56b380c5c941d68a46f321c748cd807a7 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 14 Nov 2019 07:42:04 +0200 Subject: [PATCH 14/19] literal representation: simplification Simplify calculation in grouping. Add test case to ensure `count()` can't be zero in that branch. --- clippy_lints/src/literal_representation.rs | 2 +- tests/ui/inconsistent_digit_grouping.fixed | 3 +++ tests/ui/inconsistent_digit_grouping.rs | 3 +++ tests/ui/inconsistent_digit_grouping.stderr | 8 +++++++- 4 files changed, 14 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index b6f641d1b8c..f95a27aae34 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -268,7 +268,7 @@ impl<'a> NumericLiteral<'a> { let first_group_size; if partial_group_first { - first_group_size = (digits.clone().count() + group_size - 1) % group_size + 1; + first_group_size = (digits.clone().count() - 1) % group_size + 1; if pad { for _ in 0..group_size - first_group_size { output.push('0'); diff --git a/tests/ui/inconsistent_digit_grouping.fixed b/tests/ui/inconsistent_digit_grouping.fixed index a95b7e0392d..6ae93d5375a 100644 --- a/tests/ui/inconsistent_digit_grouping.fixed +++ b/tests/ui/inconsistent_digit_grouping.fixed @@ -18,4 +18,7 @@ fn main() { let _ = 0x0100_0000; let _ = 0x1000_0000; let _ = 0x0001_0000_0000_u64; + + // Test suggestion when fraction has no digits + let _: f32 = 123_456.; } diff --git a/tests/ui/inconsistent_digit_grouping.rs b/tests/ui/inconsistent_digit_grouping.rs index e316e140de8..c594bdc64c5 100644 --- a/tests/ui/inconsistent_digit_grouping.rs +++ b/tests/ui/inconsistent_digit_grouping.rs @@ -18,4 +18,7 @@ fn main() { let _ = 0x1000000; let _ = 0x10000000; let _ = 0x100000000_u64; + + // Test suggestion when fraction has no digits + let _: f32 = 1_23_456.; } diff --git a/tests/ui/inconsistent_digit_grouping.stderr b/tests/ui/inconsistent_digit_grouping.stderr index 31acea2c5e0..81c53af1d14 100644 --- a/tests/ui/inconsistent_digit_grouping.stderr +++ b/tests/ui/inconsistent_digit_grouping.stderr @@ -56,5 +56,11 @@ error: long literal lacking separators LL | let _ = 0x100000000_u64; | ^^^^^^^^^^^^^^^ help: consider: `0x0001_0000_0000_u64` -error: aborting due to 9 previous errors +error: digits grouped inconsistently by underscores + --> $DIR/inconsistent_digit_grouping.rs:23:18 + | +LL | let _: f32 = 1_23_456.; + | ^^^^^^^^^ help: consider: `123_456.` + +error: aborting due to 10 previous errors From ceb0b2d41a3a36e2e3a44a5245b14c08d688031a Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 14 Nov 2019 08:08:24 +0200 Subject: [PATCH 15/19] literal repr: ignore more warnings in macros --- clippy_lints/src/literal_representation.rs | 24 +++++++++++++++------ tests/ui/inconsistent_digit_grouping.fixed | 15 +++++++++++++ tests/ui/inconsistent_digit_grouping.rs | 15 +++++++++++++ tests/ui/inconsistent_digit_grouping.stderr | 20 ++++++++--------- tests/ui/large_digit_groups.fixed | 9 ++++++++ tests/ui/large_digit_groups.rs | 9 ++++++++ tests/ui/large_digit_groups.stderr | 12 +++++------ 7 files changed, 81 insertions(+), 23 deletions(-) diff --git a/clippy_lints/src/literal_representation.rs b/clippy_lints/src/literal_representation.rs index f95a27aae34..8aa0e872968 100644 --- a/clippy_lints/src/literal_representation.rs +++ b/clippy_lints/src/literal_representation.rs @@ -402,8 +402,6 @@ impl EarlyLintPass for LiteralDigitGrouping { impl LiteralDigitGrouping { fn check_lit(cx: &EarlyContext<'_>, lit: &Lit) { - let in_macro = in_macro(lit.span); - if_chain! { if let Some(src) = snippet_opt(cx, lit.span); if let Some(mut num_lit) = NumericLiteral::from_lit(&src, &lit); @@ -414,9 +412,9 @@ impl LiteralDigitGrouping { let result = (|| { - let integral_group_size = Self::get_group_size(num_lit.integer.split('_'), in_macro)?; + let integral_group_size = Self::get_group_size(num_lit.integer.split('_'))?; if let Some(fraction) = num_lit.fraction { - let fractional_group_size = Self::get_group_size(fraction.rsplit('_'), in_macro)?; + let fractional_group_size = Self::get_group_size(fraction.rsplit('_'))?; let consistent = Self::parts_consistent(integral_group_size, fractional_group_size, @@ -431,7 +429,19 @@ impl LiteralDigitGrouping { if let Err(warning_type) = result { - warning_type.display(num_lit.format(), cx, lit.span) + let should_warn = match warning_type { + | WarningType::UnreadableLiteral + | WarningType::InconsistentDigitGrouping + | WarningType::LargeDigitGroups => { + !in_macro(lit.span) + } + WarningType::DecimalRepresentation | WarningType::MistypedLiteralSuffix => { + true + } + }; + if should_warn { + warning_type.display(num_lit.format(), cx, lit.span) + } } } } @@ -494,7 +504,7 @@ impl LiteralDigitGrouping { /// Returns the size of the digit groups (or None if ungrouped) if successful, /// otherwise returns a `WarningType` for linting. - fn get_group_size<'a>(groups: impl Iterator, in_macro: bool) -> Result, WarningType> { + fn get_group_size<'a>(groups: impl Iterator) -> Result, WarningType> { let mut groups = groups.map(str::len); let first = groups.next().expect("At least one group"); @@ -507,7 +517,7 @@ impl LiteralDigitGrouping { } else { Ok(Some(second)) } - } else if first > 5 && !in_macro { + } else if first > 5 { Err(WarningType::UnreadableLiteral) } else { Ok(None) diff --git a/tests/ui/inconsistent_digit_grouping.fixed b/tests/ui/inconsistent_digit_grouping.fixed index 6ae93d5375a..f10673adfb2 100644 --- a/tests/ui/inconsistent_digit_grouping.fixed +++ b/tests/ui/inconsistent_digit_grouping.fixed @@ -2,6 +2,17 @@ #[warn(clippy::inconsistent_digit_grouping)] #[allow(unused_variables, clippy::excessive_precision)] fn main() { + macro_rules! mac1 { + () => { + 1_23_456 + }; + } + macro_rules! mac2 { + () => { + 1_234.5678_f32 + }; + } + let good = ( 123, 1_234, @@ -21,4 +32,8 @@ fn main() { // Test suggestion when fraction has no digits let _: f32 = 123_456.; + + // Ignore literals in macros + let _ = mac1!(); + let _ = mac2!(); } diff --git a/tests/ui/inconsistent_digit_grouping.rs b/tests/ui/inconsistent_digit_grouping.rs index c594bdc64c5..b97df0865ee 100644 --- a/tests/ui/inconsistent_digit_grouping.rs +++ b/tests/ui/inconsistent_digit_grouping.rs @@ -2,6 +2,17 @@ #[warn(clippy::inconsistent_digit_grouping)] #[allow(unused_variables, clippy::excessive_precision)] fn main() { + macro_rules! mac1 { + () => { + 1_23_456 + }; + } + macro_rules! mac2 { + () => { + 1_234.5678_f32 + }; + } + let good = ( 123, 1_234, @@ -21,4 +32,8 @@ fn main() { // Test suggestion when fraction has no digits let _: f32 = 1_23_456.; + + // Ignore literals in macros + let _ = mac1!(); + let _ = mac2!(); } diff --git a/tests/ui/inconsistent_digit_grouping.stderr b/tests/ui/inconsistent_digit_grouping.stderr index 81c53af1d14..37211efcab5 100644 --- a/tests/ui/inconsistent_digit_grouping.stderr +++ b/tests/ui/inconsistent_digit_grouping.stderr @@ -1,5 +1,5 @@ error: digits grouped inconsistently by underscores - --> $DIR/inconsistent_digit_grouping.rs:14:16 + --> $DIR/inconsistent_digit_grouping.rs:25:16 | LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); | ^^^^^^^^ help: consider: `123_456` @@ -7,31 +7,31 @@ LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f = note: `-D clippy::inconsistent-digit-grouping` implied by `-D warnings` error: digits grouped inconsistently by underscores - --> $DIR/inconsistent_digit_grouping.rs:14:26 + --> $DIR/inconsistent_digit_grouping.rs:25:26 | LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); | ^^^^^^^^^^ help: consider: `12_345_678` error: digits grouped inconsistently by underscores - --> $DIR/inconsistent_digit_grouping.rs:14:38 + --> $DIR/inconsistent_digit_grouping.rs:25:38 | LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); | ^^^^^^^^ help: consider: `1_234_567` error: digits grouped inconsistently by underscores - --> $DIR/inconsistent_digit_grouping.rs:14:48 + --> $DIR/inconsistent_digit_grouping.rs:25:48 | LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); | ^^^^^^^^^^^^^^ help: consider: `1_234.567_8_f32` error: digits grouped inconsistently by underscores - --> $DIR/inconsistent_digit_grouping.rs:14:64 + --> $DIR/inconsistent_digit_grouping.rs:25:64 | LL | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); | ^^^^^^^^^^^^^^ help: consider: `1.234_567_8_f32` error: long literal lacking separators - --> $DIR/inconsistent_digit_grouping.rs:17:13 + --> $DIR/inconsistent_digit_grouping.rs:28:13 | LL | let _ = 0x100000; | ^^^^^^^^ help: consider: `0x0010_0000` @@ -39,25 +39,25 @@ LL | let _ = 0x100000; = note: `-D clippy::unreadable-literal` implied by `-D warnings` error: long literal lacking separators - --> $DIR/inconsistent_digit_grouping.rs:18:13 + --> $DIR/inconsistent_digit_grouping.rs:29:13 | LL | let _ = 0x1000000; | ^^^^^^^^^ help: consider: `0x0100_0000` error: long literal lacking separators - --> $DIR/inconsistent_digit_grouping.rs:19:13 + --> $DIR/inconsistent_digit_grouping.rs:30:13 | LL | let _ = 0x10000000; | ^^^^^^^^^^ help: consider: `0x1000_0000` error: long literal lacking separators - --> $DIR/inconsistent_digit_grouping.rs:20:13 + --> $DIR/inconsistent_digit_grouping.rs:31:13 | LL | let _ = 0x100000000_u64; | ^^^^^^^^^^^^^^^ help: consider: `0x0001_0000_0000_u64` error: digits grouped inconsistently by underscores - --> $DIR/inconsistent_digit_grouping.rs:23:18 + --> $DIR/inconsistent_digit_grouping.rs:34:18 | LL | let _: f32 = 1_23_456.; | ^^^^^^^^^ help: consider: `123_456.` diff --git a/tests/ui/large_digit_groups.fixed b/tests/ui/large_digit_groups.fixed index cf8b36a499b..02daa22bb36 100644 --- a/tests/ui/large_digit_groups.fixed +++ b/tests/ui/large_digit_groups.fixed @@ -2,6 +2,12 @@ #[warn(clippy::large_digit_groups)] #[allow(unused_variables)] fn main() { + macro_rules! mac { + () => { + 0b1_10110_i64 + }; + } + let good = ( 0b1011_i64, 0o1_234_u32, @@ -20,4 +26,7 @@ fn main() { 123_456.123_45_f64, 123_456.123_456_f64, ); + + // Ignore literals in macros + let _ = mac!(); } diff --git a/tests/ui/large_digit_groups.rs b/tests/ui/large_digit_groups.rs index 5b9aa8c58d8..c1bb78c9d83 100644 --- a/tests/ui/large_digit_groups.rs +++ b/tests/ui/large_digit_groups.rs @@ -2,6 +2,12 @@ #[warn(clippy::large_digit_groups)] #[allow(unused_variables)] fn main() { + macro_rules! mac { + () => { + 0b1_10110_i64 + }; + } + let good = ( 0b1011_i64, 0o1_234_u32, @@ -20,4 +26,7 @@ fn main() { 1_23456.12345_f64, 1_23456.12345_6_f64, ); + + // Ignore literals in macros + let _ = mac!(); } diff --git a/tests/ui/large_digit_groups.stderr b/tests/ui/large_digit_groups.stderr index 4b5d0bd1a9f..ba8ea6b53e7 100644 --- a/tests/ui/large_digit_groups.stderr +++ b/tests/ui/large_digit_groups.stderr @@ -1,5 +1,5 @@ error: digit groups should be smaller - --> $DIR/large_digit_groups.rs:16:9 + --> $DIR/large_digit_groups.rs:22:9 | LL | 0b1_10110_i64, | ^^^^^^^^^^^^^ help: consider: `0b11_0110_i64` @@ -7,31 +7,31 @@ LL | 0b1_10110_i64, = note: `-D clippy::large-digit-groups` implied by `-D warnings` error: digit groups should be smaller - --> $DIR/large_digit_groups.rs:17:9 + --> $DIR/large_digit_groups.rs:23:9 | LL | 0x1_23456_78901_usize, | ^^^^^^^^^^^^^^^^^^^^^ help: consider: `0x0123_4567_8901_usize` error: digit groups should be smaller - --> $DIR/large_digit_groups.rs:18:9 + --> $DIR/large_digit_groups.rs:24:9 | LL | 1_23456_f32, | ^^^^^^^^^^^ help: consider: `123_456_f32` error: digit groups should be smaller - --> $DIR/large_digit_groups.rs:19:9 + --> $DIR/large_digit_groups.rs:25:9 | LL | 1_23456.12_f32, | ^^^^^^^^^^^^^^ help: consider: `123_456.12_f32` error: digit groups should be smaller - --> $DIR/large_digit_groups.rs:20:9 + --> $DIR/large_digit_groups.rs:26:9 | LL | 1_23456.12345_f64, | ^^^^^^^^^^^^^^^^^ help: consider: `123_456.123_45_f64` error: digit groups should be smaller - --> $DIR/large_digit_groups.rs:21:9 + --> $DIR/large_digit_groups.rs:27:9 | LL | 1_23456.12345_6_f64, | ^^^^^^^^^^^^^^^^^^^ help: consider: `123_456.123_456_f64` From f2d81971104627f88f2a5e0c162b11a00d621cb1 Mon Sep 17 00:00:00 2001 From: Guanqun Lu Date: Sat, 16 Nov 2019 01:06:57 +0800 Subject: [PATCH 16/19] doc: fix the comment above the lint function --- clippy_lints/src/methods/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index db94d9dcdaf..a2954a0d7bf 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2432,7 +2432,7 @@ fn lint_find_map<'a, 'tcx>( } } -/// lint use of `filter().map()` for `Iterators` +/// lint use of `filter_map().map()` for `Iterators` fn lint_filter_map_map<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, From 4da0da9281632890cb833201eb940519b388cf25 Mon Sep 17 00:00:00 2001 From: Lzu Tao Date: Tue, 19 Nov 2019 23:08:50 +0700 Subject: [PATCH 17/19] use more efficient code to generate repeated string see https://rust.godbolt.org/z/z9vrFP for comparison --- clippy_lints/src/strings.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index fca70fe8dbd..b1099102523 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -154,7 +154,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { if let LitKind::Str(ref lit_content, style) = lit.node { let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); let expanded = if let StrStyle::Raw(n) = style { - let term = (0..n).map(|_| '#').collect::(); + let term = "#".repeat(n as usize); format!("r{0}\"{1}\"{0}", term, lit_content.as_str()) } else { format!("\"{}\"", lit_content.as_str()) From d229d91d88b4c6a74d504ac1e92c5e6cc084bca4 Mon Sep 17 00:00:00 2001 From: lzutao Date: Tue, 19 Nov 2019 23:47:18 +0700 Subject: [PATCH 18/19] use usize::from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Mateusz MikuĊ‚a --- clippy_lints/src/strings.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/strings.rs b/clippy_lints/src/strings.rs index b1099102523..a3d1193052e 100644 --- a/clippy_lints/src/strings.rs +++ b/clippy_lints/src/strings.rs @@ -154,7 +154,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for StringLitAsBytes { if let LitKind::Str(ref lit_content, style) = lit.node { let callsite = snippet(cx, args[0].span.source_callsite(), r#""foo""#); let expanded = if let StrStyle::Raw(n) = style { - let term = "#".repeat(n as usize); + let term = "#".repeat(usize::from(n)); format!("r{0}\"{1}\"{0}", term, lit_content.as_str()) } else { format!("\"{}\"", lit_content.as_str()) From 1cba0c9f7dab7ebd238ab4fa247644136ac1d729 Mon Sep 17 00:00:00 2001 From: Yerkebulan Tulibergenov Date: Thu, 24 Oct 2019 23:46:25 -0700 Subject: [PATCH 19/19] fix check_infinite_loop by checking for break or return inside loop body --- clippy_lints/src/loops.rs | 46 ++++++++++++++++++++++++++-- tests/ui/infinite_loop.rs | 19 ++++++++++++ tests/ui/infinite_loop.stderr | 57 +++++++++++++++++++++++++++++------ 3 files changed, 109 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index 75d540b38e5..ca4aa1deaed 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -2367,17 +2367,57 @@ fn check_infinite_loop<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, cond: &'tcx Expr, e return; }; let mutable_static_in_cond = var_visitor.def_ids.iter().any(|(_, v)| *v); + + let mut has_break_or_return_visitor = HasBreakOrReturnVisitor { + has_break_or_return: false, + }; + has_break_or_return_visitor.visit_expr(expr); + let has_break_or_return = has_break_or_return_visitor.has_break_or_return; + if no_cond_variable_mutated && !mutable_static_in_cond { - span_lint( + span_lint_and_then( cx, WHILE_IMMUTABLE_CONDITION, cond.span, - "Variable in the condition are not mutated in the loop body. \ - This either leads to an infinite or to a never running loop.", + "variables in the condition are not mutated in the loop body", + |db| { + db.note("this may lead to an infinite or to a never running loop"); + + if has_break_or_return { + db.note("this loop contains `return`s or `break`s"); + db.help("rewrite it as `if cond { loop { } }`"); + } + }, ); } } +struct HasBreakOrReturnVisitor { + has_break_or_return: bool, +} + +impl<'a, 'tcx> Visitor<'tcx> for HasBreakOrReturnVisitor { + fn visit_expr(&mut self, expr: &'tcx Expr) { + if self.has_break_or_return { + return; + } + + match expr.kind { + ExprKind::Ret(_) | ExprKind::Break(_, _) => { + self.has_break_or_return = true; + return; + }, + _ => {}, + } + + walk_expr(self, expr); + } + + fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { + NestedVisitorMap::None + } +} + /// Collects the set of variables in an expression /// Stops analysis if a function call is found /// Note: In some cases such as `self`, there are no mutable annotation, diff --git a/tests/ui/infinite_loop.rs b/tests/ui/infinite_loop.rs index 4df218aa4f3..09f47adc46e 100644 --- a/tests/ui/infinite_loop.rs +++ b/tests/ui/infinite_loop.rs @@ -177,6 +177,23 @@ impl Counter { } } +fn while_loop_with_break_and_return() { + let y = 0; + while y < 10 { + if y == 0 { + break; + } + println!("KO - loop contains break"); + } + + while y < 10 { + if y == 0 { + return; + } + println!("KO - loop contains return"); + } +} + fn main() { immutable_condition(); unused_var(); @@ -186,4 +203,6 @@ fn main() { let mut c = Counter { count: 0 }; c.inc_n(5); c.print_n(2); + + while_loop_with_break_and_return(); } diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr index 6f0332fa8c4..2736753c14b 100644 --- a/tests/ui/infinite_loop.stderr +++ b/tests/ui/infinite_loop.stderr @@ -1,58 +1,95 @@ -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:23:11 | LL | while y < 10 { | ^^^^^^ | = note: `#[deny(clippy::while_immutable_condition)]` on by default + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:28:11 | LL | while y < 10 && x < 3 { | ^^^^^^^^^^^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:35:11 | LL | while !cond { | ^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:79:11 | LL | while i < 3 { | ^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:84:11 | LL | while i < 3 && j > 0 { | ^^^^^^^^^^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:88:11 | LL | while i < 3 { | ^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:103:11 | LL | while i < 3 { | ^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:108:11 | LL | while i < 3 { | ^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: Variable in the condition are not mutated in the loop body. This either leads to an infinite or to a never running loop. +error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:174:15 | LL | while self.count < n { | ^^^^^^^^^^^^^^ + | + = note: this may lead to an infinite or to a never running loop -error: aborting due to 9 previous errors +error: variables in the condition are not mutated in the loop body + --> $DIR/infinite_loop.rs:182:11 + | +LL | while y < 10 { + | ^^^^^^ + | + = note: this may lead to an infinite or to a never running loop + = note: this loop contains `return`s or `break`s + = help: rewrite it as `if cond { loop { } }` + +error: variables in the condition are not mutated in the loop body + --> $DIR/infinite_loop.rs:189:11 + | +LL | while y < 10 { + | ^^^^^^ + | + = note: this may lead to an infinite or to a never running loop + = note: this loop contains `return`s or `break`s + = help: rewrite it as `if cond { loop { } }` + +error: aborting due to 11 previous errors