Skip to content

Conversation

@myrrlyn
Copy link

@myrrlyn myrrlyn commented Feb 17, 2020

This commit uses the ? operator to support optional trailing commas in
macro calls without requiring the call to use the panic-message arms.

The ? macro operator was stabilized in 1.32 for the 2018 edition,
and 1.37 for the 2015 edition. Applying this commit requires raising
the crate MSRV to one of these two versions; for the lower, the crate
must be explicitly set to the edition = "2018" key in its manifest.

This commit uses the `?` operator to support optional trailing commas in
macro calls without requiring the call to use the panic-message arms.

The `?` macro operator was stabilized in `1.32` for the 2018 edition,
and `1.37` for the 2015 edition. Applying this commit requires raising
the crate MSRV to one of these two versions; for the lower, the crate
must be explicitly set to the `edition = "2018"` key in its manifest.
@myrrlyn
Copy link
Author

myrrlyn commented Feb 17, 2020

I wrote this patch because I noticed that my rustfmt settings were causing macro calls to break. Insertion of the trailing commas forced the macro expansion into the arms expecting a panic message.

As noted, it forces an MSRV. Please feel completely free to reject the PR if this is unacceptable.

@murarth
Copy link
Owner

murarth commented Feb 19, 2020

I have to say, I'm not persuaded that there's a good reason to accept this proposed change and I'm inclined to reject it.

It seems to me that rustfmt, as a rule, should not change the tokens within a macro invocation. Indeed, it seems that the default settings of rustfmt do not change macro tokens. Further, the matches macro recently added to std does not accept a trailing comma (although that could change between now and the time of its stabilization).

@myrrlyn
Copy link
Author

myrrlyn commented Feb 19, 2020

My motivation for submitting this change was predicated solely on the fact that all the std macros started out without trailing comma support, and then added it in 1.26. I elected to use the ? operator instead of duplicate arms only for simplicity. If you're opposed to the requisite MSRV that imposes, I'm happy to rewrite to the std style; if you're opposed syntactically, I have absolutely no hard feelings about closing this.

@nothingelsematters
Copy link

I've stumbled upon this issue recently. I have to notice that the matches! std macro already accepts a trailing comma. Moreover, I do not see any reason not to accept an optional trailing comma here, I suppose that it should be the user's choice which code style to use.

@DavidAntliff
Copy link

I'm glad I found this issue because I just couldn't work out the problem from the macro expansion. Trailing commas are very common and the error message doesn't help at all.

This was fixed for assert_eq! back in 2017: rust-lang/rust#39369

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants