-
Notifications
You must be signed in to change notification settings - Fork 909
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
Changes from 10 commits
d2422de
cc9fb29
342925a
3b60f3e
c4f84a8
a88ec63
89ec6d1
bbce6ee
4bca420
d4d5381
7fd8be2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1014,6 +1014,60 @@ 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. | ||||||
|
||||||
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://github.com/rust-lang/rustfmt/issues/5346)) | ||||||
|
||||||
#### `[]` (default): | ||||||
|
||||||
All macro invocations will be formatted. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it will probably also be worth mentioning and reinforcing rustfmt's macro formatting behavior anyway. For example, calls that use brace delimiters are essentially already skipped anyway, while calls with paren or bracket delims are attempted to be formatted if some other conditions hold true. It may not make sense to try to articulate the totality of that behavior here, but if we can come up with a simple way to reference that we can (later) add a link that has more details There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually went ahead and put up #5437 with a quick and dirty explanation that I think can suffice for now to give us something to point to here. Feel free to wordsmith, but I'd suggest we phrase this something like:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion and the writeup as well, that's nice and clear. Updated the configurations doc |
||||||
|
||||||
```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` | ||||||
|
||||||
|
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"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>, | ||
} | ||
|
@@ -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 { | ||
|
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; | ||
); |
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; | ||
); |
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; | ||
); |
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; | ||
); |
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; | ||
); |
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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?