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: ambiguous outer attributes #125199

Closed
BoxyUwU opened this issue May 17, 2024 · 8 comments
Closed

regression: ambiguous outer attributes #125199

BoxyUwU opened this issue May 17, 2024 · 8 comments
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) P-critical Critical priority 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

@BoxyUwU
Copy link
Member

BoxyUwU commented May 17, 2024

[INFO] [stdout] error: ambiguous outer attributes
[INFO] [stdout]    --> src/main.rs:284:17
[INFO] [stdout]     |
[INFO] [stdout] 284 | /                 #[cfg(feature = "rustls")]
[INFO] [stdout] 285 | |                 server = server.listen_rustls(listener, ssl_builder)?;
[INFO] [stdout]     | |_____________________________________________________________________^
[INFO] [stdout]     |

Probably #124099 cc @davidtwco

@BoxyUwU BoxyUwU 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 May 17, 2024
@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 May 17, 2024
@BoxyUwU BoxyUwU added this to the 1.79.0 milestone May 17, 2024
@saethlin saethlin added A-attributes Area: Attributes (`#[…]`, `#![…]`) and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels May 18, 2024
@davidtwco
Copy link
Member

cc @voidc as author of #124099

@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

My question here is how do we want to handle the changes in #124099. I don't see a mention of it being aware about breaking changes (PR was even rolled up).

@rustbot label -I-prioritize +P-critical

@rustbot rustbot added P-critical Critical priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels May 21, 2024
@voidc
Copy link
Contributor

voidc commented May 30, 2024

I looked into each regression. Most are caused by a dependency on rustrict (versions 0.3.13..0.5.14) which contains the following code (source):

    /// TODO: This is untested.
    #[cfg(feature = "reset_censor")]
    pub fn reset(&mut self, text: I) {
        // ...
        #[cfg(any(feature = "find_false_positives", feature = "trace"))]
        self.total_matches = 0;
        // ...
    }

I assume the author meant to apply the attribute to the whole assignment statement but here it only applies to the expression self.total_matches. Applying the attribute to the statement would have required wrapping it in braces. Since the stmt_expr_attributes feature is still unstable, the above code would normally trigger an error. However, because the reset function is conditional on the (untested) reset_censor feature, the unstable feature error was never triggered during normal compilation. In a later commit the reset function was removed.

Besides rustrict there are three more problematic crates: thoughts_server, leptos_router, and varies.
In all of them, the regression boils down to a pattern similar to rustrict.

To summarize, in each case, an attribute is applied to the left-hand side of an assignment, which most likely does not match the authors' intention. This is exactly the kind of mistake that the error introduced in #124099 is meant to prevent.

@riking
Copy link

riking commented May 30, 2024

I'm unclear whether this requires nightly to trigger?

This should probably be reverted and changed to a future compat warning.

@apiraino
Copy link
Contributor

Issue was briefly mentioned today in the t-compiler triage meeting (on Zulip).

Seems that given the timeframe leading to the next stable (2024-06-13, in 13 days), a revert would be more appropriate.

@wesleywiser
Copy link
Member

Yeah, since #[something] let foo = bar; is already accepted syntax on stable, we can't change it to be an error even if most of the places in this crater are not using it correctly. I think @riking is correct and this first needs to be made into a future compatibility warning and then a hard error over an edition boundary to preserve backwards compatibility.

bors added a commit to rust-lang-ci/rust that referenced this issue Jun 7, 2024
[beta] Revert "Disallow ambiguous attributes on expressions" on beta

As discussed in [today's t-compiler meeting](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-06/near/443079505), this reverts PR rust-lang#124099 on beta to fix P-critical beta regressions rust-lang#125199.

r? `@wesleywiser`

This is the revert of rust-lang#124099 on beta that I mentioned in rust-lang#126101, in case that's what you also wanted. Opening as draft in case it's not.

I'm not well-versed in these backports, so I hope I did it right 😓
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jun 7, 2024
Revert "Disallow ambiguous attributes on expressions" on nightly

As discussed in [today's t-compiler meeting](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-06/near/443079505), this reverts PR rust-lang#124099 to fix P-critical beta regressions rust-lang#125199.

r? `@wesleywiser`

Opening as draft so that `@wesleywiser` and `@apiraino,` you can tell me whether you wanted:
1. a `beta-accepted` revert of rust-lang#124099 on nightly (this PR)? That will need to be backported to beta (even though rust-lang#126093 may be the last of those)
2. a revert of rust-lang#124099 on beta?
3. all of the above?

I also opened rust-lang#126102, another draft PR to revert rust-lang#124099 on beta, should you choose options 2 or 3.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Jun 7, 2024
Revert "Disallow ambiguous attributes on expressions" on nightly

As discussed in [today's t-compiler meeting](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-06/near/443079505), this reverts PR rust-lang#124099 to fix P-critical beta regressions rust-lang#125199.

r? ``@wesleywiser``

Opening as draft so that ``@wesleywiser`` and ``@apiraino,`` you can tell me whether you wanted:
1. a `beta-accepted` revert of rust-lang#124099 on nightly (this PR)? That will need to be backported to beta (even though rust-lang#126093 may be the last of those)
2. a revert of rust-lang#124099 on beta?
3. all of the above?

I also opened rust-lang#126102, another draft PR to revert rust-lang#124099 on beta, should you choose options 2 or 3.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 7, 2024
Rollup merge of rust-lang#126101 - lqd:revert-124099, r=wesleywiser

Revert "Disallow ambiguous attributes on expressions" on nightly

As discussed in [today's t-compiler meeting](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-06/near/443079505), this reverts PR rust-lang#124099 to fix P-critical beta regressions rust-lang#125199.

r? ``@wesleywiser``

Opening as draft so that ``@wesleywiser`` and ``@apiraino,`` you can tell me whether you wanted:
1. a `beta-accepted` revert of rust-lang#124099 on nightly (this PR)? That will need to be backported to beta (even though rust-lang#126093 may be the last of those)
2. a revert of rust-lang#124099 on beta?
3. all of the above?

I also opened rust-lang#126102, another draft PR to revert rust-lang#124099 on beta, should you choose options 2 or 3.
@Mark-Simulacrum
Copy link
Member

Closing since fixes landed on beta/nightly: #126093

@lqd
Copy link
Member

lqd commented Jun 7, 2024

This was fixed by reverting #124099 on nightly (in #126101) and beta (in #126093).

flip1995 pushed a commit to flip1995/rust that referenced this issue Jun 13, 2024
Revert "Disallow ambiguous attributes on expressions" on nightly

As discussed in [today's t-compiler meeting](https://rust-lang.zulipchat.com/#narrow/stream/238009-t-compiler.2Fmeetings/topic/.5Bweekly.5D.202024-06-06/near/443079505), this reverts PR rust-lang#124099 to fix P-critical beta regressions rust-lang#125199.

r? ``@wesleywiser``

Opening as draft so that ``@wesleywiser`` and ``@apiraino,`` you can tell me whether you wanted:
1. a `beta-accepted` revert of rust-lang#124099 on nightly (this PR)? That will need to be backported to beta (even though rust-lang#126093 may be the last of those)
2. a revert of rust-lang#124099 on beta?
3. all of the above?

I also opened rust-lang#126102, another draft PR to revert rust-lang#124099 on beta, should you choose options 2 or 3.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-attributes Area: Attributes (`#[…]`, `#![…]`) P-critical Critical priority 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

No branches or pull requests

10 participants