Merge pull request #1900 from jayhardee9/luckily_literals_love_lints

Add lints for digit grouping.

clippy_lints/src/lib.rs

@@ -101,6 +101,7 @@ macro_rules! declare_restriction_lint {
 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,

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::<Vec<_>>()
+                .chunks(group_size)
+                .map(|chunk| chunk.into_iter().rev().collect())
+                .rev()
+                .collect::<Vec<String>>()
+                .join("_");
+            let frac_part_hint = parts
+                .next()
+                .unwrap()
+                .chars()
+                .filter(|&c| c != '_')
+                .collect::<Vec<_>>()
+                .chunks(group_size)
+                .map(|chunk| chunk.into_iter().collect())
+                .collect::<Vec<String>>()
+                .join("_");
+            format!("{}.{}{}", int_part_hint, frac_part_hint, self.suffix.unwrap_or(""))
+        } else {
+            let hint = self.digits
+                .chars()
+                .rev()
+                .filter(|&c| c != '_')
+                .collect::<Vec<_>>()
+                .chunks(group_size)
+                .map(|chunk| chunk.into_iter().rev().collect())
+                .rev()
+                .collect::<Vec<String>>()
+                .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::<String>();
+                        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<usize, WarningType> {
+        // Grab underscore indices with respect to the units digit.
+        let underscore_positions: Vec<usize> = 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)
+        }
+    }
+}

clippy_lints/src/misc_early.rs

@@ -108,7 +108,7 @@
 /// **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 @@ fn check_lit(&self, cx: &EarlyContext, lit: &Lit) {
                     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')),
                     );
                 });
             }

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);
+}

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.

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);
+}

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.

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;
 }

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

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);
+}

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.