Reduce the places where stable annotations are needed

[estebank]

Looking at the definition of std::option::Option, we have the following stable annotations:

#[derive(Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
#[stable(feature = "rust1", since = "1.0.0")]
pub enum Option<T> {
    /// No value
    #[stable(feature = "rust1", since = "1.0.0")]
    None,
    /// Some value `T`
    #[stable(feature = "rust1", since = "1.0.0")]
    Some(#[stable(feature = "rust1", since = "1.0.0")] T),
}

Is it strictly necessary for the stability checker to look at the fields of enum variants (or of structs, for that matter)?

It seems to me that MissingStabilityAnnotations could be modified to allow stability markings to flow down from at least a variant to its fields, as changing them is not a backwards compatible change and shouldn't ever happen. Even extending an existing enum would be backwards incompatible, so I would even flow from ADT attribute downwards. This would let the definition above to be:

#[derive(Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
#[stable(feature = "rust1", since = "1.0.0")]
pub enum Option<T> {
    /// No value
    None,
    /// Some value `T`
    Some(T),
}

This is normally not an issue, but with the new output in #65421 we will start showing this code to end users in errors, and it'd be nice if we could eliminate unnecessary boilerplate.

[pnkfelix]

Just as a reminder for anyone reading: private state (and state that falls under an unstable parent) does not need a stability attribute.

Here is a demo: play

But enums are implicitly public (and there's no way to opt out of that), so the proposal here does make some sense to me, at least for enums alone.

Also, if an enum were marked non-exhaustive (#44109), then of course it would be backwards compatible to add a new variant (which I what I interpreted "extending an existing enum" to mean, though perhaps that was meant to denote adding a new field to an existing variant???). So whatever we do here, it should account for that too.

[pnkfelix]

(also, we don't seem to have a tracking issue for staged_api, presumably because it is likely to remain forever an internal implementation detail? So I suspect we might consider removing T-lang from the discussion here...)

[pnkfelix]

triage: Leaving nominated for discussion. Not assigning a priority yet (I would have assigned P-medium since this is mostly internal technical debt, but the point @estebank makes about this soon become end-user visible is a little worrisome to me...)

[pnkfelix]

with the new output in #65421 we will start showing this code to end users in errors

wait, @estebank , are you sure we're going to point users to spans inside libcore and libstd? I would have assumed that such spans are not displayed in any diagnostics.

• Holy cow, we really do: https://github.com/rust-lang/rust/blob/96b83b9b7364ca3d5c52dddf8f24c9d94ec98511/src/test/ui/proc-macro/parent-source-spans.stderr#L136

[nikomatsakis]

I'd be in favor of lessening the annotation burden here, but I've not given a lot of thought, or looked for edge cases that might get overlooked.

[estebank]

@pnkfelix the PR that would make this a more pressing matter is being held up on this. I might merge the PR without pointing at std for now, but we do need to resolve this as we'll likely start pointing more and more into the stdlib, within reason.

[Centril]

But enums are implicitly public (and there's no way to opt out of that), so the proposal here does make some sense to me, at least for enums alone.

I've personally wanted to add privacy annotations to enum variants, so while the default would still be implicitly private, if we add such annotations, we'd need to remember to change the behavior here.

[Centril]

Worth noting that these stability annotations serve as documenting the history of additions. This seems relevant when considering non_exhaustive enums.

[estebank]

Currently the stable attribute is needed in 3 different levels (4 places in this case). Removing the need in at least one of them would be a win in my eyes.

[pnkfelix]

triage: leaving nominated and unprioritized.

My intention is for the team to come to a consensus as to whether we want to do this or not. I had hoped such discussion would occur during last week's T-compiler meeting, but I did not make my intent clear at that time.

Update: we subsequently discussed it at this week's meeting.

[nikomatsakis]

My opinion:

We should probably remove stability annotations from (public) enum variants.

However, I would love it if somebody from @rust-lang/libs would weigh in just to give a "seems good" sort of comment -- I'm wondering if there are scenarios they can think of where they wanted to have an enum that was stable but not its variants, and whether they feel like they'd be surprised by the change here.

[alexcrichton]

I would agree with @nikomatsakis's conclusion, I think we've never had a case for public enums and private variants (other than maybe __Nonexhaustive sometimes which is fixed with #[non_exhaustive]), so removing all the internal attributes should be fine.

[estebank]

At the very least I believe that the stability attribute in the variant fields is redundant as I cannot imagine a situation where a variant would be stable but its fields wouldn't, whereas I could imagine a contrived case where an enum stable but its variants aren't.

[dtolnay]

Removing #[stable] from public enum variants and their fields both seem good to me.

This isn't something we would need right away but what would make sense to me is if public elements of an item (its variants and fields) inherit #[stable] from the item, with explicit #[unstable] to negate that behavior.

#[stable(feature = "demo", since = "0.0.0")]
pub enum E {
    // stable because the item is stable
    A,
    #[unstable(feature = "demo_b", issue = "0")]
    B,
}

[SimonSapin]

It seems to me that MissingStabilityAnnotations could be modified to allow stability markings to flow down from at least a variant to its fields, as changing them is not a backwards compatible change and shouldn't ever happen.

Isn’t this already the case? We definitely already have cases when stability is "inherited" from the parent node / context. For example the std::raw::TraitObject struct is unstable because the std::raw module is.

rust/src/libcore/raw.rs

Line 2 in 253fc0e

#![unstable(feature = "raw", issue = "27751")]

rust/src/libcore/raw.rs

Lines 79 to 82 in 253fc0e

	#[repr(C)]
	#[derive(Copy, Clone)]
	#[allow(missing_debug_implementations)]
	pub struct TraitObject {

I would be in favor of extending this kind of inheritance to other cases, and removing from library source code attributes that are or become redundant.

However I don’t see the point of removing from the language support for having stability attributes on enum fields, enum variants, or any kind of AST node.

Currently the stable attribute is needed in 3 different levels (4 places in this case). Removing the need in at least one of them would be a win in my eyes.

Removing the need: yes. Removing the possibility: why?

At the very least I believe that the stability attribute in the variant fields is redundant as I cannot imagine a situation where a variant would be stable but its fields wouldn't,

It didn’t work out, but we had a plan to do exactly this in TryReserveError: #61780, #48043 (comment).

[pnkfelix]

triage: P-medium. Removing nomination and assigning to @estebank to move forward with this.

[estebank]

Isn’t this already the case? We definitely already have cases when stability is "inherited" from the parent node / context.

We only inherit for #[unstable(..)], not for #[stable(..)].

I would be in favor of extending this kind of inheritance to other cases, and removing from library source code attributes that are or become redundant.

However I don’t see the point of removing from the language support for having stability attributes on enum fields, enum variants, or any kind of AST node.

Agree. It seems like everybody in this thread is in agreement of the behavior we want.

I'm making a small change in behavior as outlined by @dtolnay and @SimonSapin above.

Landing the changes in std will have to wait until next beta promotion.

[estebank]

The pull request is ready for review. On the separate (blocked) PR you can see the impact this would have on the stdlib once this lands.