]> git.lizzy.rs Git - rust.git/commitdiff
Merge pull request #106 from marcusklaas/issue-number-check-2
authorNick Cameron <nrc@ncameron.org>
Mon, 15 Jun 2015 08:32:12 +0000 (20:32 +1200)
committerNick Cameron <nrc@ncameron.org>
Mon, 15 Jun 2015 08:32:12 +0000 (20:32 +1200)
Implement checks for unnumbered TODOs and FIXMEs

src/changes.rs
src/config.rs
src/default.toml
src/issues.rs [new file with mode: 0644]
src/lib.rs
src/lists.rs
src/utils.rs
tests/config/small_tabs.toml

index 5ed1c82b22f929495636a4b14bc3b50ae8d9be4f..97133ab67fbdd688fb6d2e22d1e2b4a651bf2800 100644 (file)
@@ -21,6 +21,7 @@
 use std::io::{Write, stdout};
 use WriteMode;
 use NewlineStyle;
+use utils::round_up_to_power_of_two;
 
 // This is basically a wrapper around a bunch of Ropes which makes it convenient
 // to work with libsyntax. It is badly named.
@@ -41,11 +42,10 @@ pub fn from_codemap(codemap: &'a CodeMap) -> ChangeSet<'a> {
 
         for f in codemap.files.borrow().iter() {
             // Use the length of the file as a heuristic for how much space we
-            // need. I hope that at some stage someone rounds this up to the next
-            // power of two. TODO check that or do it here.
-            result.file_map.insert(f.name.clone(),
-                                   StringBuffer::with_capacity(f.src.as_ref().unwrap().len()));
+            // need. Round to the next power of two.
+            let buffer_cap = round_up_to_power_of_two(f.src.as_ref().unwrap().len());
 
+            result.file_map.insert(f.name.clone(), StringBuffer::with_capacity(buffer_cap));
             result.file_spans.push((f.start_pos.0, f.end_pos.0));
         }
 
index 8d6fa827b66eee4bcc50ecf5fc657d5df4e8b303..c803a07d9d0ed9c44616ced038500caf1aa9d64f 100644 (file)
 
 extern crate toml;
 
+use {NewlineStyle, BraceStyle, ReturnIndent};
+use lists::SeparatorTactic;
+use issues::ReportTactic;
+
 #[derive(RustcDecodable)]
 pub struct Config {
     pub max_width: usize,
     pub ideal_width: usize,
     pub leeway: usize,
     pub tab_spaces: usize,
-    pub newline_style: ::NewlineStyle,
-    pub fn_brace_style: ::BraceStyle,
-    pub fn_return_indent: ::ReturnIndent,
+    pub newline_style: NewlineStyle,
+    pub fn_brace_style: BraceStyle,
+    pub fn_return_indent: ReturnIndent,
     pub fn_args_paren_newline: bool,
     pub struct_trailing_comma: bool,
-    pub struct_lit_trailing_comma: ::lists::SeparatorTactic,
+    pub struct_lit_trailing_comma: SeparatorTactic,
     pub enum_trailing_comma: bool,
+    pub report_todo: ReportTactic,
+    pub report_fixme: ReportTactic,
 }
 
 impl Config {
index 1a7b01473f3865dc4d867dad5de51c8e2a72b219..75ab3a60164489a575eceed77c734647e080a80f 100644 (file)
@@ -9,3 +9,5 @@ fn_args_paren_newline = true
 struct_trailing_comma = true
 struct_lit_trailing_comma = "Vertical"
 enum_trailing_comma = true
+report_todo = "Always"
+report_fixme = "Never"
diff --git a/src/issues.rs b/src/issues.rs
new file mode 100644 (file)
index 0000000..9ee70e1
--- /dev/null
@@ -0,0 +1,298 @@
+// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// Objects for seeking through a char stream for occurences of TODO and FIXME.
+// Depending on the loaded configuration, may also check that these have an
+// associated issue number.
+
+use std::fmt;
+
+static TO_DO_CHARS: &'static [char] = &['T', 'O', 'D', 'O'];
+static FIX_ME_CHARS: &'static [char] = &['F', 'I', 'X', 'M', 'E'];
+
+#[derive(Clone, Copy)]
+pub enum ReportTactic {
+    Always,
+    Unnumbered,
+    Never
+}
+
+impl ReportTactic {
+    fn is_enabled(&self) -> bool {
+        match *self {
+            ReportTactic::Always => true,
+            ReportTactic::Unnumbered => true,
+            ReportTactic::Never => false
+        }
+    }
+}
+
+impl_enum_decodable!(ReportTactic, Always, Unnumbered, Never);
+
+#[derive(Clone, Copy)]
+enum Seeking {
+    Issue {
+        todo_idx: usize,
+        fixme_idx: usize
+    },
+    Number {
+        issue: Issue,
+        part: NumberPart
+    }
+}
+
+#[derive(Clone, Copy)]
+enum NumberPart {
+    OpenParen,
+    Pound,
+    Number,
+    CloseParen
+}
+
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
+pub struct Issue {
+    issue_type: IssueType,
+    // Indicates whether we're looking for issues with missing numbers, or
+    // all issues of this type.
+    missing_number: bool,
+}
+
+impl fmt::Display for Issue {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+        let msg = match self.issue_type {
+            IssueType::Todo => "TODO",
+            IssueType::Fixme => "FIXME",
+        };
+        let details = if self.missing_number { " without issue number" } else { "" };
+
+        write!(fmt, "{}{}", msg, details)
+    }
+}
+
+#[derive(PartialEq, Eq, Debug, Clone, Copy)]
+enum IssueType {
+    Todo,
+    Fixme
+}
+
+enum IssueClassification {
+    Good,
+    Bad(Issue),
+    None
+}
+
+pub struct BadIssueSeeker {
+    state: Seeking,
+    report_todo: ReportTactic,
+    report_fixme: ReportTactic,
+}
+
+impl BadIssueSeeker {
+    pub fn new(report_todo: ReportTactic, report_fixme: ReportTactic) -> BadIssueSeeker {
+        BadIssueSeeker {
+            state: Seeking::Issue { todo_idx: 0, fixme_idx: 0 },
+            report_todo: report_todo,
+            report_fixme: report_fixme,
+        }
+    }
+
+    // Check whether or not the current char is conclusive evidence for an
+    // unnumbered TO-DO or FIX-ME.
+    pub fn inspect(&mut self, c: char) -> Option<Issue> {
+        match self.state {
+            Seeking::Issue { todo_idx, fixme_idx } => {
+                self.state = self.inspect_issue(c, todo_idx, fixme_idx);
+            },
+            Seeking::Number { issue, part } => {
+                let result = self.inspect_number(c, issue, part);
+
+                if let IssueClassification::None = result {
+                    return None;
+                }
+
+                self.state = Seeking::Issue { todo_idx: 0, fixme_idx: 0 };
+
+                if let IssueClassification::Bad(issue) = result {
+                    return Some(issue);
+                }
+            }
+        }
+
+        None
+    }
+
+    fn inspect_issue(&mut self, c: char, mut todo_idx: usize, mut fixme_idx: usize) -> Seeking {
+        // FIXME: Should we also check for lower case characters?
+        if self.report_todo.is_enabled() && c == TO_DO_CHARS[todo_idx] {
+            todo_idx += 1;
+            if todo_idx == TO_DO_CHARS.len() {
+                return Seeking::Number {
+                    issue: Issue {
+                        issue_type: IssueType::Todo,
+                        missing_number: if let ReportTactic::Unnumbered = self.report_todo {
+                            true
+                        } else {
+                            false
+                        }
+                    },
+                    part: NumberPart::OpenParen
+                };
+            }
+            fixme_idx = 0;
+        } else if self.report_fixme.is_enabled() && c == FIX_ME_CHARS[fixme_idx] {
+            // Exploit the fact that the character sets of todo and fixme
+            // are disjoint by adding else.
+            fixme_idx += 1;
+            if fixme_idx == FIX_ME_CHARS.len() {
+                return Seeking::Number {
+                    issue: Issue {
+                        issue_type: IssueType::Fixme,
+                        missing_number: if let ReportTactic::Unnumbered = self.report_fixme {
+                            true
+                        } else {
+                            false
+                        }
+                    },
+                    part: NumberPart::OpenParen
+                };
+            }
+            todo_idx = 0;
+        } else {
+            todo_idx = 0;
+            fixme_idx = 0;
+        }
+
+        Seeking::Issue { todo_idx: todo_idx, fixme_idx: fixme_idx }
+    }
+
+    fn inspect_number(&mut self,
+                      c: char,
+                      issue: Issue,
+                      mut part: NumberPart)
+        -> IssueClassification
+    {
+        if ! issue.missing_number || c == '\n' {
+            return IssueClassification::Bad(issue);
+        } else if c == ')' {
+            return if let NumberPart::CloseParen = part {
+                IssueClassification::Good
+            } else {
+                IssueClassification::Bad(issue)
+            };
+        }
+
+        match part {
+            NumberPart::OpenParen => {
+                if c != '(' {
+                    return IssueClassification::Bad(issue);
+                } else {
+                    part = NumberPart::Pound;
+                }
+            },
+            NumberPart::Pound => {
+                if c == '#' {
+                    part = NumberPart::Number;
+                }
+            },
+            NumberPart::Number => {
+                if c >= '0' && c <= '9' {
+                    part = NumberPart::CloseParen;
+                } else {
+                    return IssueClassification::Bad(issue);
+                }
+            },
+            NumberPart::CloseParen => {}
+        }
+
+        self.state = Seeking::Number {
+            part: part,
+            issue: issue
+        };
+
+        IssueClassification::None
+    }
+}
+
+#[test]
+fn find_unnumbered_issue() {
+    fn check_fail(text: &str, failing_pos: usize) {
+        println!("{:?}", text);
+        let mut seeker = BadIssueSeeker::new(ReportTactic::Unnumbered, ReportTactic::Unnumbered);
+        assert_eq!(Some(failing_pos), text.chars().position(|c| seeker.inspect(c).is_some()));
+    }
+
+    fn check_pass(text: &str) {
+        let mut seeker = BadIssueSeeker::new(ReportTactic::Unnumbered, ReportTactic::Unnumbered);
+        assert_eq!(None, text.chars().position(|c| seeker.inspect(c).is_some()));
+    }
+
+    check_fail("TODO\n", 4);
+    check_pass(" TO FIX DOME\n");
+    check_fail(" \n FIXME\n", 8);
+    check_fail("FIXME(\n", 6);
+    check_fail("FIXME(#\n", 7);
+    check_fail("FIXME(#1\n", 8);
+    check_fail("FIXME(#)1\n", 7);
+    check_pass("FIXME(#1222)\n");
+    check_fail("FIXME(#12\n22)\n", 9);
+    check_pass("FIXME(@maintainer, #1222, hello)\n");
+    check_fail("TODO(#22) FIXME\n", 15);
+}
+
+#[test]
+fn find_issue() {
+    fn is_bad_issue(text: &str, report_todo: ReportTactic, report_fixme: ReportTactic) -> bool {
+        let mut seeker = BadIssueSeeker::new(report_todo, report_fixme);
+        text.chars().any(|c| seeker.inspect(c).is_some())
+    }
+
+    assert!(is_bad_issue("TODO(@maintainer, #1222, hello)\n",
+                         ReportTactic::Always,
+                         ReportTactic::Never));
+
+    assert!(! is_bad_issue("TODO: no number\n",
+                           ReportTactic::Never,
+                           ReportTactic::Always));
+
+    assert!(is_bad_issue("This is a FIXME(#1)\n",
+                         ReportTactic::Never,
+                         ReportTactic::Always));
+
+    assert!(! is_bad_issue("bad FIXME\n",
+                           ReportTactic::Always,
+                           ReportTactic::Never));
+}
+
+#[test]
+fn issue_type() {
+    let mut seeker = BadIssueSeeker::new(ReportTactic::Always, ReportTactic::Never);
+    let expected = Some(Issue {
+        issue_type: IssueType::Todo,
+        missing_number: false
+    });
+
+    assert_eq!(expected,
+               "TODO(#100): more awesomeness".chars()
+                                       .map(|c| seeker.inspect(c))
+                                       .find(Option::is_some)
+                                       .unwrap());
+
+    let mut seeker = BadIssueSeeker::new(ReportTactic::Never, ReportTactic::Unnumbered);
+    let expected = Some(Issue {
+        issue_type: IssueType::Fixme,
+        missing_number: true
+    });
+
+    assert_eq!(expected,
+               "Test. FIXME: bad, bad, not good".chars()
+                                                .map(|c| seeker.inspect(c))
+                                                .find(Option::is_some)
+                                                .unwrap());
+}
index 9d80f259ebdc7d95dd38e55a56687ab20ed7532d..c8fdc2eab96f03a720d1f5f26d1078c19ab4950e 100644 (file)
@@ -33,8 +33,6 @@
 use rustc::session::config::Input;
 use rustc_driver::{driver, CompilerCalls, Compilation};
 
-use rustc_serialize::{Decodable, Decoder};
-
 use syntax::ast;
 use syntax::codemap::CodeMap;
 use syntax::diagnostics;
@@ -42,7 +40,9 @@
 
 use std::path::PathBuf;
 use std::collections::HashMap;
+use std::fmt;
 
+use issues::{BadIssueSeeker, Issue};
 use changes::ChangeSet;
 use visitor::FmtVisitor;
 
@@ -58,6 +58,7 @@
 mod types;
 mod expr;
 mod imports;
+mod issues;
 
 const MIN_STRING: usize = 10;
 // When we get scoped annotations, we should have rustfmt::skip.
@@ -106,6 +107,58 @@ pub enum ReturnIndent {
 
 impl_enum_decodable!(ReturnIndent, WithArgs, WithWhereClause);
 
+enum ErrorKind {
+    // Line has more than config!(max_width) characters
+    LineOverflow,
+    // Line ends in whitespace
+    TrailingWhitespace,
+    // TO-DO or FIX-ME item without an issue number
+    BadIssue(Issue),
+}
+
+impl fmt::Display for ErrorKind {
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+        match *self {
+            ErrorKind::LineOverflow => {
+                write!(fmt, "line exceeded maximum length")
+            },
+            ErrorKind::TrailingWhitespace => {
+                write!(fmt, "left behind trailing whitespace")
+            },
+            ErrorKind::BadIssue(issue) => {
+                write!(fmt, "found {}", issue)
+            },
+        }
+    }
+}
+
+// Formatting errors that are identified *after* rustfmt has run
+struct FormattingError {
+    line: u32,
+    kind: ErrorKind,
+}
+
+struct FormatReport {
+    // Maps stringified file paths to their associated formatting errors
+    file_error_map: HashMap<String, Vec<FormattingError>>,
+}
+
+impl fmt::Display for FormatReport {
+    // Prints all the formatting errors.
+    fn fmt(&self, fmt: &mut fmt::Formatter) -> Result<(), fmt::Error> {
+        for (file, errors) in self.file_error_map.iter() {
+            for error in errors {
+                try!(write!(fmt,
+                            "Rustfmt failed at {}:{}: {} (sorry)\n",
+                            file,
+                            error.line,
+                            error.kind));
+            }
+        }
+        Ok(())
+    }
+}
+
 // Formatting which depends on the AST.
 fn fmt_ast<'a>(krate: &ast::Crate, codemap: &'a CodeMap) -> ChangeSet<'a> {
     let mut visitor = FmtVisitor::from_codemap(codemap);
@@ -119,11 +172,11 @@ fn fmt_ast<'a>(krate: &ast::Crate, codemap: &'a CodeMap) -> ChangeSet<'a> {
 }
 
 // Formatting done on a char by char or line by line basis.
-// TODO warn on TODOs and FIXMEs without an issue number
 // TODO warn on bad license
 // TODO other stuff for parity with make tidy
-fn fmt_lines(changes: &mut ChangeSet) {
+fn fmt_lines(changes: &mut ChangeSet) -> FormatReport {
     let mut truncate_todo = Vec::new();
+    let mut report = FormatReport { file_error_map: HashMap::new() };
 
     // Iterate over the chars in the change set.
     for (f, text) in changes.text() {
@@ -132,8 +185,21 @@ fn fmt_lines(changes: &mut ChangeSet) {
         let mut line_len = 0;
         let mut cur_line = 1;
         let mut newline_count = 0;
+        let mut errors = vec![];
+        let mut issue_seeker = BadIssueSeeker::new(config!(report_todo),
+                                                   config!(report_fixme));
+
         for (c, b) in text.chars() {
             if c == '\r' { continue; }
+
+            // Add warnings for bad todos/ fixmes
+            if let Some(issue) = issue_seeker.inspect(c) {
+                errors.push(FormattingError {
+                    line: cur_line,
+                    kind: ErrorKind::BadIssue(issue)
+                });
+            }
+
             if c == '\n' {
                 // Check for (and record) trailing whitespace.
                 if let Some(lw) = last_wspace {
@@ -142,9 +208,10 @@ fn fmt_lines(changes: &mut ChangeSet) {
                 }
                 // Check for any line width errors we couldn't correct.
                 if line_len > config!(max_width) {
-                    // TODO store the error rather than reporting immediately.
-                    println!("Rustfmt couldn't fix (sorry). {}:{}: line longer than {} characters",
-                             f, cur_line, config!(max_width));
+                    errors.push(FormattingError {
+                        line: cur_line,
+                        kind: ErrorKind::LineOverflow
+                    });
                 }
                 line_len = 0;
                 cur_line += 1;
@@ -165,18 +232,24 @@ fn fmt_lines(changes: &mut ChangeSet) {
 
         if newline_count > 1 {
             debug!("track truncate: {} {} {}", f, text.len, newline_count);
-            truncate_todo.push((f.to_string(), text.len - newline_count + 1))
+            truncate_todo.push((f.to_owned(), text.len - newline_count + 1))
         }
 
         for &(l, _, _) in trims.iter() {
-            // TODO store the error rather than reporting immediately.
-            println!("Rustfmt left trailing whitespace at {}:{} (sorry)", f, l);
+            errors.push(FormattingError {
+                line: l,
+                kind: ErrorKind::TrailingWhitespace
+            });
         }
+
+        report.file_error_map.insert(f.to_owned(), errors);
     }
 
     for (f, l) in truncate_todo {
         changes.get_mut(&f).truncate(l);
     }
+
+    report
 }
 
 struct RustFmtCalls {
@@ -237,7 +310,7 @@ fn build_controller(&mut self, _: &Session) -> driver::CompileController<'a> {
             // For some reason, the codemap does not include terminating newlines
             // so we must add one on for each file. This is sad.
             changes.append_newlines();
-            fmt_lines(&mut changes);
+            println!("{}", fmt_lines(&mut changes));
 
             let result = changes.write_all_files(write_mode);
 
index f31458fcc57736cb0e2a02cddf272d973ab552de..9f1006c39f0bc9711a4515317789fc0c00e6eb23 100644 (file)
@@ -9,7 +9,6 @@
 // except according to those terms.
 
 use utils::make_indent;
-use rustc_serialize::{Decodable, Decoder};
 
 #[derive(Eq, PartialEq, Debug, Copy, Clone)]
 pub enum ListTactic {
index 0e992e9f7da4cddf787b792fa95afd75d789b1b9..c32a929332635b6923e5f30c314d6dc2fd22ac95 100644 (file)
@@ -48,12 +48,38 @@ pub fn format_visibility(vis: Visibility) -> &'static str {
     }
 }
 
+#[inline]
+#[cfg(target_pointer_width="64")]
+// Based on the trick layed out at
+// http://graphics.stanford.edu/~seander/bithacks.html#RoundUpPowerOf2
+pub fn round_up_to_power_of_two(mut x: usize) -> usize {
+    x -= 1;
+    x |= x >> 1;
+    x |= x >> 2;
+    x |= x >> 4;
+    x |= x >> 8;
+    x |= x >> 16;
+    x |= x >> 32;
+    x + 1
+}
+
+#[cfg(target_pointer_width="32")]
+pub fn round_up_to_power_of_two(mut x: usize) -> usize {
+    x -= 1;
+    x |= x >> 1;
+    x |= x >> 2;
+    x |= x >> 4;
+    x |= x >> 8;
+    x |= x >> 16;
+    x + 1
+}
+
 // Macro for deriving implementations of Decodable for enums
 #[macro_export]
 macro_rules! impl_enum_decodable {
     ( $e:ident, $( $x:ident ),* ) => {
-        impl Decodable for $e {
-            fn decode<D: Decoder>(d: &mut D) -> Result<Self, D::Error> {
+        impl ::rustc_serialize::Decodable for $e {
+            fn decode<D: ::rustc_serialize::Decoder>(d: &mut D) -> Result<Self, D::Error> {
                 let s = try!(d.read_str());
                 match &*s {
                     $(
@@ -65,3 +91,10 @@ fn decode<D: Decoder>(d: &mut D) -> Result<Self, D::Error> {
         }
     };
 }
+
+#[test]
+fn power_rounding() {
+    assert_eq!(1, round_up_to_power_of_two(1));
+    assert_eq!(64, round_up_to_power_of_two(33));
+    assert_eq!(256, round_up_to_power_of_two(256));
+}
index 316ee8ae288d2e4a1bc94d8e931da965dc560c06..688248386445ff3a458d3235b4907959915dc2d7 100644 (file)
@@ -9,3 +9,5 @@ fn_args_paren_newline = true
 struct_trailing_comma = true
 struct_lit_trailing_comma = "Vertical"
 enum_trailing_comma = true
+report_todo = "Always"
+report_fixme = "Never"