rustfmt icon indicating copy to clipboard operation
rustfmt copied to clipboard

Prevent unlimitted indentations in macro body formatting

Open davidBar-On opened this issue 3 years ago • 2 comments
trafficstars

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.

davidBar-On avatar Jul 27 '22 18:07 davidBar-On

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 https://github.com/rust-lang/rustfmt/pull/5277#issue-1183872992 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.

ytmimi avatar Jul 27 '22 19:07 ytmimi

... 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).

davidBar-On avatar Jul 27 '22 19:07 davidBar-On