Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add skip_macro_invocations option #5347

Merged
merged 11 commits into from
Jul 13, 2022
56 changes: 56 additions & 0 deletions Configurations.md
Original file line number Diff line number Diff line change
Expand Up @@ -1014,6 +1014,62 @@ macro_rules! foo {

See also [`format_macro_matchers`](#format_macro_matchers).

## `skip_macro_invocations`

Skip formatting the bodies of macro invocations with the following names.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wondering if we could update the help text here to be a little more descriptive. Something like:

rustfmt will not format any macro invocation for macros who's names are set in this list. Setting this value to "*" will prevent any macro invocations from being formatted.

Note: This option does not have any impact on how rustfmt formats macro definitions.

Copy link
Member

@calebcartwright calebcartwright Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this for the opening line?

Suggested change
Skip formatting the bodies of macro invocations with the following names.
Specify the names of macro invocations that rustfmt should always skip, regardless of which delimiters are used.


rustfmt will not format any macro invocation for macros with names set in this list.
Including the special value "*" will prevent any macro invocations from being formatted.

Note: This option does not have any impact on how rustfmt formats macro definitions.

- **Default value**: `[]`
- **Possible values**: a list of macro name idents, `["name_0", "name_1", ..., "*"]`
- **Stable**: No (tracking issue: [#5346](https:/rust-lang/rustfmt/issues/5346))

#### `[]` (default):

rustfmt will follow its standard approach to formatting macro invocations.

No macro invocations will be skipped based on their name. More information about rustfmt's standard macro invocation formatting behavior can be found in [#5437](https:/rust-lang/rustfmt/discussions/5437).

```rust
lorem!(
const _: u8 = 0;
);

ipsum!(
const _: u8 = 0;
);
```

#### `["lorem"]`:

The named macro invocations will be skipped.

```rust
lorem!(
const _: u8 = 0;
);

ipsum!(
const _: u8 = 0;
);
```

#### `["*"]`:

The special selector `*` will skip all macro invocations.

```rust
lorem!(
const _: u8 = 0;
);

ipsum!(
const _: u8 = 0;
);
```

## `format_strings`

Expand Down
7 changes: 7 additions & 0 deletions src/config/config_type.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::config::file_lines::FileLines;
use crate::config::macro_names::MacroSelectors;
use crate::config::options::{IgnoreList, WidthHeuristics};

/// Trait for types that can be used in `Config`.
Expand Down Expand Up @@ -46,6 +47,12 @@ impl ConfigType for FileLines {
}
}

impl ConfigType for MacroSelectors {
fn doc_hint() -> String {
String::from("[<string>, ...]")
}
}

impl ConfigType for WidthHeuristics {
fn doc_hint() -> String {
String::new()
Expand Down
118 changes: 118 additions & 0 deletions src/config/macro_names.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
//! This module contains types and functions to support formatting specific macros.

use itertools::Itertools;
use std::{fmt, str};

use serde::{Deserialize, Serialize};
use serde_json as json;
use thiserror::Error;

/// Defines the name of a macro.
#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, Deserialize, Serialize)]
pub struct MacroName(String);

impl MacroName {
pub fn new(other: String) -> Self {
Self(other)
}
}

impl fmt::Display for MacroName {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
self.0.fmt(f)
}
}

impl From<MacroName> for String {
fn from(other: MacroName) -> Self {
other.0
}
}

/// Defines a selector to match against a macro.
#[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd, Deserialize, Serialize)]
pub enum MacroSelector {
Name(MacroName),
All,
}

impl fmt::Display for MacroSelector {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::Name(name) => name.fmt(f),
Self::All => write!(f, "*"),
}
}
}

impl str::FromStr for MacroSelector {
type Err = std::convert::Infallible;

fn from_str(s: &str) -> Result<Self, Self::Err> {
Ok(match s {
"*" => MacroSelector::All,
name => MacroSelector::Name(MacroName(name.to_owned())),
})
}
}

/// A set of macro selectors.
#[derive(Clone, Debug, Default, PartialEq, Deserialize, Serialize)]
pub struct MacroSelectors(pub Vec<MacroSelector>);

impl fmt::Display for MacroSelectors {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
write!(f, "{}", self.0.iter().format(", "))
}
}

#[derive(Error, Debug)]
pub enum MacroSelectorsError {
#[error("{0}")]
Json(json::Error),
}

// This impl is needed for `Config::override_value` to work for use in tests.
impl str::FromStr for MacroSelectors {
type Err = MacroSelectorsError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
let raw: Vec<&str> = json::from_str(s).map_err(MacroSelectorsError::Json)?;
Ok(Self(
raw.into_iter()
.map(|raw| {
MacroSelector::from_str(raw).expect("MacroSelector from_str is infallible")
})
.collect(),
))
}
}

#[cfg(test)]
mod test {
use super::*;
use std::str::FromStr;

#[test]
fn macro_names_from_str() {
let macro_names = MacroSelectors::from_str(r#"["foo", "*", "bar"]"#).unwrap();
assert_eq!(
macro_names,
MacroSelectors(
[
MacroSelector::Name(MacroName("foo".to_owned())),
MacroSelector::All,
MacroSelector::Name(MacroName("bar".to_owned()))
]
.into_iter()
.collect()
)
);
}

#[test]
fn macro_names_display() {
let macro_names = MacroSelectors::from_str(r#"["foo", "*", "bar"]"#).unwrap();
assert_eq!(format!("{}", macro_names), "foo, *, bar");
}
}
20 changes: 20 additions & 0 deletions src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ pub use crate::config::file_lines::{FileLines, FileName, Range};
#[allow(unreachable_pub)]
pub use crate::config::lists::*;
#[allow(unreachable_pub)]
pub use crate::config::macro_names::{MacroSelector, MacroSelectors};
#[allow(unreachable_pub)]
pub use crate::config::options::*;

#[macro_use]
Expand All @@ -22,6 +24,7 @@ pub(crate) mod options;

pub(crate) mod file_lines;
pub(crate) mod lists;
pub(crate) mod macro_names;

// This macro defines configuration options used in rustfmt. Each option
// is defined as follows:
Expand Down Expand Up @@ -67,6 +70,8 @@ create_config! {
format_macro_matchers: bool, false, false,
"Format the metavariable matching patterns in macros";
format_macro_bodies: bool, true, false, "Format the bodies of macros";
skip_macro_invocations: MacroSelectors, MacroSelectors::default(), false,
"Skip formatting the bodies of macros invoked with the following names.";
hex_literal_case: HexLiteralCase, HexLiteralCase::Preserve, false,
"Format hexadecimal integer literals";

Expand Down Expand Up @@ -403,6 +408,7 @@ mod test {
use super::*;
use std::str;

use crate::config::macro_names::MacroName;
use rustfmt_config_proc_macro::{nightly_only_test, stable_only_test};

#[allow(dead_code)]
Expand Down Expand Up @@ -611,6 +617,7 @@ normalize_doc_attributes = false
format_strings = false
format_macro_matchers = false
format_macro_bodies = true
skip_macro_invocations = []
hex_literal_case = "Preserve"
empty_item_single_line = true
struct_lit_single_line = true
Expand Down Expand Up @@ -1019,4 +1026,17 @@ make_backup = false
);
}
}

#[test]
fn test_override_skip_macro_invocations() {
let mut config = Config::default();
config.override_value("skip_macro_invocations", r#"["*", "println"]"#);
assert_eq!(
config.skip_macro_invocations(),
MacroSelectors(vec![
MacroSelector::All,
MacroSelector::Name(MacroName::new("println".to_owned()))
])
);
}
}
10 changes: 9 additions & 1 deletion src/skip.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use rustc_ast_pretty::pprust;
/// by other context. Query this context to know if you need skip a block.
#[derive(Default, Clone)]
pub(crate) struct SkipContext {
pub(crate) all_macros: bool,
macros: Vec<String>,
Comment on lines +10 to 11
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like this structure, I think it should be refactored into something like

enum SkipMacroContext {
    All,
    Named(HashSet<MacroName>),
}

pub(crate) struct SkipContext {
    macros: SkipMacroContext,
    attributes: HashSet<String>,
}

but thought that was probably out of scope for this PR. Happy to open a followup afterwards, or include as an additional commit if you think it's okay to add here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the PR is in a pretty good state and I'm leaning towards making this change in a follow up PR. If you feel strongly about it though and don't think it will add a lot of extra work to the current PR, then I'll leave that call up to you

attributes: Vec<String>,
}
Expand All @@ -23,8 +24,15 @@ impl SkipContext {
self.attributes.append(&mut other.attributes);
}

pub(crate) fn update_macros<T>(&mut self, other: T)
where
T: IntoIterator<Item = String>,
{
self.macros.extend(other.into_iter());
}

pub(crate) fn skip_macro(&self, name: &str) -> bool {
self.macros.iter().any(|n| n == name)
self.all_macros || self.macros.iter().any(|n| n == name)
}

pub(crate) fn skip_attribute(&self, name: &str) -> bool {
Expand Down
7 changes: 6 additions & 1 deletion src/test/configuration_snippet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,13 @@ impl ConfigurationSection {
lazy_static! {
static ref CONFIG_NAME_REGEX: regex::Regex =
regex::Regex::new(r"^## `([^`]+)`").expect("failed creating configuration pattern");
// Configuration values, which will be passed to `from_str`:
//
// - must be prefixed with `####`
// - must be wrapped in backticks
// - may by wrapped in double quotes (which will be stripped)
tommilligan marked this conversation as resolved.
Show resolved Hide resolved
static ref CONFIG_VALUE_REGEX: regex::Regex =
regex::Regex::new(r#"^#### `"?([^`"]+)"?`"#)
regex::Regex::new(r#"^#### `"?([^`]+?)"?`"#)
.expect("failed creating configuration value pattern");
}

Expand Down
13 changes: 11 additions & 2 deletions src/visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_span::{symbol, BytePos, Pos, Span};
use crate::attr::*;
use crate::comment::{contains_comment, rewrite_comment, CodeCharKind, CommentCodeSlices};
use crate::config::Version;
use crate::config::{BraceStyle, Config};
use crate::config::{BraceStyle, Config, MacroSelector};
use crate::coverage::transform_missing_snippet;
use crate::items::{
format_impl, format_trait, format_trait_alias, is_mod_decl, is_use_item, rewrite_extern_crate,
Expand Down Expand Up @@ -770,6 +770,15 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
snippet_provider: &'a SnippetProvider,
report: FormatReport,
) -> FmtVisitor<'a> {
let mut skip_context = SkipContext::default();
let mut macro_names = Vec::new();
for macro_selector in config.skip_macro_invocations().0 {
match macro_selector {
MacroSelector::Name(name) => macro_names.push(name.to_string()),
MacroSelector::All => skip_context.all_macros = true,
}
}
skip_context.update_macros(macro_names);
FmtVisitor {
parent_context: None,
parse_sess: parse_session,
Expand All @@ -784,7 +793,7 @@ impl<'b, 'a: 'b> FmtVisitor<'a> {
is_macro_def: false,
macro_rewrite_failure: false,
report,
skip_context: Default::default(),
skip_context,
}
}

Expand Down
11 changes: 11 additions & 0 deletions tests/source/skip_macro_invocations/all.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// rustfmt-skip_macro_invocations: ["*"]

// Should skip this invocation
items!(
const _: u8 = 0;
);

// Should skip this invocation
renamed_items!(
const _: u8 = 0;
);
11 changes: 11 additions & 0 deletions tests/source/skip_macro_invocations/all_and_name.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// rustfmt-skip_macro_invocations: ["*","items"]

// Should skip this invocation
items!(
const _: u8 = 0;
);

// Should also skip this invocation, as the wildcard covers it
renamed_items!(
const _: u8 = 0;
);
11 changes: 11 additions & 0 deletions tests/source/skip_macro_invocations/empty.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// rustfmt-skip_macro_invocations: []

// Should not skip this invocation
items!(
const _: u8 = 0;
);

// Should not skip this invocation
renamed_items!(
const _: u8 = 0;
);
11 changes: 11 additions & 0 deletions tests/source/skip_macro_invocations/name.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// rustfmt-skip_macro_invocations: ["items"]

// Should skip this invocation
items!(
const _: u8 = 0;
);

// Should not skip this invocation
renamed_items!(
const _: u8 = 0;
);
6 changes: 6 additions & 0 deletions tests/source/skip_macro_invocations/name_unknown.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// rustfmt-skip_macro_invocations: ["unknown"]

// Should not skip this invocation
items!(
const _: u8 = 0;
);
Loading