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

Open
BoxyUwU opened this issue May 17, 2024 · 5 comments
Open

regression: ambiguous outer attributes #125199

BoxyUwU opened this issue May 17, 2024 · 5 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.

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

7 participants