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

Handle rustc_const_stable attribute in library feature collector #95354

Merged
merged 4 commits into from
Apr 2, 2022

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Mar 27, 2022

The library feature collector in compiler/rustc_passes/src/lib_features.rs has only been looking at #[stable(…)], #[unstable(…)], and #[rustc_const_unstable(…)] attributes, while ignoring #[rustc_const_stable(…)]. The consequences of this were:

  • When any const feature got stabilized (changing one or more rustc_const_unstable to rustc_const_stable), users who had previously enabled that unstable feature using #![feature(…)] would get told "unknown feature", rather than rustc's nicer "the feature … has been stable since … and no longer requires an attribute to enable".

    This can be seen in the way that Stabilize const_ptr_offset #93957 (comment) failed after rebase:

    error[E0635]: unknown feature `const_ptr_offset`
      --> $DIR/offset_from_ub.rs:1:35
       |
    LL | #![feature(const_ptr_offset_from, const_ptr_offset)]
       |                                   ^^^^^^^^^^^^^^^^
  • We weren't enforcing that a particular feature is either stable everywhere or unstable everywhere, and that a feature that has been stabilized has the same stabilization version everywhere, both of which we enforce for the other stability attributes.

This PR updates the library feature collector to handle rustc_const_stable, and fixes places in the standard library and test suite where rustc_const_stable was being used in a way that does not meet the rules for a stability attribute.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Mar 27, 2022
@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(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 Mar 27, 2022
@dtolnay dtolnay removed the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Mar 28, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Mar 30, 2022

cc @jhpratt you've been working on related changes

@m-ou-se m-ou-se removed their assignment Mar 30, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Mar 30, 2022

r? rust-lang/compiler

@jhpratt
Copy link
Member

jhpratt commented Mar 30, 2022

While I'm not on the list of reviewers, I'll still take a look later today.

Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

Change the the compiler looks good as-is. There's two places where I do have questions, though.

@@ -2,7 +2,7 @@
#![feature(repr_simd)]
#![feature(platform_intrinsics)]
#![feature(staged_api)]
#![stable(feature = "foo", since = "1.33.7")]
#![stable(feature = "foo", since = "1.3.37")]
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

error[E0711]: feature `foo` is declared stable since 1.3.37, but was previously declared stable since 1.33.7
  --> /git/rust/src/test/ui/consts/const-eval/simd/insert_extract.rs:15:5
   |
LL |     #[rustc_const_stable(feature = "foo", since = "1.3.37")]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0711]: feature `foo` is declared stable since 1.3.37, but was previously declared stable since 1.33.7
  --> /git/rust/src/test/ui/consts/const-eval/simd/insert_extract.rs:17:5
   |
LL |     #[rustc_const_stable(feature = "foo", since = "1.3.37")]
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

Copy link
Member

Choose a reason for hiding this comment

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

Ah, didn't look at the full context.

@@ -905,7 +905,7 @@ impl<T> MaybeUninit<T> {
/// };
/// ```
#[stable(feature = "maybe_uninit_ref", since = "1.55.0")]
#[rustc_const_unstable(feature = "const_maybe_uninit_assume_init", issue = "none")]
#[rustc_const_unstable(feature = "const_maybe_uninit_assume_init_mut", issue = "none")]
Copy link
Member

Choose a reason for hiding this comment

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

The two changes in this file are the only ones where an unstable feature gate is being changed. Is there a reason this is the case, as opposed to changing the gate for the already-stabilized part of this feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

No reason. I didn't put a lot of thought into the required name changes but I would be happy to consider suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense to change the gate that's already stabilized, as that would avoid unnecessary breakage on Rust's end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in 9479358dda11c677064b89d676a8b67d0181be3b

@rust-log-analyzer

This comment has been minimized.

@dtolnay
Copy link
Member Author

dtolnay commented Mar 31, 2022

@lcnr
Copy link
Contributor

lcnr commented Apr 1, 2022

thanks @jhpratt for your review

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 1, 2022

📌 Commit 971ecff has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 1, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 1, 2022
Handle rustc_const_stable attribute in library feature collector

The library feature collector in [compiler/rustc_passes/src/lib_features.rs](https://github.com/rust-lang/rust/blob/551b4fa395fa588d91cbecfb0cdfe1baa02670cf/compiler/rustc_passes/src/lib_features.rs) has only been looking at `#[stable(…)]`, `#[unstable(…)]`, and `#[rustc_const_unstable(…)]` attributes, while ignoring `#[rustc_const_stable(…)]`. The consequences of this were:

- When any const feature got stabilized (changing one or more `rustc_const_unstable` to `rustc_const_stable`), users who had previously enabled that unstable feature using `#![feature(…)]` would get told "unknown feature", rather than rustc's nicer "the feature … has been stable since … and no longer requires an attribute to enable".

    This can be seen in the way that rust-lang#93957 (comment) failed after rebase:

    ```console
    error[E0635]: unknown feature `const_ptr_offset`
      --> $DIR/offset_from_ub.rs:1:35
       |
    LL | #![feature(const_ptr_offset_from, const_ptr_offset)]
       |                                   ^^^^^^^^^^^^^^^^
    ```

- We weren't enforcing that a particular feature is either stable everywhere or unstable everywhere, and that a feature that has been stabilized has the same stabilization version everywhere, both of which we enforce for the other stability attributes.

This PR updates the library feature collector to handle `rustc_const_stable`, and fixes places in the standard library and test suite where `rustc_const_stable` was being used in a way that does not meet the rules for a stability attribute.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 1, 2022
Handle rustc_const_stable attribute in library feature collector

The library feature collector in [compiler/rustc_passes/src/lib_features.rs](https://github.com/rust-lang/rust/blob/551b4fa395fa588d91cbecfb0cdfe1baa02670cf/compiler/rustc_passes/src/lib_features.rs) has only been looking at `#[stable(…)]`, `#[unstable(…)]`, and `#[rustc_const_unstable(…)]` attributes, while ignoring `#[rustc_const_stable(…)]`. The consequences of this were:

- When any const feature got stabilized (changing one or more `rustc_const_unstable` to `rustc_const_stable`), users who had previously enabled that unstable feature using `#![feature(…)]` would get told "unknown feature", rather than rustc's nicer "the feature … has been stable since … and no longer requires an attribute to enable".

    This can be seen in the way that rust-lang#93957 (comment) failed after rebase:

    ```console
    error[E0635]: unknown feature `const_ptr_offset`
      --> $DIR/offset_from_ub.rs:1:35
       |
    LL | #![feature(const_ptr_offset_from, const_ptr_offset)]
       |                                   ^^^^^^^^^^^^^^^^
    ```

- We weren't enforcing that a particular feature is either stable everywhere or unstable everywhere, and that a feature that has been stabilized has the same stabilization version everywhere, both of which we enforce for the other stability attributes.

This PR updates the library feature collector to handle `rustc_const_stable`, and fixes places in the standard library and test suite where `rustc_const_stable` was being used in a way that does not meet the rules for a stability attribute.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 2, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#95354 (Handle rustc_const_stable attribute in library feature collector)
 - rust-lang#95373 (invalid_value lint: detect invalid initialization of arrays)
 - rust-lang#95430 (Disable #[thread_local] support on i686-pc-windows-msvc)
 - rust-lang#95544 (Add error message suggestion for missing noreturn in naked function)
 - rust-lang#95556 (Implement provenance preserving methods on NonNull)
 - rust-lang#95557 (Fix `thread_local!` macro to be compatible with `no_implicit_prelude`)
 - rust-lang#95559 (small type system refactoring)
 - rust-lang#95560 (convert more `DefId`s to `LocalDefId`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d7a2400 into rust-lang:master Apr 2, 2022
@rustbot rustbot added this to the 1.61.0 milestone Apr 2, 2022
@dtolnay dtolnay deleted the rustc_const_stable branch April 3, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants