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

remove pointless allowed_through_unstable_modules on TryFromSliceError #135489

Merged
merged 2 commits into from
Jan 24, 2025

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 14, 2025

This got added in #132482 but the PR does not explain why. @lukas-code do you still remember? Also Cc @Noratrieb as reviewer of that PR.

If I understand the issue description correctly, all paths under which this type is exported are stable now: core::array::TryFromSliceError and std::array::TryFromSliceError. If that is the case, we shouldn't have the attribute; it's a terrible hack that should only be used when needed to maintain backward compatibility. Getting some historic information right is IMO not sufficient justification to risk accidentally exposing this type via more unstable paths today or in the future.

@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2025

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 14, 2025
@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2025

The rustc-dev-guide subtree was changed. If this PR only touches the dev guide consider submitting a PR directly to rust-lang/rustc-dev-guide otherwise thank you for updating the dev guide with your changes.

cc @BoxyUwU, @jieyouxu, @Kobzol

@lukas-code
Copy link
Member

The point of adding this atrribute was to make the "since" version on the rustdoc page show up correctly: https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html

std::array::TryFromSliceError is stably exposed by that path in 1.34.0, but the module std::array is only stably exposed in 1.35.0. (This is becaue we didn't the full path stability back then.)

Usually, we want to show the stability of the "most unstable" path component in rustdoc, this is important for example for core::error::Error, which itself has a #[stable(since = "1.0.0")] attribute, but shows as "stable since 1.81.0", because that's when core::error got stabilized: https://doc.rust-lang.org/nightly/core/error/trait.Error.html

But by that logic std::array::TryFromSliceError would also show as stable since 1.35.0.


Getting some historic information right is IMO not sufficient justification to risk accidentally exposing this type via more unstable paths today or in the future.

This seems plausible to me, and there are probably very few crates that still use MSRV 1.34.0 and name this type dicectly, so I'm fine with removing this again.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 14, 2025

The point of adding this atrribute was to make the "since" version on the rustdoc page show up correctly: https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html

It is news to me that rustdoc actually uses the attribute when computing the information shown there, but maybe it does? Cc @notriddle

I am not sure if there is some other way to get rustdoc to show the correct information here, but whatever we do should be rustdoc-only and not affect how rustc behaves on current stable releases. But it's probably not worth worrying about this tiny mistake only affecting a single version released five years ago...

Usually, we want to show the stability of the "most unstable" path component in rustdoc, this is important for example for core::error::Error, which itself has a #[stable(since = "1.0.0")] attribute, but shows as "stable since 1.81.0", because that's when core::error got stabilized: https://doc.rust-lang.org/nightly/core/error/trait.Error.html

TBH I find that page confusing since it is not at all clear that the "Since 1.81.0" refers to the path rather than the item.

@notriddle
Copy link
Contributor

notriddle commented Jan 14, 2025

It is news to me that rustdoc actually uses the attribute when computing the information shown there, but maybe it does?

I don't think the allowed_through_unstable_modules attribute is supposed to affect the version number shown there. That version number is calculated in this code:

https://github.com/rust-lang/rust/blob/master/src/librustdoc/passes/propagate_stability.rs

If the item was inlined, it'll pick the version number from the import, but allowed_through_unstable_modules off the original item. allowed_through_unstable_modules isn't specified on imports because it makes no sense there.

TBH I find that page confusing since it is not at all clear that the "Since 1.81.0" refers to the path rather than the item.

Assuming we want the number there to refer to the item, where should the stability of the path be specified? We don't want someone to copy a path into their code, with a "stable since 1.0" badge on it, only to actually try it in the 1.73 compiler and get rug-pulled when it turns out not to actually be stable. That's pretty bad for people's confidence in the docs.

The clearest, most explicit option of flat-out adding another attribute, like this?

Trait Error

Since 1.0.0 • In core since 1.81.0 • Source

It's unambiguous, but it's also not great for progressive disclosure, because that's a pretty complex concept that most people don't need to care about.

@RalfJung
Copy link
Member Author

RalfJung commented Jan 15, 2025

I don't think the allowed_through_unstable_modules attribute is supposed to affect the version number shown there. That version number is calculated in this code:

Odd. Then why does https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html show 1.34? It seems to ignore the stability of the path components... but clearly those are not ignored, as demonstrated by core::error::Error.

Assuming we want the number there to refer to the item, where should the stability of the path be specified?

Good question. Maybe it should say explicitly that the item is stable since X, but only available under this particular path since Y. I don't know how to best make that concise though. For the common case where both of these are the same version, we can keep the current display of course, so hopefully the "progressive disclosure" concern is remedied by this fact? The "path stability" flag could also have some hover / long-press text explaining what this is about.

Anyway that's a separate discussion from this PR.

@notriddle
Copy link
Contributor

Odd. Then why does https://doc.rust-lang.org/nightly/core/array/struct.TryFromSliceError.html show 1.34?

You're right. It affects the version number in this case.

if let Some(own_stab) = own_stability
&& let StabilityLevel::Stable { since: own_since, allowed_through_unstable_modules: false } =
own_stab.level
&& let Some(parent_stab) = parent_stability
&& (parent_stab.is_unstable()
|| parent_stab.stable_since().is_some_and(|parent_since| parent_since > own_since))
{
parent_stability

Items inherit stability from their parent module if they don't have this attribute and if their stability version is lower than the parent module's. That includes the version number. My mistake. I was too focused on inlining through use statements, and not thinking about parent modules.

@RalfJung
Copy link
Member Author

r? libs

@rustbot rustbot assigned tgross35 and unassigned joboet Jan 23, 2025
@tgross35
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 23, 2025

📌 Commit 6103896 has been approved by tgross35

It is now in the queue for this repository.

@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 Jan 23, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2025
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#133605 (Add extensive set of drop order tests)
 - rust-lang#135489 (remove pointless allowed_through_unstable_modules on TryFromSliceError)
 - rust-lang#135757 (Add NuttX support for AArch64 and ARMv7-A targets)
 - rust-lang#135799 (rustdoc-json: Rename `Path::name` to `path`, and give it the path again.)
 - rust-lang#135865 (For E0223, suggest associated functions that are similar to the path, even if the base type has multiple inherent impl blocks.)
 - rust-lang#135890 (Implement `VecDeque::pop_front_if` & `VecDeque::pop_back_if`)
 - rust-lang#135914 (Remove usages of `QueryNormalizer` in the compiler)
 - rust-lang#135936 (fix reify-intrinsic test)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a3fb2a0 into rust-lang:master Jan 24, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 24, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 24, 2025
Rollup merge of rust-lang#135489 - RalfJung:TryFromSliceError, r=tgross35

remove pointless allowed_through_unstable_modules on TryFromSliceError

This got added in rust-lang#132482 but the PR does not explain why. `@lukas-code` do you still remember? Also Cc `@Noratrieb` as reviewer of that PR.

If I understand the issue description correctly, all paths under which this type is exported are stable now: `core::array::TryFromSliceError` and `std::array::TryFromSliceError`. If that is the case, we shouldn't have the attribute; it's a terrible hack that should only be used when needed to maintain backward compatibility. Getting some historic information right is IMO *not* sufficient justification to risk accidentally exposing this type via more unstable paths today or in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustc-dev-guide Area: rustc-dev-guide S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants