diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d7b915fd16de..425fb4670b27 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -101,6 +101,7 @@ pub mod large_enum_variant; pub mod len_zero; pub mod let_if_seq; pub mod lifetimes; +pub mod literal_digit_grouping; pub mod loops; pub mod map_clone; pub mod matches; @@ -316,6 +317,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box large_enum_variant::LargeEnumVariant::new(conf.enum_variant_size_threshold)); reg.register_late_lint_pass(box should_assert_eq::ShouldAssertEq); reg.register_late_lint_pass(box needless_pass_by_value::NeedlessPassByValue); + reg.register_early_lint_pass(box literal_digit_grouping::LiteralDigitGrouping); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -418,6 +420,9 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { let_if_seq::USELESS_LET_IF_SEQ, lifetimes::NEEDLESS_LIFETIMES, lifetimes::UNUSED_LIFETIMES, + literal_digit_grouping::UNREADABLE_LITERAL, + literal_digit_grouping::INCONSISTENT_DIGIT_GROUPING, + literal_digit_grouping::LARGE_DIGIT_GROUPS, loops::EMPTY_LOOP, loops::EXPLICIT_COUNTER_LOOP, loops::EXPLICIT_INTO_ITER_LOOP, diff --git a/clippy_lints/src/literal_digit_grouping.rs b/clippy_lints/src/literal_digit_grouping.rs new file mode 100644 index 000000000000..ec80e6968f85 --- /dev/null +++ b/clippy_lints/src/literal_digit_grouping.rs @@ -0,0 +1,351 @@ +//! Lints concerned with the grouping of digits with underscores in integral or +//! floating-point literal expressions. + +use rustc::lint::*; +use syntax::ast::*; +use syntax_pos; +use utils::{span_help_and_lint, snippet_opt, in_external_macro}; + +/// **What it does:** Warns if a long integral or floating-point constant does +/// not contain underscores. +/// +/// **Why is this bad?** Reading long numbers is difficult without separators. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// +/// ```rust +/// 61864918973511 +/// ``` +declare_lint! { + pub UNREADABLE_LITERAL, + Warn, + "long integer literal without underscores" +} + +/// **What it does:** Warns if an integral or floating-point constant is +/// grouped inconsistently with underscores. +/// +/// **Why is this bad?** Readers may incorrectly interpret inconsistently +/// grouped digits. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// +/// ```rust +/// 618_64_9189_73_511 +/// ``` +declare_lint! { + pub INCONSISTENT_DIGIT_GROUPING, + Warn, + "integer literals with digits grouped inconsistently" +} + +/// **What it does:** Warns if the digits of an integral or floating-point +/// constant are grouped into groups that +/// are too large. +/// +/// **Why is this bad?** Negatively impacts readability. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// +/// ```rust +/// 6186491_8973511 +/// ``` +declare_lint! { + pub LARGE_DIGIT_GROUPS, + Warn, + "grouping digits into groups that are too large" +} + +#[derive(Debug)] +enum Radix { + Binary, + Octal, + Decimal, + Hexadecimal, +} + +impl Radix { + /// Return a reasonable digit group size for this radix. + pub fn suggest_grouping(&self) -> usize { + match *self { + Radix::Binary | Radix::Hexadecimal => 4, + Radix::Octal | Radix::Decimal => 3, + } + } +} + +#[derive(Debug)] +struct DigitInfo<'a> { + /// Characters of a literal between the radix prefix and type suffix. + pub digits: &'a str, + /// Which radix the literal was represented in. + pub radix: Radix, + /// The radix prefix, if present. + pub prefix: Option<&'a str>, + /// The type suffix, including preceding underscore if present. + pub suffix: Option<&'a str>, + /// True for floating-point literals. + pub float: bool, +} + +impl<'a> DigitInfo<'a> { + pub fn new(lit: &str, float: bool) -> DigitInfo { + // Determine delimiter for radix prefix, if present, and radix. + let radix = if lit.starts_with("0x") { + Radix::Hexadecimal + } else if lit.starts_with("0b") { + Radix::Binary + } else if lit.starts_with("0o") { + Radix::Octal + } else { + Radix::Decimal + }; + + // Grab part of the literal after prefix, if present. + let (prefix, sans_prefix) = if let Radix::Decimal = radix { + (None, lit) + } else { + let (p, s) = lit.split_at(2); + (Some(p), s) + }; + + let mut last_d = '\0'; + for (d_idx, d) in sans_prefix.char_indices() { + if !float && (d == 'i' || d == 'u') || float && d == 'f' { + let suffix_start = if last_d == '_' { d_idx - 1 } else { d_idx }; + let (digits, suffix) = sans_prefix.split_at(suffix_start); + return DigitInfo { + digits: digits, + radix: radix, + prefix: prefix, + suffix: Some(suffix), + float: float, + }; + } + last_d = d + } + + // No suffix found + DigitInfo { + digits: sans_prefix, + radix: radix, + prefix: prefix, + suffix: None, + float: float, + } + } + + /// Returns digits grouped in a sensible way. + fn grouping_hint(&self) -> String { + let group_size = self.radix.suggest_grouping(); + if self.digits.contains('.') { + let mut parts = self.digits.split('.'); + let int_part_hint = parts + .next() + .unwrap() + .chars() + .rev() + .filter(|&c| c != '_') + .collect::>() + .chunks(group_size) + .map(|chunk| chunk.into_iter().rev().collect()) + .rev() + .collect::>() + .join("_"); + let frac_part_hint = parts + .next() + .unwrap() + .chars() + .filter(|&c| c != '_') + .collect::>() + .chunks(group_size) + .map(|chunk| chunk.into_iter().collect()) + .collect::>() + .join("_"); + format!("{}.{}{}", int_part_hint, frac_part_hint, self.suffix.unwrap_or("")) + } else { + let hint = self.digits + .chars() + .rev() + .filter(|&c| c != '_') + .collect::>() + .chunks(group_size) + .map(|chunk| chunk.into_iter().rev().collect()) + .rev() + .collect::>() + .join("_"); + format!("{}{}{}", self.prefix.unwrap_or(""), hint, self.suffix.unwrap_or("")) + } + } +} + +enum WarningType { + UnreadableLiteral, + InconsistentDigitGrouping, + LargeDigitGroups, +} + + +impl WarningType { + pub fn display(&self, grouping_hint: &str, cx: &EarlyContext, span: &syntax_pos::Span) { + match *self { + WarningType::UnreadableLiteral => { + span_help_and_lint( + cx, + UNREADABLE_LITERAL, + *span, + "long literal lacking separators", + &format!("consider: {}", grouping_hint), + ) + }, + WarningType::LargeDigitGroups => { + span_help_and_lint( + cx, + LARGE_DIGIT_GROUPS, + *span, + "digit groups should be smaller", + &format!("consider: {}", grouping_hint), + ) + }, + WarningType::InconsistentDigitGrouping => { + span_help_and_lint( + cx, + INCONSISTENT_DIGIT_GROUPING, + *span, + "digits grouped inconsistently by underscores", + &format!("consider: {}", grouping_hint), + ) + }, + }; + } +} + +#[derive(Copy, Clone)] +pub struct LiteralDigitGrouping; + +impl LintPass for LiteralDigitGrouping { + fn get_lints(&self) -> LintArray { + lint_array!(UNREADABLE_LITERAL, INCONSISTENT_DIGIT_GROUPING, LARGE_DIGIT_GROUPS) + } +} + +impl EarlyLintPass for LiteralDigitGrouping { + fn check_expr(&mut self, cx: &EarlyContext, expr: &Expr) { + if in_external_macro(cx, expr.span) { + return; + } + + if let ExprKind::Lit(ref lit) = expr.node { + self.check_lit(cx, lit) + } + } +} + +impl LiteralDigitGrouping { + fn check_lit(&self, cx: &EarlyContext, lit: &Lit) { + // Lint integral literals. + if_let_chain! {[ + let LitKind::Int(..) = lit.node, + let Some(src) = snippet_opt(cx, lit.span), + let Some(firstch) = src.chars().next(), + char::to_digit(firstch, 10).is_some() + ], { + let digit_info = DigitInfo::new(&src, false); + let _ = LiteralDigitGrouping::do_lint(digit_info.digits).map_err(|warning_type| { + warning_type.display(&digit_info.grouping_hint(), cx, &lit.span) + }); + }} + + // Lint floating-point literals. + if_let_chain! {[ + let LitKind::Float(..) = lit.node, + let Some(src) = snippet_opt(cx, lit.span), + let Some(firstch) = src.chars().next(), + char::to_digit(firstch, 10).is_some() + ], { + 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 _ = LiteralDigitGrouping::do_lint(parts[0]) + .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 _ = LiteralDigitGrouping::do_lint(fractional_part) + .map(|fractional_group_size| { + let consistent = LiteralDigitGrouping::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 + /// parts, and the length + /// of both parts, determine if the digits have been grouped consistently. + fn parts_consistent(int_group_size: usize, frac_group_size: usize, 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, + // Integral part has grouped digits, fractional part does not. + (_, 0) => frac_size <= int_group_size, + // Fractional part has grouped digits, integral part does not. + (0, _) => int_size <= frac_group_size, + // Both parts have grouped digits. Groups should be the same 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) -> 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(); + + if underscore_positions.is_empty() { + // Check if literal needs underscores. + if digits.len() > 4 { + Err(WarningType::UnreadableLiteral) + } else { + Ok(0) + } + } 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().unwrap() <= group_size + 1); + + if !consistent { + return Err(WarningType::InconsistentDigitGrouping); + } else if group_size > 4 { + return Err(WarningType::LargeDigitGroups); + } + Ok(group_size) + } + } +} diff --git a/clippy_lints/src/misc_early.rs b/clippy_lints/src/misc_early.rs index 1d48199004ca..c63dff52ae92 100644 --- a/clippy_lints/src/misc_early.rs +++ b/clippy_lints/src/misc_early.rs @@ -108,7 +108,7 @@ declare_lint! { /// **What it does:** Warns if an integral constant literal starts with `0`. /// /// **Why is this bad?** In some languages (including the infamous C language and most of its -/// familly), this marks an octal constant. In Rust however, this is a decimal constant. This could +/// family), this marks an octal constant. In Rust however, this is a decimal constant. This could /// be confusing for both the writer and a reader of the constant. /// /// **Known problems:** None. @@ -367,12 +367,12 @@ impl MiscEarly { db.span_suggestion( lit.span, "if you mean to use a decimal constant, remove the `0` to remove confusion", - src.trim_left_matches('0').to_string(), + src.trim_left_matches(|c| c == '_' || c == '0').to_string(), ); db.span_suggestion( lit.span, "if you mean to use an octal constant, use `0o`", - format!("0o{}", src.trim_left_matches('0')), + format!("0o{}", src.trim_left_matches(|c| c == '_' || c == '0')), ); }); } diff --git a/clippy_tests/examples/inconsistent_digit_grouping.rs b/clippy_tests/examples/inconsistent_digit_grouping.rs new file mode 100644 index 000000000000..06e8996deb7b --- /dev/null +++ b/clippy_tests/examples/inconsistent_digit_grouping.rs @@ -0,0 +1,8 @@ +#![feature(plugin)] +#![plugin(clippy)] +#[warn(inconsistent_digit_grouping)] +#[allow(unused_variables)] +fn main() { + let good = (123, 1_234, 1_2345_6789, 123_f32, 1_234.12_f32, 1_234.123_4_f32, 1.123_456_7_f32); + let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); +} diff --git a/clippy_tests/examples/inconsistent_digit_grouping.stderr b/clippy_tests/examples/inconsistent_digit_grouping.stderr new file mode 100644 index 000000000000..ba2bc305d8d3 --- /dev/null +++ b/clippy_tests/examples/inconsistent_digit_grouping.stderr @@ -0,0 +1,45 @@ +error: digits grouped inconsistently by underscores + --> inconsistent_digit_grouping.rs:7:16 + | +7 | let bad = (1_23_456, 1_234_5678, 1234_567, 1_234.5678_f32, 1.234_5678_f32); + | ^^^^^^^^ + | + = note: `-D inconsistent-digit-grouping` implied by `-D warnings` + = help: consider: 123_456 + +error: digits grouped inconsistently by underscores + --> inconsistent_digit_grouping.rs:7:26 + | +7 | 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 + --> inconsistent_digit_grouping.rs:7:38 + | +7 | 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 + --> inconsistent_digit_grouping.rs:7:48 + | +7 | 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 + --> inconsistent_digit_grouping.rs:7:64 + | +7 | 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 + + +To learn more, run the command again with --verbose. diff --git a/clippy_tests/examples/large_digit_groups.rs b/clippy_tests/examples/large_digit_groups.rs new file mode 100644 index 000000000000..65bcdc7435e8 --- /dev/null +++ b/clippy_tests/examples/large_digit_groups.rs @@ -0,0 +1,8 @@ +#![feature(plugin)] +#![plugin(clippy)] +#[warn(large_digit_groups)] +#[allow(unused_variables)] +fn main() { + let good = (0b1011_i64, 0o1_234_u32, 0x1_234_567, 1_2345_6789, 1234_f32, 1_234.12_f32, 1_234.123_f32, 1.123_4_f32); + let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); +} diff --git a/clippy_tests/examples/large_digit_groups.stderr b/clippy_tests/examples/large_digit_groups.stderr new file mode 100644 index 000000000000..408bff8b3cd6 --- /dev/null +++ b/clippy_tests/examples/large_digit_groups.stderr @@ -0,0 +1,53 @@ +error: digit groups should be smaller + --> large_digit_groups.rs:7:16 + | +7 | let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); + | ^^^^^^^^^^^^^ + | + = note: `-D large-digit-groups` implied by `-D warnings` + = help: consider: 0b11_0110_i64 + +error: digit groups should be smaller + --> large_digit_groups.rs:7:31 + | +7 | let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: consider: 0x123_4567_8901_usize + +error: digit groups should be smaller + --> large_digit_groups.rs:7:54 + | +7 | let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); + | ^^^^^^^^^^^ + | + = help: consider: 123_456_f32 + +error: digit groups should be smaller + --> large_digit_groups.rs:7:67 + | +7 | let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); + | ^^^^^^^^^^^^^^ + | + = help: consider: 123_456.12_f32 + +error: digit groups should be smaller + --> large_digit_groups.rs:7:83 + | +7 | let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); + | ^^^^^^^^^^^^^^^^^ + | + = help: consider: 123_456.123_45_f32 + +error: digit groups should be smaller + --> large_digit_groups.rs:7:102 + | +7 | let bad = (0b1_10110_i64, 0x1_23456_78901_usize, 1_23456_f32, 1_23456.12_f32, 1_23456.12345_f32, 1_23456.12345_6_f32); + | ^^^^^^^^^^^^^^^^^^^ + | + = help: consider: 123_456.123_456_f32 + +error: aborting due to 6 previous errors + + +To learn more, run the command again with --verbose. diff --git a/clippy_tests/examples/literals.rs b/clippy_tests/examples/literals.rs index fa76d3308786..e8105a74b5cf 100644 --- a/clippy_tests/examples/literals.rs +++ b/clippy_tests/examples/literals.rs @@ -14,7 +14,7 @@ fn main() { let fail1 = 0xabCD; let fail2 = 0xabCD_u32; let fail2 = 0xabCD_isize; - let fail_multi_zero = 000123usize; + let fail_multi_zero = 000_123usize; let ok6 = 1234_i32; let ok7 = 1234_f32; @@ -30,5 +30,5 @@ fn main() { let fail8 = 0123; let ok11 = 0o123; - let ok12 = 0b101010; + let ok12 = 0b10_1010; } diff --git a/clippy_tests/examples/literals.stderr b/clippy_tests/examples/literals.stderr index 93edaaf235a6..05f623cae951 100644 --- a/clippy_tests/examples/literals.stderr +++ b/clippy_tests/examples/literals.stderr @@ -21,16 +21,16 @@ error: inconsistent casing in hexadecimal literal error: integer type suffix should be separated by an underscore --> literals.rs:17:27 | -17 | let fail_multi_zero = 000123usize; - | ^^^^^^^^^^^ +17 | let fail_multi_zero = 000_123usize; + | ^^^^^^^^^^^^ | = note: `-D unseparated-literal-suffix` implied by `-D warnings` error: this is a decimal constant --> literals.rs:17:27 | -17 | let fail_multi_zero = 000123usize; - | ^^^^^^^^^^^ +17 | let fail_multi_zero = 000_123usize; + | ^^^^^^^^^^^^ | = note: `-D zero-prefixed-literal` implied by `-D warnings` help: if you mean to use a decimal constant, remove the `0` to remove confusion diff --git a/clippy_tests/examples/unreadable_literal.rs b/clippy_tests/examples/unreadable_literal.rs new file mode 100644 index 000000000000..45daf70b171a --- /dev/null +++ b/clippy_tests/examples/unreadable_literal.rs @@ -0,0 +1,8 @@ +#![feature(plugin)] +#![plugin(clippy)] +#[warn(unreadable_literal)] +#[allow(unused_variables)] +fn main() { + let good = (0b1011_i64, 0o1_234_u32, 0x1_234_567, 1_2345_6789, 1234_f32, 1_234.12_f32, 1_234.123_f32, 1.123_4_f32); + let bad = (0b10110_i64, 0x12345678901_usize, 12345_f32, 1.23456_f32); +} diff --git a/clippy_tests/examples/unreadable_literal.stderr b/clippy_tests/examples/unreadable_literal.stderr new file mode 100644 index 000000000000..e4c110915d0a --- /dev/null +++ b/clippy_tests/examples/unreadable_literal.stderr @@ -0,0 +1,37 @@ +error: long literal lacking separators + --> unreadable_literal.rs:7:16 + | +7 | let bad = (0b10110_i64, 0x12345678901_usize, 12345_f32, 1.23456_f32); + | ^^^^^^^^^^^ + | + = note: `-D unreadable-literal` implied by `-D warnings` + = help: consider: 0b1_0110_i64 + +error: long literal lacking separators + --> unreadable_literal.rs:7:29 + | +7 | let bad = (0b10110_i64, 0x12345678901_usize, 12345_f32, 1.23456_f32); + | ^^^^^^^^^^^^^^^^^^^ + | + = help: consider: 0x123_4567_8901_usize + +error: long literal lacking separators + --> unreadable_literal.rs:7:50 + | +7 | let bad = (0b10110_i64, 0x12345678901_usize, 12345_f32, 1.23456_f32); + | ^^^^^^^^^ + | + = help: consider: 12_345_f32 + +error: long literal lacking separators + --> unreadable_literal.rs:7:61 + | +7 | let bad = (0b10110_i64, 0x12345678901_usize, 12345_f32, 1.23456_f32); + | ^^^^^^^^^^^ + | + = help: consider: 1.234_56_f32 + +error: aborting due to 4 previous errors + + +To learn more, run the command again with --verbose.