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

Formatting a macro that generates a struct with a field with a multiline attribute adds indentation forever #5489

Open
shepmaster opened this issue Aug 7, 2022 · 7 comments · May be fixed by #5518
Labels
a-macros bug Panic, non-idempotency, invalid code, etc.

Comments

@shepmaster
Copy link
Member

macro_rules! generate_the_struct {
    ($a:literal) => {
        pub struct Struct {
            #[clap(
                long = "--alpha-beta-gamma",
                env = "ALPHA_BETA_GAMMA",
                default_value = $a,
            )]
            alpha_beta_gamma: usize,
        }
    };
}

The arguments of the clap attribute will be indented for every call to cargo fmt. For example...

% for x in $(seq 10); do cargo fmt; done
% git diff
diff --git a/src/main.rs b/src/main.rs
index 1f1a25c..c1dee62 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -2,10 +2,10 @@ macro_rules! generate_the_struct {
     ($a:literal) => {
         pub struct Struct {
             #[clap(
-                long = "--alpha-beta-gamma",
-                env = "ALPHA_BETA_GAMMA",
-                default_value = $a,
-            )]
+                                                                        long = "--alpha-beta-gamma",
+                                                                        env = "ALPHA_BETA_GAMMA",
+                                                                        default_value = $a,
+                                                                    )]
             alpha_beta_gamma: usize,
         }
     };

This occurs with:

  • rustfmt 1.4.38-stable (e092d0b6 2022-07-16)
  • rustfmt 1.5.1-nightly (f6f9d5e7 2022-08-04)
@calebcartwright
Copy link
Member

Believe this is just another case like #4609 and various others

@calebcartwright calebcartwright added bug Panic, non-idempotency, invalid code, etc. a-macros labels Aug 7, 2022
@shepmaster
Copy link
Member Author

another case like #4609 and various others

Perhaps, but PR #5473 from @davidBar-On does not appear to prevent this case.

@calebcartwright
Copy link
Member

Perhaps I should've elaborated, but it was deliberate that I didn't close this issue/mark as a duplicate.

The behavior you're seeing is based on a chain of two bugs/gaps. The first is a more general class where rustfmt's attempts to convert the macro contents into something it can format fails, and there's various, unrelated macro scenarios that can trigger this. This failure is actually not terribly impactful (it just results in rustfmt deciding to leave the mac alone). However, that failure then triggers the second bug which is the one that causes the runaway indentation.

The PR you've referenced is an attempt to fix a specific type of the first class (which is different than the cause of your snippet, hence the non-effect), but doesn't attempt to address the second. Ideally we'd fix this second one, but it's proven to be rather gnarly.

Essentially, your issue could be addressed in one of two ways:

  • identify cause of this specific macro that's making rustfmt choke and address it so that rustfmt can actually format your macro
  • address the second indentation issue that occurs when rustfmt is unable to format the macro (indentation idempotence issue is resolved, but rustfmt would still be unable to format the macro)

@davidBar-On
Copy link
Contributor

... address the second indentation issue that occurs when rustfmt is unable to format the macro (indentation idempotence issue is resolved, but rustfmt would still be unable to format the macro)

A problem with this approach is that the format error is not propagated to the code that handles the macro formatting. In PR #4629 I tried to fix that, so that the format error will be propagated, but it was decided that it is better to resolve each specific cause than taking this approach.

@davidBar-On
Copy link
Contributor

The root cause of the problem is here:

if let Some(ref meta) = self.meta() {

self.meta() returns None, because an Attribute key's value is expected to be a Literal, but the $a value of default_value is parsed as Identifier.

This is not a macro only issue. The same problem happens if a const is used as a value, as the const name is not a Literal.

It seems that to resolve the issue ast::Attributes::tokens() may need to be used when ast::Attributes::meta() fails. Is there already a Rewrite for in rustfmt that may be used for that?

@davidBar-On
Copy link
Contributor

davidBar-On commented Aug 17, 2022

It seems that to resolve the issue ast::Attributes::tokens() may need to be used when ast::Attributes::meta() fails.

On a second though, a better solution is that rustc_ast::ast::Attribute::meta will support Indent, and not just Lit as an attribute value (e.g. in from_mac_args. However, I am not sure whether such change may be reasonable, as I don't know where else meta is used, and I don't know where to ask or suggest the change.

Can someone help with where such change can be discussed or suggested?

davidBar-On added a commit to davidBar-On/rustfmt that referenced this issue Aug 26, 2022
davidBar-On added a commit to davidBar-On/rustfmt that referenced this issue Aug 26, 2022
@davidBar-On
Copy link
Contributor

Submitted PR #5518 with a suggested fix, although a better approach for the fix is probably enhancing Attribute::meta().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-macros bug Panic, non-idempotency, invalid code, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants