Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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.
# for free to join this conversation on GitHub. Already have an account? # 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