Skip to content

Fix incorrect gating of nonterminals in key-value attributes#85445

Closed
Aaron1011 wants to merge 1 commit intorust-lang:masterfrom
Aaron1011:cfg-eval-attr-nt
Closed

Fix incorrect gating of nonterminals in key-value attributes#85445
Aaron1011 wants to merge 1 commit intorust-lang:masterfrom
Aaron1011:cfg-eval-attr-nt

Conversation

@Aaron1011
Copy link
Contributor

@Aaron1011 Aaron1011 commented May 18, 2021

Fixes #85432

When processing a #[derive] or #[cfg_eval] attribute, we need to
re-parse our attribute target, which requires flattenting all
Nonterminals. However, this caused us to incorrectly gate on a
(flattented) nonterminal in a key-value attribute, which is supposed to
be allowed.

Since we already perform this gating during the initial parse, we
suppress it in capture_cfg mode.

@rust-highfive
Copy link
Contributor

rust-highfive commented May 18, 2021

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2021
@Aaron1011 Aaron1011 mentioned this pull request May 18, 2021
Closed
@hellow554
Copy link
Contributor

hellow554 commented May 18, 2021 •
edited
Loading

Is there a specific reason that there is no test for this? I've shown two examples [1] [2]

@Aaron1011
Fix incorrect gating of nonterminals in key-value attributes
fd1e19e 
Fixes rust-lang#85432

When processing a `#[derive]` or `#[cfg_eval]` attribute, we need to
re-parse our attribute target, which requires flattenting all
`Nonterminals`. However, this caused us to incorrectly gate on a
(flattented) nonterminal in a key-value attribute, which is supposed to
be allowed.

Since we already perform this gating during the initial parse, we
suppress it in `capture_cfg` mode.
@Aaron1011
Copy link
Contributor Author

Aaron1011 commented May 18, 2021

@hellow554 - I added a test locally, but forgot to git add it. Fixed.

@jyn514
Copy link
Member

jyn514 commented May 18, 2021

@Aaron1011 this should already be fixed by #83366, which is waiting on bors.

@jyn514
Copy link
Member

jyn514 commented May 18, 2021

Oh, I guess this is still useful for backporting to beta since #82608 landed in 1.53.0. Or another alternative is to revert #82608 in beta, not sure how hard/useful that is.

@Aaron1011
Copy link
Contributor Author

Aaron1011 commented May 18, 2021

@jyn514: Backporting this PR would be much simpler, since it's a one-line diff.

@jyn514 jyn514 added beta-nominated Nominated for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 18, 2021
@hellow554
Copy link
Contributor

hellow554 commented May 18, 2021

nevertheless, the test should be included in either PR, probably in this one, because the other one is already approved to bors

@bors
Copy link
Collaborator

bors commented May 19, 2021

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

davidtwco
davidtwco approved these changes May 19, 2021
View reviewed changes
Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, is my understanding correct that we don't want to r+ this, only backport?

@Aaron1011
Copy link
Contributor Author

Aaron1011 commented May 19, 2021

@davidtwco: That's right - now that gating has been removed entirely (since extended_key_value_attributes was stabilized), this is only needed on beta.

@Aaron1011 Aaron1011 closed this May 19, 2021
@Aaron1011 Aaron1011 reopened this May 20, 2021
@apiraino
Copy link
Contributor

apiraino commented May 20, 2021

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 20, 2021
@jyn514
Copy link
Member

jyn514 commented May 21, 2021

@Aaron1011 make you you update this to point to beta, right now the target branch is master

@Aaron1011 Aaron1011 changed the base branch from master to beta May 21, 2021 02:11
@Aaron1011 Aaron1011 changed the base branch from beta to master May 21, 2021 02:11
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 11, 2021
@Mark-Simulacrum Mark-Simulacrum added this to the 1.53.0 milestone Jun 11, 2021
@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jun 11, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 11, 2021
@bors
Auto merge of rust-lang#86225 - Mark-Simulacrum:beta-next, r=Mark-Sim…
0a0f816 
…ulacrum

[beta] backports

* Disable the machine outliner by default rust-lang#86020
* Fix incorrect gating of nonterminals in key-value attributes rust-lang#85445
* Build crtbegin.o/crtend.o from source code rust-lang#85395
* Bring back x86_64-sun-solaris target to rustup rust-lang#85252
* Preserve SyntaxContext for invalid/dummy spans in crate metadata rust-lang#85211
* [beta] backport: Remove unsound TrustedRandomAccess implementations rust-lang#86222

r? `@Mark-Simulacrum`
@emilyalbini
Copy link
Member

emilyalbini commented Jun 14, 2021

So, should this be closed?

@jyn514
Copy link
Member

jyn514 commented Jun 14, 2021

Yes.

@jyn514 jyn514 closed this Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels

beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.53.0

Development

Successfully merging this pull request may close these issues.

Nightly rejects some macro-generated doc attribute values accepted by stable