Support multiple unstable attributes on items #94988

compiler-errors · 2022-03-16T03:55:13Z

This PR started out pretty simple, but ended up being quite a few changes in a lot of different places to support multiple unstable attributes on items.

Please let me know if I should split this up into separate commits. I feel somewhat bad having one mega-commit, but I wrote this kinda all over the place, so I didn't naturally split it up when I was making these changes.

Fixes #94770

cc: @m-ou-se

rust-highfive · 2022-03-16T03:55:16Z

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occurred in clean/types.rs.

cc @camelid

rust-highfive · 2022-03-16T03:55:17Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

compiler-errors · 2022-03-16T04:58:58Z

Hmmmmm. Seems to be an issue with the way that Encodable interacts with the custom ProcMacroData encoding. It seems we don't store crate metadata for proc macros, so the span stored in the macro's StabilityLevel fails to be decoded.

Not exactly sure how to fix this. Any thoughts @Aaron1011 (I think you're familiar with this code, cc #76897)?

petrochenkov · 2022-03-16T07:02:31Z

@cjgillot do you want to review this (given that you are working on #93017)?
If not, then feel free to reassign back.
r? @cjgillot

bors · 2022-03-18T03:41:52Z

☔ The latest upstream changes (presumably #95056) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot

Thanks for the PR @compiler-errors.
I took a brief look, everything looks good.

What is the motivation for this PR?
What does it mean for an item to be multiply unstable?
How should it be handled/diagnosed?
I saw you have a specific handling for const-unstable attributes, could you elaborate on that?

compiler/rustc_const_eval/src/transform/check_consts/check.rs

cjgillot · 2022-03-20T10:34:02Z

compiler/rustc_const_eval/src/transform/check_consts/check.rs

+                } else {
+                    // FIXME(ecstaticmorse); For compatibility, we consider `unstable` callees that
+                    // have no `rustc_const_stable` attributes to be const-unstable as well. This
+                    // should be fixed later.


Is this compatibility layer still used?

I have no idea. @ecstatic-morse do you know if this FIXME is still valid?

IIRC, these rules proved somewhat contentious, although I don't know if this particular idea was the problematic one. You can take out the FIXME and the "this should be fixed", but please leave the description if it's still correct.

compiler-errors · 2022-03-21T20:42:19Z

@cjgillot:

What is the motivation for this PR?

See #94770 for the ask from @m-ou-se who wanted multiple unstable attributes on a single item, I think to avoid accidentally stabilizing it when removing the one unstable flag.

What does it mean for an item to be multiply unstable? How should it be handled/diagnosed?

I believe it must be that all unstable features must be enabled or the usage is considered invalid and will be an error. Basically each unstable here is considered independently.

I saw you have a specific handling for const-unstable attributes, could you elaborate on that?

I believe you're referencing the const-call code, which I had to generalize to several unstable attributes. I don't believe this changes behavior for a single const_unstable attribute, but I needed to refactor the code to only pass if all const_unstable attributes were allowed.

m-ou-se · 2022-03-21T21:49:22Z

What is the motivation for this PR?

Imagine we have an unstable type called NonTwoU32 which can never be 2, gated by non_two_ints gate, and an unstable method on all the integer types called is_power_of_three() (including u32 and NonZeroU32 and NonTwoU32 etc.), gated by the is_power_of_three feature gate. Then NonTwoU32::is_power_of_three() should be gated by both these feature gates. If either of the features is stabilized, its #[unstable] tag can be removed from that specific method, while still keeping the other one.

In the past we've had situations where we accidentally stabilized a method like NonTwoU32::is_power_of_three() while stabilizing NonTwoU32, while we hadn't yet decided if it was a good idea to have a is_power_of_three() method on any type yet.

compiler-errors · 2022-03-23T01:24:11Z

@rustbot ready

I believe this is ready for a more thorough review

src/librustdoc/clean/inline.rs

camelid · 2022-03-23T21:28:48Z

src/librustdoc/html/render/mod.rs

-        Some(ConstStability { level: StabilityLevel::Unstable { issue, .. }, feature, .. }) => {
-            let unstable = if let Some(n) = issue {
+        Some(ConstStability { level: StabilityLevel::Unstable { unstables, .. }, .. }) => {
+            // FIXME(compiler-errors): Should we link to several issues here? How?


cc @jsha who's looked into redesigning rustdoc's version display

The current style is like:

const: unstable · source

I don't think there's room in there for multiple unstable links. Instead I'd recommend picking the first one to link to, and solving this in the issue descriptions by cross-linking to issues that apply to intersection sets of items.

camelid · 2022-03-23T21:29:12Z

src/librustdoc/lib.rs

@@ -40,7 +41,6 @@ extern crate rustc_ast;
 extern crate rustc_ast_lowering;
 extern crate rustc_ast_pretty;
 extern crate rustc_attr;
-extern crate rustc_const_eval;


Why this change?

I didn't do this intentionally. no idea why that happened, can add it back.

Yeah, I think it should be re-added.

ahh, so I moved is_unstable_const_fn (which was exported from rustc_const_eval as a function) to rustc_middle as a method on TyCtxt, so there is no dependency on rustc_const_eval anymore.

In fact, x check rustdoc fails if I add this crate back:

error: unused extern crate --> src/librustdoc/lib.rs:44:1 | 44 | extern crate rustc_const_eval; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove it

guess i did do it intentionally, but just forgot about it

bors · 2022-03-26T03:26:25Z

☔ The latest upstream changes (presumably #95149) made this pull request unmergeable. Please resolve the merge conflicts.

cjgillot

The implementation looks good overall, left a few nits.

Second question about semantics:
Unstability attributes can be inherited from an enclosing block (examples below). In this implementation, the innermost attribute overrides attributes from outer scopes. I wonder if they should not be made cumulative:

#[rustc_unstable(feature="foo")]
struct Foo {
  #[rustc_unstable(feature="bar")]
  Bar()
  // Is this unstable by ["foo", "bar"] or only ["bar"]?
}

#[rustc_const_unstable(feature="foo")]
impl Foo for () {
  #[rustc_const_unstable(feature="bar")]
  fn bar() {}
  // Is this unstable by ["foo", "bar"] or only ["bar"]?
}

cjgillot · 2022-03-26T11:29:56Z

src/librustdoc/html/render/mod.rs

-        .map(|stab| (stab.level, stab.feature))
+    if let Some(rustc_attr::Stability {
+        level: StabilityLevel::Unstable { unstables, .. }, ..
+    }) = item.stability(cx.tcx())
    {


This block will add the "nightly-only experimental API" message even if there is only the rustc_private feature. Why this change in behaviour?

cjgillot · 2022-03-26T11:30:41Z

src/librustdoc/html/render/mod.rs

-        .map(|stab| (stab.level, stab.feature))
+    if let Some(rustc_attr::Stability {
+        level: StabilityLevel::Unstable { unstables, .. }, ..
+    }) = item.stability(cx.tcx())
    {
        let mut message =
            "<span class=\"emoji\">🔬</span> This is a nightly-only experimental API.".to_owned();


This string and the parentheses could be incorporated to the extra_info.push below.

compiler-errors · 2022-03-27T04:13:35Z

Second question about semantics

@m-ou-se, do you know if we should change the behavior so that the unstable attributes inherited from parent modules are additive?

m-ou-se · 2022-03-28T18:52:41Z

Second question about semantics

@m-ou-se, do you know if we should change the behavior so that the unstable attributes inherited from parent modules are additive?

Oh, that's a good question. I'd expect them to be additive, but we should take a look at the current #[unstable] attributes in core/alloc/std, and see what makes sense there. Having an unstable module with stable items (or differently-unstable items) inside it sounds wrong, but I suspect we actually use that in some places in combination with a re-export from a stable module.

compiler-errors · 2022-05-17T17:25:30Z

whoops, I definitely let this PR sit for too long. Let me put it back onto my to-do list.

I'd expect them to be additive, but we should take a look at the current #[unstable] attributes in core/alloc/std, and see what makes sense there.

@m-ou-se, who would be the right person to check this? I can definitely give it a first look-over, but might need some help from T-libs to know what is okay and what might break due to this PR.

m-ou-se · 2022-05-17T17:37:06Z

@m-ou-se, who would be the right person to check this? I can definitely give it a first look-over, but might need some help from T-libs to know what is okay and what might break due to this PR.

I think @yaahc recently looked into this, but I might be wrong.

yaahc · 2022-05-17T18:34:43Z

I think @yaahc recently looked into this, but I might be wrong.

You are not wrong! I'm working on something very closely related in #95956 where I'm adding support for checking the stability of path segments when naming items. This in effect makes them additive and would end up duplicating the functionality in this PR if you added parent module additive inheritance.

Having an unstable module with stable items (or differently-unstable items) inside it sounds wrong, but I suspect we actually use that in some places in combination with a re-export from a stable module.

Like you said, this seems wrong but we already do this in multiple places, between core::unicode::UNICODE_VERSION and many of the intrinsics like core::intrinsics::transmute. Not to mention that I'm looking to depend upon such functionality to move Error into core unstably, by having an unstable core::error module with a stable Error trait in it that is then re-exported from the stable std::error module. I think it's important that we preserve that functionality.

Based on that and how hard I anticipate auditing the inherited stability attributes to be, I think the right call is to not make them inherit additively in this PR, and instead rely on the stable-in-unstable PR's path segment stability checks to get the additive functionality between parent module stability and item stability.

compiler-errors · 2022-11-05T18:40:00Z

This has PR has rotted so much that it probably needs to be re-done from scratch. I'll close this and unclaim the issue.

rust-highfive assigned petrochenkov Mar 16, 2022

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 16, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2022

This comment has been minimized.

Sign in to view

rust-highfive assigned cjgillot and unassigned petrochenkov Mar 16, 2022

compiler-errors force-pushed the multiple-unstables branch from ff62114 to c44e420 Compare March 17, 2022 03:18

compiler-errors force-pushed the multiple-unstables branch from c44e420 to 16ee185 Compare March 19, 2022 04:54

cjgillot reviewed Mar 20, 2022

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2022

compiler-errors force-pushed the multiple-unstables branch from 16ee185 to 2db02ac Compare March 23, 2022 01:23

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 23, 2022

This comment has been minimized.

Sign in to view

compiler-errors force-pushed the multiple-unstables branch from 2db02ac to f732d45 Compare March 23, 2022 04:06

camelid reviewed Mar 23, 2022

View reviewed changes

compiler-errors added 2 commits March 23, 2022 16:35

Support multiple unstable attributes

90a545b

Remove spans from StabilityLevel, so it can be decoded in ProcMacroData

013ac1a

compiler-errors force-pushed the multiple-unstables branch from f732d45 to 013ac1a Compare March 23, 2022 23:35

cjgillot reviewed Mar 26, 2022

View reviewed changes

cjgillot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 2, 2022

compiler-errors closed this Nov 5, 2022

compiler-errors deleted the multiple-unstables branch August 11, 2023 20:07

dianne mentioned this pull request Oct 17, 2024

Support multiple stability attributes on items #131824

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple unstable attributes on items #94988

Support multiple unstable attributes on items #94988

compiler-errors commented Mar 16, 2022

rust-highfive commented Mar 16, 2022

rust-highfive commented Mar 16, 2022

This comment has been minimized.

compiler-errors commented Mar 16, 2022

petrochenkov commented Mar 16, 2022

bors commented Mar 18, 2022

cjgillot left a comment

cjgillot Mar 20, 2022

compiler-errors Mar 23, 2022

ecstatic-morse Mar 28, 2022

compiler-errors commented Mar 21, 2022

m-ou-se commented Mar 21, 2022

compiler-errors commented Mar 23, 2022

This comment has been minimized.

camelid Mar 23, 2022

jsha Jun 12, 2022

camelid Mar 23, 2022

compiler-errors Mar 23, 2022

camelid Mar 23, 2022

compiler-errors Mar 23, 2022 •

edited

Loading

bors commented Mar 26, 2022

cjgillot left a comment

cjgillot Mar 26, 2022

cjgillot Mar 26, 2022

compiler-errors commented Mar 27, 2022

m-ou-se commented Mar 28, 2022

compiler-errors commented May 17, 2022

m-ou-se commented May 17, 2022

yaahc commented May 17, 2022

compiler-errors commented Nov 5, 2022

Support multiple unstable attributes on items #94988

Support multiple unstable attributes on items #94988

Conversation

compiler-errors commented Mar 16, 2022

rust-highfive commented Mar 16, 2022

rust-highfive commented Mar 16, 2022

This comment has been minimized.

compiler-errors commented Mar 16, 2022

petrochenkov commented Mar 16, 2022

bors commented Mar 18, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Mar 21, 2022

m-ou-se commented Mar 21, 2022

compiler-errors commented Mar 23, 2022

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

bors commented Mar 26, 2022

cjgillot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors commented Mar 27, 2022

m-ou-se commented Mar 28, 2022

compiler-errors commented May 17, 2022

m-ou-se commented May 17, 2022

yaahc commented May 17, 2022

compiler-errors commented Nov 5, 2022

compiler-errors Mar 23, 2022 •

edited

Loading