From 5dafcc24e46a05713c47235d51ac2d50c7899feb Mon Sep 17 00:00:00 2001 From: topecongiro Date: Wed, 23 Aug 2017 23:20:32 +0900 Subject: [PATCH 1/2] Add merge_derives config option --- Configurations.md | 52 ++++++++++++++++------ src/config.rs | 1 + src/lists.rs | 4 +- src/visitor.rs | 44 +++++++++++++++++- tests/source/configs-merge_derives-true.rs | 10 +++++ tests/target/configs-merge_derives-true.rs | 8 ++++ 6 files changed, 102 insertions(+), 17 deletions(-) create mode 100644 tests/source/configs-merge_derives-true.rs create mode 100644 tests/target/configs-merge_derives-true.rs diff --git a/Configurations.md b/Configurations.md index 13a704240c37..373447bf24f6 100644 --- a/Configurations.md +++ b/Configurations.md @@ -1230,7 +1230,7 @@ Put a match sub-patterns' separator (`|`) in front or back. - **Default value**: `"Back"` - **Possible values**: `"Back"`, `"Front"` -#### `"Back"` +#### `"Back"`: ```rust match m { @@ -1243,7 +1243,7 @@ match m { } ``` -#### `Front` +#### `Front`: ```rust match m { @@ -1265,6 +1265,31 @@ Maximum width of each line See also [`error_on_line_overflow`](#error_on_line_overflow). +## `merge_derives` + +Merge multiple derives into a single one. + +- **Default value**: `true` +- **Possible values**: `true`, `false` + +*Note*: The merged derives will be put after all other attributes or doc comments. + +#### `true`: + +```rust +#[derive(Eq, PartialEq, Debug, Copy, Clone)] +pub enum Foo {} +``` + +#### `false`: + +```rust +#[derive(Eq, PartialEq)] +#[derive(Debug)] +#[derive(Copy, Clone)] +pub enum Foo {} +``` + ## `multiline_closure_forces_block` Force multiline closure bodies to be wrapped in a block @@ -1272,6 +1297,18 @@ Force multiline closure bodies to be wrapped in a block - **Default value**: `false` - **Possible values**: `false`, `true` +#### `true`: + +```rust + +result.and_then(|maybe_value| { + match maybe_value { + None => ..., + Some(value) => ..., + } +}) +``` + #### `false`: ```rust @@ -1281,17 +1318,6 @@ result.and_then(|maybe_value| match maybe_value { }) ``` -#### `true`: - -```rust -result.and_then(|maybe_value| { - match maybe_value { - None => ..., - Some(value) => ..., - } -}) -``` - ## `multiline_match_arm_forces_block` Force multiline match arm bodies to be wrapped in a block diff --git a/src/config.rs b/src/config.rs index 9659f19377d0..31eccf249419 100644 --- a/src/config.rs +++ b/src/config.rs @@ -619,6 +619,7 @@ create_config! { "Force multiline closure bodies to be wrapped in a block"; multiline_match_arm_forces_block: bool, false, "Force multiline match arm bodies to be wrapped in a block"; + merge_derives: bool, true, "Merge multiple `#[derive(...)]` into a single one"; } #[cfg(test)] diff --git a/src/lists.rs b/src/lists.rs index ea34cdfc4801..8787850634e4 100644 --- a/src/lists.rs +++ b/src/lists.rs @@ -19,10 +19,10 @@ use config::{Config, IndentStyle}; use rewrite::RewriteContext; use utils::{first_line_width, last_line_width, mk_sp}; -#[derive(Eq, PartialEq, Debug, Copy, Clone)] /// Formatting tactic for lists. This will be cast down to a /// DefinitiveListTactic depending on the number and length of the items and /// their comments. +#[derive(Eq, PartialEq, Debug, Copy, Clone)] pub enum ListTactic { // One item per row. Vertical, @@ -144,8 +144,8 @@ impl ListItem { } } -#[derive(Eq, PartialEq, Debug, Copy, Clone)] /// The definitive formatting tactic for lists. +#[derive(Eq, PartialEq, Debug, Copy, Clone)] pub enum DefinitiveListTactic { Vertical, Horizontal, diff --git a/src/visitor.rs b/src/visitor.rs index 49772f97e2d7..b0df0bcaf9bf 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -925,6 +925,8 @@ impl<'a> Rewrite for [ast::Attribute] { } let indent = shape.indent.to_string(context.config); + let mut derive_args = Vec::new(); + for (i, a) in self.iter().enumerate() { let a_str = try_opt!(a.rewrite(context, shape)); @@ -956,13 +958,51 @@ impl<'a> Rewrite for [ast::Attribute] { } // Write the attribute itself. - result.push_str(&a_str); + if context.config.merge_derives() { + if let Some(mut args) = get_derive_args(context, a) { + // If the attribute is `#[derive(...)]`, take the arguments and skip rewriting. + // We will merge the all arguments into a single `#[derive(...)]` at last. + derive_args.append(&mut args); + } else { + result.push_str(&a_str); - if i < self.len() - 1 { + if i < self.len() - 1 { + result.push('\n'); + } + } + } else { + result.push_str(&a_str); + + if i < self.len() - 1 { + result.push('\n'); + } + } + } + + // Add merged `#[derive(...)]` at last. + if context.config.merge_derives() && !derive_args.is_empty() { + if !result.is_empty() && !result.ends_with('\n') { + result.push_str(&indent); result.push('\n'); } + result.push_str(&format!("#[derive({})]", derive_args.join(", "))); } Some(result) } } + +/// Returns the arguments of `#[derive(...)]`. +fn get_derive_args(context: &RewriteContext, attr: &ast::Attribute) -> Option> { + attr.meta().and_then(|meta_item| match meta_item.node { + ast::MetaItemKind::List(ref args) if meta_item.name.as_str() == "derive" => { + // Every argument of `derive` should be `NestedMetaItemKind::Literal`. + Some( + args.iter() + .map(|a| context.snippet(a.span)) + .collect::>(), + ) + } + _ => None, + }) +} diff --git a/tests/source/configs-merge_derives-true.rs b/tests/source/configs-merge_derives-true.rs new file mode 100644 index 000000000000..804a48c43c05 --- /dev/null +++ b/tests/source/configs-merge_derives-true.rs @@ -0,0 +1,10 @@ +// rustfmt-merge_derives: true +// Merge multiple derives to a single one. + +#[bar] +#[derive(Eq, PartialEq)] +#[foo] +#[derive(Debug)] +#[foobar] +#[derive(Copy, Clone)] +pub enum Foo {} diff --git a/tests/target/configs-merge_derives-true.rs b/tests/target/configs-merge_derives-true.rs new file mode 100644 index 000000000000..8cf7b9a346d2 --- /dev/null +++ b/tests/target/configs-merge_derives-true.rs @@ -0,0 +1,8 @@ +// rustfmt-merge_derives: true +// Merge multiple derives to a single one. + +#[bar] +#[foo] +#[foobar] +#[derive(Eq, PartialEq, Debug, Copy, Clone)] +pub enum Foo {} From 669a139956681edf13c9fa21f08fa27979d5c345 Mon Sep 17 00:00:00 2001 From: topecongiro Date: Thu, 24 Aug 2017 23:46:22 +0900 Subject: [PATCH 2/2] Only merge consecutive derives --- Configurations.md | 2 - src/visitor.rs | 46 +++++++++++++--------- tests/source/configs-merge_derives-true.rs | 36 +++++++++++++++++ tests/target/configs-merge_derives-true.rs | 34 +++++++++++++++- 4 files changed, 96 insertions(+), 22 deletions(-) diff --git a/Configurations.md b/Configurations.md index 373447bf24f6..972c2e58241a 100644 --- a/Configurations.md +++ b/Configurations.md @@ -1272,8 +1272,6 @@ Merge multiple derives into a single one. - **Default value**: `true` - **Possible values**: `true`, `false` -*Note*: The merged derives will be put after all other attributes or doc comments. - #### `true`: ```rust diff --git a/src/visitor.rs b/src/visitor.rs index b0df0bcaf9bf..2f6a503dc136 100644 --- a/src/visitor.rs +++ b/src/visitor.rs @@ -927,7 +927,8 @@ impl<'a> Rewrite for [ast::Attribute] { let mut derive_args = Vec::new(); - for (i, a) in self.iter().enumerate() { + let mut iter = self.iter().enumerate().peekable(); + while let Some((i, a)) = iter.next() { let a_str = try_opt!(a.rewrite(context, shape)); // Write comments and blank lines between attributes. @@ -954,44 +955,51 @@ impl<'a> Rewrite for [ast::Attribute] { } else if multi_line { result.push('\n'); } - result.push_str(&indent); + if derive_args.is_empty() { + result.push_str(&indent); + } } // Write the attribute itself. + let mut insert_new_line = true; if context.config.merge_derives() { + // If the attribute is `#[derive(...)]`, take the arguments. if let Some(mut args) = get_derive_args(context, a) { - // If the attribute is `#[derive(...)]`, take the arguments and skip rewriting. - // We will merge the all arguments into a single `#[derive(...)]` at last. derive_args.append(&mut args); + match iter.peek() { + // If the next attribute is `#[derive(...)]` as well, skip rewriting. + Some(&(_, next_attr)) if is_derive(next_attr) => insert_new_line = false, + // If not, rewrite the merged derives. + _ => { + result.push_str(&format!("#[derive({})]", derive_args.join(", "))); + derive_args.clear(); + } + } } else { result.push_str(&a_str); - - if i < self.len() - 1 { - result.push('\n'); - } } } else { result.push_str(&a_str); - - if i < self.len() - 1 { - result.push('\n'); - } } - } - // Add merged `#[derive(...)]` at last. - if context.config.merge_derives() && !derive_args.is_empty() { - if !result.is_empty() && !result.ends_with('\n') { - result.push_str(&indent); + if insert_new_line && i < self.len() - 1 { result.push('\n'); } - result.push_str(&format!("#[derive({})]", derive_args.join(", "))); } - Some(result) } } +fn is_derive(attr: &ast::Attribute) -> bool { + match attr.meta() { + Some(meta_item) => match meta_item.node { + ast::MetaItemKind::List(..) => meta_item.name.as_str() == "derive", + _ => false, + }, + _ => false, + } +} + /// Returns the arguments of `#[derive(...)]`. fn get_derive_args(context: &RewriteContext, attr: &ast::Attribute) -> Option> { attr.meta().and_then(|meta_item| match meta_item.node { diff --git a/tests/source/configs-merge_derives-true.rs b/tests/source/configs-merge_derives-true.rs index 804a48c43c05..18b8443f0d7b 100644 --- a/tests/source/configs-merge_derives-true.rs +++ b/tests/source/configs-merge_derives-true.rs @@ -8,3 +8,39 @@ #[foobar] #[derive(Copy, Clone)] pub enum Foo {} + +#[derive(Eq, PartialEq)] +#[derive(Debug)] +#[foobar] +#[derive(Copy, Clone)] +pub enum Bar {} + +#[derive(Eq, PartialEq)] +#[derive(Debug)] +#[derive(Copy, Clone)] +pub enum FooBar {} + +mod foo { +#[bar] +#[derive(Eq, PartialEq)] +#[foo] +#[derive(Debug)] +#[foobar] +#[derive(Copy, Clone)] +pub enum Foo {} +} + +mod bar { +#[derive(Eq, PartialEq)] +#[derive(Debug)] +#[foobar] +#[derive(Copy, Clone)] +pub enum Bar {} +} + +mod foobar { +#[derive(Eq, PartialEq)] +#[derive(Debug)] +#[derive(Copy, Clone)] +pub enum FooBar {} +} diff --git a/tests/target/configs-merge_derives-true.rs b/tests/target/configs-merge_derives-true.rs index 8cf7b9a346d2..4d0148b1c6e2 100644 --- a/tests/target/configs-merge_derives-true.rs +++ b/tests/target/configs-merge_derives-true.rs @@ -2,7 +2,39 @@ // Merge multiple derives to a single one. #[bar] +#[derive(Eq, PartialEq)] #[foo] +#[derive(Debug)] #[foobar] -#[derive(Eq, PartialEq, Debug, Copy, Clone)] +#[derive(Copy, Clone)] pub enum Foo {} + +#[derive(Eq, PartialEq, Debug)] +#[foobar] +#[derive(Copy, Clone)] +pub enum Bar {} + +#[derive(Eq, PartialEq, Debug, Copy, Clone)] +pub enum FooBar {} + +mod foo { + #[bar] + #[derive(Eq, PartialEq)] + #[foo] + #[derive(Debug)] + #[foobar] + #[derive(Copy, Clone)] + pub enum Foo {} +} + +mod bar { + #[derive(Eq, PartialEq, Debug)] + #[foobar] + #[derive(Copy, Clone)] + pub enum Bar {} +} + +mod foobar { + #[derive(Eq, PartialEq, Debug, Copy, Clone)] + pub enum FooBar {} +}