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

Prevent unlimitted indentations in macro body formatting #5473

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

davidBar-On
Copy link
Contributor

@davidBar-On davidBar-On commented Jul 27, 2022

Fixes #4609

Suggested fix for an issue of unlimited macro body indentation when repeating the formatting.

The root cause of the problem is that $d is set as '$' in the outer macro, and therefore the $d s parameter in the inner macro is translated to $ s which is the same as $s. However, rustfmt handles the $d s parameter to println! macro calls an two parameters and is expecting a , between them.

A similar issue exists in the following example, where the problem is now in the macro body:

macro_rules! binop {
    ($l:expr, $op:tt, $r:expr) => {
        $l $op $r
    };
}

In this case rustfmt tries to format the $l $op $r as an expression.

The proposed solution is to handle such cases of consecutive identities as one identity during formatting. E.g. the $l $op $r is handled as $l_$op_$r (or zl_zop_zr) during formatting. This is done by modifying replace_names() to handle also these cases.

One drawback of this approach is that the concatenated consecutive identities cannot be split between lines during formatting. However, I believe that this is better than having the unlimited indentation issue.

The fix does not handle _ at the beginning of a name. This is because in the existing code a _ causes termination of a $name - see this condition that does consider _ as valid char in a $name. I am not sure if this is a bug or not. In addition, this test line may also indicate that _ at the beginning of a identifier name that follows $name is legal (I don't know rust well enough ....).

Note that other cases of macro formatting failure can still cause unlimited indentations. For example rust issue 88959. The issue there is that the * is translated to the * operation which is illegal outside of an expression during the formatting.

@ytmimi
Copy link
Contributor

ytmimi commented Jul 27, 2022

Haven't had a chance to review, but I want to encourage you to read the GitHub docs on linking PRs to issuses. Basically if you write Closes #issue-number on its own line in the PR description GitHub can automatically link the PR to the issue. There are a few other verbs you can use before the issue number that GitHub will recognize.

For more examples read through the linked docs above, but also check out #5277 (comment) for an example of linking more than one issue.

Overall it's not a big deal, and we can manually link the PR to the issue, but automatically doing so is a little nicer (IMO). If the PR and issue are linked then once the PR is merged the issue will get closed! At the end of the day that'll help us keep a tidy backlog 😁

You can edit the issue description to give it a try if you want.

@davidBar-On
Copy link
Contributor Author

... if you write Closes #issue-number on its own line in the PR description GitHub can automatically link the PR to the issue.

Thanks for the advice. Done (using Fixes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested macro_rules utilising inner metavariable are infinitely indented with repeated formatting
2 participants