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

Split AttrArgs from MacArgs. #96209

Closed
wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

MacArgs is confusing. It has three variants. It's used in two
different cases.

  • Attributes, where all three variants are used.
  • Function-like macro calls, where only one variant is used.

This commit separates these two cases. AttrArgs keeps much of the
functionality of the old MacArgs. The new MacArgs is a greatly
streamlined version of the old MacArgs. There is a small amount of
code duplication, but I think this change improves readability. It also
lets a few unreachable code paths (e.g. unwrap/expect calls) be
removed.

The commit also renames MacDelimeter as AttrMacDelimeter, because
it's used for both AttrArgs and MacArgs. For this type, it makes
more sense to share a single type between the two cases because all
variants and methods are used in both cases.

r? @matthiaskrgr

`MacArgs` is confusing. It has three variants. It's used in two
different cases.
- Attributes, where all three variants are used.
- Function-like macro calls, where only one variant is used.

This commit separates these two cases. `AttrArgs` keeps much of the
functionality of the old `MacArgs`. The new `MacArgs` is a greatly
streamlined version of the old `MacArgs`. There is a small amount of
code duplication, but I think this change improves readability. It also
lets a few unreachable code paths (e.g. `unwrap`/`expect` calls) be
removed.

The commit also renames `MacDelimeter` as `AttrMacDelimeter`, because
it's used for both `AttrArgs` and `MacArgs`. For this type, it makes
more sense to share a single type between the two cases because all
variants and methods are used in both cases.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 19, 2022
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt.

cc @rust-lang/rustfmt

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 19, 2022
@petrochenkov
Copy link
Contributor

It's used in two different cases

Those two are the same case, that's why I introduced MacArgs in #66935.
I'll look what this PR does, but I'm generally against the idea.

@petrochenkov petrochenkov self-assigned this Apr 19, 2022
@nnethercote
Copy link
Contributor Author

In what way are they the same case?

I found this naming confusing. I also don't like it when types allow invalid values, e.g. if a macro call involves a MacArgs::Eq or MacArgs::Empty value.

@petrochenkov
Copy link
Contributor

I reviewed the changes, and I'm still against doing this.

First about the terminology - Mac here means any macro, including attributes and potentially derives when the #[derive(DeriveMacro(args...))] syntax is supported.
That's why fn-like macro calls are specifically called MacCall in AST.
So MacArgs are arguments to any kinds of macros, and they turn into the same thing when reaching proc macro interface in particular.

Second, I'd like to keep MacArgs::Empty for fn-like macro calls at least for recovery, because macro "constants" let foo = FOO!; (#66935 (comment)) are a quite reasonable thing with an obvious meaning since "word" attributes #[foo] are already long supported, there was just not enough push to adopt them officially.

This only leaves MacArgs::Eq as an impossible case, but I think it's more important to keep all the arguments unified than to eliminate it.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 21, 2022
@bors
Copy link
Contributor

bors commented Apr 28, 2022

☔ The latest upstream changes (presumably #96495) made this pull request unmergeable. Please resolve the merge conflicts.

@nnethercote nnethercote closed this May 5, 2022
@nnethercote nnethercote deleted the split-MacArgs branch May 5, 2022 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants