diff --git a/src/changes.rs b/src/changes.rs index 5ed1c82b22f..97133ab67fb 100644 --- a/src/changes.rs +++ b/src/changes.rs @@ -21,6 +21,7 @@ use std::fs::File; 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 @@ impl<'a> 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)); } diff --git a/src/config.rs b/src/config.rs index 8d6fa827b66..c803a07d9d0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -10,19 +10,25 @@ 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 { diff --git a/src/default.toml b/src/default.toml index 1a7b01473f3..75ab3a60164 100644 --- a/src/default.toml +++ b/src/default.toml @@ -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 index 00000000000..9ee70e1aadc --- /dev/null +++ b/src/issues.rs @@ -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 or the MIT license +// , 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 { + 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()); +} diff --git a/src/lib.rs b/src/lib.rs index 9d80f259ebd..c8fdc2eab96 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -33,8 +33,6 @@ use rustc::session::config as rustc_config; 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 syntax::visit; 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 lists; 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>, +} + +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 @@ impl<'a> CompilerCalls<'a> for RustFmtCalls { // 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); diff --git a/src/lists.rs b/src/lists.rs index f31458fcc57..9f1006c39f0 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -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 { diff --git a/src/utils.rs b/src/utils.rs index 0e992e9f7da..c32a9293326 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -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: &mut D) -> Result { + impl ::rustc_serialize::Decodable for $e { + fn decode(d: &mut D) -> Result { let s = try!(d.read_str()); match &*s { $( @@ -65,3 +91,10 @@ macro_rules! impl_enum_decodable { } }; } + +#[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)); +} diff --git a/tests/config/small_tabs.toml b/tests/config/small_tabs.toml index 316ee8ae288..68824838644 100644 --- a/tests/config/small_tabs.toml +++ b/tests/config/small_tabs.toml @@ -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"