Switch to accessing config items via method.

Preparation for #865, which proposes adding a flag which outputs which
config options are used during formatting.

This PR should not make any difference to functionality. A lot of this
was search-and-replace.

Some areas worthy of review/discussion:

 - The method for each config item returns a clone of the underlying
   value. We can't simply return an immutable reference, as lots of
   places in the code expect to be able to pass the returned value as
   `bool` (not `&bool). It would be nice if the `bool` items could
   return a copy, but the more complex types a borrowed reference... but
   unfortunately, I couldn't get the macro to do this.
 - A few places (mostly tests and `src/bin/rustfmt.rs`) were overriding
   config items by modifying the fields of the `Config` struct directly.
   They now use the existing `override_value()` method, which has been
   modified to return a `Result` for use by `src/bin/rustfmt.rs`. This
   benefits of this are that the complex `file_lines` and `write_mode`
   strings are now parsed in one place (`Config.override_value`) instead
   of multiple. The disadvantages are that it moves the compile-time
   checks for config names to become run-time checks.

Contributing.md

@@ -207,6 +207,6 @@ handling of configuration options is done in [src/config.rs](src/config.rs). Loo
 `create_config!` macro at the end of the file for all the options. The rest of
 the file defines a bunch of enums used for options, and the machinery to produce
 the config struct and parse a config file, etc. Checking an option is done by
-accessing the correct field on the config struct, e.g., `config.max_width`. Most
+accessing the correct field on the config struct, e.g., `config.max_width()`. Most
 functions have a `Config`, or one can be accessed via a visitor or context of
 some kind.

Design.md

@@ -150,7 +150,7 @@ for its configuration.
 
 Our visitor keeps track of the desired current indent due to blocks (
 `block_indent`). Each `visit_*` method reformats code according to this indent,
-`config.comment_width` and `config.max_width`. Most reformatting done in the
+`config.comment_width()` and `config.max_width()`. Most reformatting done in the
 `visit_*` methods is a bit hackey and is meant to be temporary until it can be
 done properly.
 

src/bin/rustfmt.rs

@@ -18,14 +18,12 @@ extern crate env_logger;
 extern crate getopts;
 
 use rustfmt::{run, Input, Summary};
-use rustfmt::file_lines::FileLines;
-use rustfmt::config::{Config, WriteMode};
+use rustfmt::config::Config;
 
 use std::{env, error};
 use std::fs::{self, File};
 use std::io::{self, ErrorKind, Read, Write};
 use std::path::{Path, PathBuf};
-use std::str::FromStr;
 
 use getopts::{Matches, Options};
 
@@ -63,8 +61,8 @@ enum Operation {
 struct CliOptions {
     skip_children: bool,
     verbose: bool,
-    write_mode: Option<WriteMode>,
-    file_lines: FileLines, // Default is all lines in all files.
+    write_mode: Option<String>,
+    file_lines: Option<String>,
 }
 
 impl CliOptions {
@@ -73,28 +71,29 @@ impl CliOptions {
         options.skip_children = matches.opt_present("skip-children");
         options.verbose = matches.opt_present("verbose");
 
-        if let Some(ref write_mode) = matches.opt_str("write-mode") {
-            if let Ok(write_mode) = WriteMode::from_str(write_mode) {
-                options.write_mode = Some(write_mode);
-            } else {
-                return Err(FmtError::from(format!("Invalid write-mode: {}", write_mode)));
-            }
+        if let Some(write_mode) = matches.opt_str("write-mode") {
+            options.write_mode = Some(write_mode);
         }
 
-        if let Some(ref file_lines) = matches.opt_str("file-lines") {
-            options.file_lines = file_lines.parse()?;
+        if let Some(file_lines) = matches.opt_str("file-lines") {
+            options.file_lines = Some(file_lines);
         }
 
         Ok(options)
     }
 
-    fn apply_to(self, config: &mut Config) {
-        config.skip_children = self.skip_children;
-        config.verbose = self.verbose;
-        config.file_lines = self.file_lines;
-        if let Some(write_mode) = self.write_mode {
-            config.write_mode = write_mode;
+    fn apply_to(&self, config: &mut Config) -> FmtResult<()> {
+        let bool_to_str = |b| if b { "true" } else { "false" };
+        config
+            .override_value("skip_children", bool_to_str(self.skip_children))?;
+        config.override_value("verbose", bool_to_str(self.verbose))?;
+        if let Some(ref write_mode) = self.write_mode {
+            config.override_value("write_mode", &write_mode)?;
         }
+        if let Some(ref file_lines) = self.file_lines {
+            config.override_value("file_lines", &file_lines)?;
+        }
+        Ok(())
     }
 }
 
@@ -222,12 +221,12 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
                                                          &env::current_dir().unwrap())?;
 
             // write_mode is always Plain for Stdin.
-            config.write_mode = WriteMode::Plain;
+            config.override_value("write_mode", "Plain")?;
 
             // parse file_lines
             if let Some(ref file_lines) = matches.opt_str("file-lines") {
-                config.file_lines = file_lines.parse()?;
-                for f in config.file_lines.files() {
+                config.override_value("file-lines", file_lines)?;
+                for f in config.file_lines().files() {
                     if f != "stdin" {
                         println!("Warning: Extra file listed in file_lines option '{}'", f);
                     }
@@ -239,12 +238,6 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
         Operation::Format { files, config_path } => {
             let options = CliOptions::from_matches(&matches)?;
 
-            for f in options.file_lines.files() {
-                if !files.contains(&PathBuf::from(f)) {
-                    println!("Warning: Extra file listed in file_lines option '{}'", f);
-                }
-            }
-
             let mut config = Config::default();
             let mut path = None;
             // Load the config path file if provided
@@ -253,6 +246,13 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
                 config = cfg_tmp;
                 path = path_tmp;
             };
+            options.apply_to(&mut config)?;
+
+            for f in config.file_lines().files() {
+                if !files.contains(&PathBuf::from(f)) {
+                    println!("Warning: Extra file listed in file_lines option '{}'", f);
+                }
+            }
 
             if options.verbose {
                 if let Some(path) = path.as_ref() {
@@ -282,7 +282,7 @@ fn execute(opts: &Options) -> FmtResult<Summary> {
                         config = config_tmp;
                     }
 
-                    options.clone().apply_to(&mut config);
+                    options.apply_to(&mut config)?;
                     error_summary.add(run(Input::File(file), &config));
                 }
             }

src/chains.rs

@@ -114,23 +114,23 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
     };
     let (nested_shape, extend) = if !parent_rewrite_contains_newline && is_continuable(&parent) {
         let nested_shape = if first_subexpr_is_try {
-            parent_shape.block_indent(context.config.tab_spaces)
+            parent_shape.block_indent(context.config.tab_spaces())
         } else {
             chain_indent(context, shape.add_offset(parent_rewrite.len()))
         };
         (nested_shape,
-         context.config.chain_indent == IndentStyle::Visual ||
-         parent_rewrite.len() <= context.config.tab_spaces)
+         context.config.chain_indent() == IndentStyle::Visual ||
+         parent_rewrite.len() <= context.config.tab_spaces())
     } else if is_block_expr(&parent, &parent_rewrite) {
         // The parent is a block, so align the rest of the chain with the closing
         // brace.
         (parent_shape, false)
     } else if parent_rewrite_contains_newline {
         (chain_indent(context,
-                      parent_shape.block_indent(context.config.tab_spaces)),
+                      parent_shape.block_indent(context.config.tab_spaces())),
          false)
     } else {
-        (shape.block_indent(context.config.tab_spaces), false)
+        (shape.block_indent(context.config.tab_spaces()), false)
     };
 
     let max_width = try_opt!((shape.width + shape.indent.width() + shape.offset)
@@ -143,14 +143,14 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
     };
     let first_child_shape = if extend {
         let mut shape = try_opt!(parent_shape.offset_left(last_line_width(&parent_rewrite)));
-        match context.config.chain_indent {
+        match context.config.chain_indent() {
             IndentStyle::Visual => shape,
             IndentStyle::Block => {
                 shape.offset = shape
                     .offset
-                    .checked_sub(context.config.tab_spaces)
+                    .checked_sub(context.config.tab_spaces())
                     .unwrap_or(0);
-                shape.indent.block_indent += context.config.tab_spaces;
+                shape.indent.block_indent += context.config.tab_spaces();
                 shape
             }
         }
@@ -176,7 +176,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
         .fold(0, |a, b| a + first_line_width(b)) + parent_rewrite.len();
     let one_line_len = rewrites.iter().fold(0, |a, r| a + r.len()) + parent_rewrite.len();
 
-    let veto_single_line = if one_line_len > context.config.chain_one_line_max {
+    let veto_single_line = if one_line_len > context.config.chain_one_line_max() {
         if rewrites.len() > 1 {
             true
         } else if rewrites.len() == 1 {
@@ -185,7 +185,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
         } else {
             false
         }
-    } else if context.config.take_source_hints && subexpr_list.len() > 1 {
+    } else if context.config.take_source_hints() && subexpr_list.len() > 1 {
         // Look at the source code. Unless all chain elements start on the same
         // line, we won't consider putting them on a single line either.
         let last_span = context.snippet(mk_sp(subexpr_list[1].span.hi, total_span.hi));
@@ -214,7 +214,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
                                                          shape) {
                         // If the first line of the last method does not fit into a single line
                         // after the others, allow new lines.
-                        almost_total + first_line_width(&last[0]) < context.config.max_width
+                        almost_total + first_line_width(&last[0]) < context.config.max_width()
                     } else {
                         false
                     }
@@ -242,7 +242,7 @@ pub fn rewrite_chain(expr: &ast::Expr, context: &RewriteContext, shape: Shape) -
                      parent_rewrite,
                      first_connector,
                      join_rewrites(&rewrites, &subexpr_list, &connector)),
-             context.config.max_width,
+             context.config.max_width(),
              shape)
 }
 
@@ -320,9 +320,9 @@ fn make_subexpr_list(expr: &ast::Expr, context: &RewriteContext) -> (ast::Expr,
 }
 
 fn chain_indent(context: &RewriteContext, shape: Shape) -> Shape {
-    match context.config.chain_indent {
+    match context.config.chain_indent() {
         IndentStyle::Visual => shape.visual_indent(0),
-        IndentStyle::Block => shape.block_indent(context.config.tab_spaces),
+        IndentStyle::Block => shape.block_indent(context.config.tab_spaces()),
     }
 }
 
@@ -372,7 +372,7 @@ fn pop_expr_chain(expr: &ast::Expr, context: &RewriteContext) -> Option<ast::Exp
 
 fn convert_try(expr: &ast::Expr, context: &RewriteContext) -> ast::Expr {
     match expr.node {
-        ast::ExprKind::Mac(ref mac) if context.config.use_try_shorthand => {
+        ast::ExprKind::Mac(ref mac) if context.config.use_try_shorthand() => {
             if let Some(subexpr) = convert_try_mac(mac, context) {
                 subexpr
             } else {
@@ -428,7 +428,7 @@ fn rewrite_method_call(method_name: ast::Ident,
         let type_list: Vec<_> =
             try_opt!(types.iter().map(|ty| ty.rewrite(context, shape)).collect());
 
-        let type_str = if context.config.spaces_within_angle_brackets && type_list.len() > 0 {
+        let type_str = if context.config.spaces_within_angle_brackets() && type_list.len() > 0 {
             format!("::< {} >", type_list.join(", "))
         } else {
             format!("::<{}>", type_list.join(", "))

src/comment.rs

@@ -38,23 +38,23 @@ pub fn rewrite_comment(orig: &str,
                        config: &Config)
                        -> Option<String> {
     // If there are lines without a starting sigil, we won't format them correctly
-    // so in that case we won't even re-align (if !config.normalize_comments) and
+    // so in that case we won't even re-align (if !config.normalize_comments()) and
     // we should stop now.
     let num_bare_lines = orig.lines()
         .map(|line| line.trim())
         .filter(|l| !(l.starts_with('*') || l.starts_with("//") || l.starts_with("/*")))
         .count();
-    if num_bare_lines > 0 && !config.normalize_comments {
+    if num_bare_lines > 0 && !config.normalize_comments() {
         return Some(orig.to_owned());
     }
 
-    if !config.normalize_comments && !config.wrap_comments {
+    if !config.normalize_comments() && !config.wrap_comments() {
         return light_rewrite_comment(orig, shape.indent, config);
     }
 
     let (opener, closer, line_start) = if block_style {
         ("/* ", " */", " * ")
-    } else if !config.normalize_comments {
+    } else if !config.normalize_comments() {
         if orig.starts_with("/**") && !orig.starts_with("/**/") {
             ("/** ", " **/", " ** ")
         } else if orig.starts_with("/*!") {
@@ -128,7 +128,7 @@ pub fn rewrite_comment(orig: &str,
             result.push_str(line_start);
         }
 
-        if config.wrap_comments && line.len() > max_chars {
+        if config.wrap_comments() && line.len() > max_chars {
             let rewrite = rewrite_string(line, &fmt).unwrap_or(line.to_owned());
             result.push_str(&rewrite);
         } else {
@@ -579,7 +579,7 @@ pub fn recover_comment_removed(new: String,
     if changed_comment_content(&snippet, &new) {
         // We missed some comments
         // Keep previous formatting if it satisfies the constrains
-        wrap_str(snippet, context.config.max_width, shape)
+        wrap_str(snippet, context.config.max_width(), shape)
     } else {
         Some(new)
     }
@@ -731,8 +731,10 @@ mod test {
     #[cfg_attr(rustfmt, rustfmt_skip)]
     fn format_comments() {
         let mut config: ::config::Config = Default::default();
-        config.wrap_comments = true;
-        config.normalize_comments = true;
+        config.override_value("wrap_comments", "true")
+            .expect("Could not set wrap_comments to true");
+        config.override_value("normalize_comments", "true")
+            .expect("Could not set normalize_comments to true");
 
         let comment = rewrite_comment(" //test",
                                       true,

src/config.rs

@@ -10,6 +10,9 @@
 
 extern crate toml;
 
+use std::error;
+use std::result;
+
 use file_lines::FileLines;
 use lists::{SeparatorTactic, ListTactic};
 
@@ -212,7 +215,7 @@ macro_rules! create_config {
     ($($i:ident: $ty:ty, $def:expr, $( $dstring:expr ),+ );+ $(;)*) => (
         #[derive(Deserialize, Clone)]
         pub struct Config {
-            $(pub $i: $ty),+
+            $($i: $ty),+
         }
 
         // Just like the Config struct but with each property wrapped
@@ -227,6 +230,12 @@ macro_rules! create_config {
 
         impl Config {
 
+            $(
+            pub fn $i(&self) -> $ty {
+                self.$i.clone()
+            }
+            )+
+
             fn fill_from_parsed_config(mut self, parsed: ParsedConfig) -> Config {
             $(
                 if let Some(val) = parsed.$i {
@@ -270,19 +279,16 @@ macro_rules! create_config {
                 }
             }
 
-            pub fn override_value(&mut self, key: &str, val: &str) {
+            pub fn override_value(&mut self, key: &str, val: &str)
+                -> result::Result<(), Box<error::Error + Send + Sync>>
+            {
                 match key {
                     $(
-                        stringify!($i) => {
-                            self.$i = val.parse::<$ty>()
-                                .expect(&format!("Failed to parse override for {} (\"{}\") as a {}",
-                                                 stringify!($i),
-                                                 val,
-                                                 stringify!($ty)));
-                        }
+                        stringify!($i) => self.$i = val.parse::<$ty>()?,
                     )+
                     _ => panic!("Unknown config key in override: {}", key)
                 }
+                Ok(())
             }
 
             pub fn print_docs() {