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

regression: parsing change in allow()? #117058

Closed
Mark-Simulacrum opened this issue Oct 22, 2023 · 3 comments · Fixed by #117092
Closed

regression: parsing change in allow()? #117058

Mark-Simulacrum opened this issue Oct 22, 2023 · 3 comments · Fixed by #117092
Assignees
Labels
E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Milestone

Comments

@Mark-Simulacrum
Copy link
Member

https://crater-reports.s3.amazonaws.com/beta-1.74-4/beta-2023-10-21/gh/ken-matsui.diagonal-loop/log.txt

[INFO] [stdout] error: expected one of `(`, `,`, `::`, or `=`, found `-`
[INFO] [stdout]   --> src/dim2.rs:28:36
[INFO] [stdout]    |
[INFO] [stdout] 28 |         #[allow(clippy::collapsible-else-if)]
[INFO] [stdout]    |                                    ^ expected one of `(`, `,`, `::`, or `=`
@Mark-Simulacrum Mark-Simulacrum added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. regression-from-stable-to-beta Performance or correctness regression from stable to beta. labels Oct 22, 2023
@Mark-Simulacrum Mark-Simulacrum added this to the 1.74.0 milestone Oct 22, 2023
@rustbot rustbot added I-prioritize Issue: Indicates that prioritization has been requested for this issue. needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Oct 22, 2023
@Noratrieb
Copy link
Member

searched nightlies: from nightly-2023-08-01 to nightly-2023-10-10
regressed nightly: nightly-2023-09-15
searched commit range: 8142a31...ca2b74f
regressed commit: dac91a8

bisected with cargo-bisect-rustc v0.6.6

Host triple: x86_64-unknown-linux-gnu
Reproduce with:

cargo bisect-rustc 2023-08-01 --end 2023-10-10 --access github --regress error 

this bisection may look pretty nonsensical, but I did manually confirm that nightly-2023-09-14 worked and nightly-2023-09-16 didn't. I am surprised that this ever parsed, and given that there's only a single random repo that regressed, and the regressing code is a mistake, I don't think this is a problematic regression. @matthewjasper anyways.

@Noratrieb Noratrieb removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 22, 2023
@Mark-Simulacrum
Copy link
Member Author

Agreed that we don't necessarily need a fix, but we should try to make sure that this is indeed limited to this one case (i.e., it's not some broader incompatibility that may have gone uncaught in Crater)

@matthewjasper
Copy link
Contributor

matthewjasper commented Oct 23, 2023

The change here is that AST validation is now checking that the attribute is valid. Some other cases:

fn allow_if() {
    #[allow(two-words)]
    if true {} else {}
    #[allow(two-words)]
    (1);
    #[allow(two-words)]
    match 1 { _ => {} }
    #[allow(two-words)]
    while false {}
}

Marking as needs-test, with the new behavior being correct

@matthewjasper matthewjasper added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Oct 23, 2023
@apiraino apiraino removed the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Oct 23, 2023
@matthewjasper matthewjasper self-assigned this Oct 23, 2023
matthewjasper added a commit to matthewjasper/rust that referenced this issue Oct 23, 2023
matthewjasper added a commit to matthewjasper/rust that referenced this issue Oct 24, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Oct 24, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#117081 (fix typos in comments)
 - rust-lang#117091 (`OptWithInfcx` naming nits, trait bound simplifications)
 - rust-lang#117092 (Add regression test for rust-lang#117058)
 - rust-lang#117093 (Update books)
 - rust-lang#117105 (remove change-id assertion in bootstrap test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors closed this as completed in af58cda Oct 24, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 24, 2023
Rollup merge of rust-lang#117092 - matthewjasper:attribute-validation, r=compiler-errors

Add regression test for rust-lang#117058

The new behavior in nightly is correct, so add a test that it stays this way.

Closes rust-lang#117058
rcvalle pushed a commit to rcvalle/rust that referenced this issue Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants