Error on private items with stability attributes #83397
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
camelid
added
A-stability
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
camelid
force-pushed
the
stability-without-publicity
branch
from
ce0f22f to
a59f8da
Compare
This comment has been minimized.
This comment has been minimized.
camelid
added
the
S-waiting-on-author
jyn514
added
the
T-libs
|
r? @m-ou-se since I think she'll be able to answer "should we do this at all" better |
|
@camelid Ping from triage, what's the current status of this? Seems CI is still red. |
Yeah, I'm not really sure why the code isn't working, and I haven't gotten around to asking yet. |
I'm very much in favour of lints like this for the attributes we use in I haven't looked in detail through the errors CI is giving, but I wonder how many of these are actual errors with these attributes in Assigning it back for compiler review: |
jyn514
reviewed
Apr 12, 2021
|
|
||
| fn check_private_stability(&self, hir_id: HirId, span: Span) { | ||
| let stab = self.tcx.stability().local_stability(hir_id); | ||
| let is_error = stab.is_some() && !self.access_levels.is_reachable(hir_id); |
There was a problem hiding this comment.
Note this is currently buggy: #64762
jyn514
removed
the
S-waiting-on-review
|
@camelid Ping from triage, CI still red, and there's merge conflicts. Would you mind addressing the comments above? Thanks! If you need help from team members, maybe Zulip is a more effective approach to talk with them. Thanks! |
crlf0710
added
S-waiting-on-author
|
Thanks for the ping! I'm currently catching up on my pull requests, so I'll try to get to this one soon :) |
camelid
force-pushed
the
stability-without-publicity
branch
from
a59f8da to
bd4823e
Compare
|
Starting with a rebase... |
This comment has been minimized.
This comment has been minimized.
|
@camelid Ping from triage, CI is still red here. |
|
@camelid Ping again from triage, CI is still failing |
|
I will try to get to this soon. |
It doesn't make sense for a private item to have a stability attribute, and it is likely a mistake. Report an error when we see a case of this.
camelid
force-pushed
the
stability-without-publicity
branch
from
ba48420 to
dad5673
Compare
|
Rebased. |
This comment has been minimized.
This comment has been minimized.
camelid
force-pushed
the
stability-without-publicity
branch
from
dad5673 to
b318344
Compare
|
Hmm, still getting thousands of stability errors when compiling |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
|
Ok, it seems like the issue is that definitions are inheriting stability from their parent. E.g., in this code: #![stable(feature = "rust1", since = "1.0.0")]
#[stable(feature = "rust1", since = "1.0.0")]
pub struct MyStruct {
my_field: u32,
}
Removing both the crate-level stability attribute and the struct-level one causes the field error to disappear (although then it rightly errors because the crate and struct are public but don't have stability attributes). It seems like this might be a pre-existing bug in how stabilities are computed. |
|
Ok, I fixed part of it: it's down from 3608 to 3300 errors 😅 I think some of the bugs might have been introduced in #71481. |
|
It seems that some of the errors were due to bugs in the stability-checking code, but many of them (probably the vast majority) were because some items were private but should still inherit stability from their parent. I'm pretty sure that the stability of a private item is irrelevant because it can't be accessed outside the crate, but I'd rather not mess with the existing stability code too much since it's a bit hard to understand what's going on. So, I think a better approach, at least for now, is to just look at the item's actual stability attributes (not what it's inherited from its parent). This approach seems promising: I'm down to 53 errors in |
|
A lot of the errors seem to be correct! There are a few false positives; so far, I've seen a couple cases where |
Hmm, I wonder if a better approach would be to not inherit stability for private items... |
Actually those weren't false positives: the |
|
Unfortunately, now that I've removed the |
|
@camelid can you post one of the errors? |
|
Sure, here are some samples: Sample 1 (errors from the start of the output)Sample 2 (errors from the end of the output)And here's a diff showing what stability attributes I deleted: Diffdiff --git a/library/core/src/future/future.rs b/library/core/src/future/future.rs
index 15952c6806f..bffc5c430f7 100644
--- a/library/core/src/future/future.rs
+++ b/library/core/src/future/future.rs
@@ -1,5 +1,3 @@
-#![stable(feature = "futures_api", since = "1.36.0")]
-
use crate::marker::Unpin;
use crate::ops;
use crate::pin::Pin;
diff --git a/library/core/src/hash/sip.rs b/library/core/src/hash/sip.rs
index 6178b0af137..ac7be9b390a 100644
--- a/library/core/src/hash/sip.rs
+++ b/library/core/src/hash/sip.rs
@@ -27,11 +27,6 @@ pub struct SipHasher13 {
/// An implementation of SipHash 2-4.
///
/// See: <https://131002.net/siphash/>
-#[unstable(feature = "hashmap_internals", issue = "none")]
-#[rustc_deprecated(
- since = "1.13.0",
- reason = "use `std::collections::hash_map::DefaultHasher` instead"
-)]
#[derive(Debug, Clone, Default)]
struct SipHasher24 {
hasher: Hasher<Sip24Rounds>,
diff --git a/library/core/src/iter/adapters/mod.rs b/library/core/src/iter/adapters/mod.rs
index a3fbf4d9c38..8f315472085 100644
--- a/library/core/src/iter/adapters/mod.rs
+++ b/library/core/src/iter/adapters/mod.rs
@@ -196,7 +196,6 @@ fn ok<B, T>(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result<B, !> {
}
}
-#[unstable(issue = "none", feature = "inplace_iteration")]
unsafe impl<S: Iterator, I, E> SourceIter for ResultShunt<'_, I, E>
where
I: SourceIter<Source = S>,
@@ -213,7 +212,6 @@ unsafe fn as_inner(&mut self) -> &mut S {
// SAFETY: ResultShunt::next calls I::find, which has to advance `iter` in order to
// return `Some(_)`. Since `iter` has type `I: InPlaceIterable` it's guaranteed that
// at least one item will be moved out from the underlying source.
-#[unstable(issue = "none", feature = "inplace_iteration")]
unsafe impl<I, T, E> InPlaceIterable for ResultShunt<'_, I, E> where
I: Iterator<Item = Result<T, E>> + InPlaceIterable
{
diff --git a/library/core/src/lib.rs b/library/core/src/lib.rs
index 540cdf124ee..f283f285e1f 100644
--- a/library/core/src/lib.rs
+++ b/library/core/src/lib.rs
@@ -313,7 +313,6 @@ pub mod assert_matches {
// FIXME: This annotation should be moved into rust-lang/stdarch after clashing_extern_declarations is
// merged. It currently cannot because bootstrap fails as the lint hasn't been defined yet.
#[allow(clashing_extern_declarations)]
-#[unstable(feature = "stdsimd", issue = "48556")]
mod core_arch;
#[doc = include_str!("../../stdarch/crates/core_arch/src/core_arch_docs.md")]
diff --git a/library/core/src/ops/mod.rs b/library/core/src/ops/mod.rs
index 85e04740d96..f9fe121bbc5 100644
--- a/library/core/src/ops/mod.rs
+++ b/library/core/src/ops/mod.rs
@@ -187,7 +187,6 @@
#[unstable(feature = "try_trait_v2", issue = "84277")]
pub use self::try_trait::Try;
-#[unstable(feature = "try_trait_transition", reason = "for bootstrap", issue = "none")]
pub(crate) use self::try_trait::Try as TryV2;
#[unstable(feature = "generator_trait", issue = "43122")]
diff --git a/library/core/src/ptr/metadata.rs b/library/core/src/ptr/metadata.rs
index 287ae69acd1..bb7652a3526 100644
--- a/library/core/src/ptr/metadata.rs
+++ b/library/core/src/ptr/metadata.rs
@@ -1,5 +1,3 @@
-#![unstable(feature = "ptr_metadata", issue = "81513")]
-
use crate::fmt;
use crate::hash::{Hash, Hasher};
diff --git a/library/core/src/slice/iter.rs b/library/core/src/slice/iter.rs
index 5cbc6343e3a..5dfa1dc3441 100644
--- a/library/core/src/slice/iter.rs
+++ b/library/core/src/slice/iter.rs
@@ -880,7 +880,6 @@ fn next_back(&mut self) -> Option<&'a [T]> {
}
}
-#[stable(feature = "slice_rsplit", since = "1.27.0")]
impl<'a, T, P> SplitIter for RSplit<'a, T, P>
where
P: FnMut(&T) -> bool,
@@ -936,7 +935,6 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
}
}
-#[stable(feature = "slice_rsplit", since = "1.27.0")]
impl<'a, T, P> SplitIter for RSplitMut<'a, T, P>
where
P: FnMut(&T) -> bool,
diff --git a/library/core/src/slice/mod.rs b/library/core/src/slice/mod.rs
index de25c984abf..5087bf8ae89 100644
--- a/library/core/src/slice/mod.rs
+++ b/library/core/src/slice/mod.rs
@@ -1582,7 +1582,6 @@ pub fn split_at_mut(&mut self, mid: usize) -> (&mut [T], &mut [T]) {
/// assert_eq!(right, []);
/// }
/// ```
- #[unstable(feature = "slice_split_at_unchecked", reason = "new API", issue = "76014")]
#[inline]
unsafe fn split_at_unchecked(&self, mid: usize) -> (&[T], &[T]) {
// SAFETY: Caller has to check that `0 <= mid <= self.len()`
@@ -1622,7 +1621,6 @@ unsafe fn split_at_unchecked(&self, mid: usize) -> (&[T], &[T]) {
/// }
/// assert_eq!(v, [1, 2, 3, 4, 5, 6]);
/// ```
- #[unstable(feature = "slice_split_at_unchecked", reason = "new API", issue = "76014")]
#[inline]
unsafe fn split_at_mut_unchecked(&mut self, mid: usize) -> (&mut [T], &mut [T]) {
let len = self.len();
diff --git a/library/core/src/str/validations.rs b/library/core/src/str/validations.rs
index 373a8212425..56d8506d9a4 100644
--- a/library/core/src/str/validations.rs
+++ b/library/core/src/str/validations.rs
@@ -250,9 +250,8 @@ pub(super) fn run_utf8_validation(v: &[u8]) -> Result<(), Utf8Error> {
];
/// Given a first byte, determines how many bytes are in this UTF-8 character.
-#[unstable(feature = "str_internals", issue = "none")]
#[inline]
-pub fn utf8_char_width(b: u8) -> usize {
+pub(crate) fn utf8_char_width(b: u8) -> usize {
UTF8_CHAR_WIDTH[b as usize] as usize
}
diff --git a/library/core/src/task/poll.rs b/library/core/src/task/poll.rs
index fc0a4e74797..5cc050e97be 100644
--- a/library/core/src/task/poll.rs
+++ b/library/core/src/task/poll.rs
@@ -1,5 +1,3 @@
-#![stable(feature = "futures_api", since = "1.36.0")]
-
use crate::convert;
use crate::ops::{self, ControlFlow};
use crate::result::Result;
diff --git a/library/core/src/task/wake.rs b/library/core/src/task/wake.rs
index b775e022a54..f6740ea928c 100644
--- a/library/core/src/task/wake.rs
+++ b/library/core/src/task/wake.rs
@@ -1,5 +1,3 @@
-#![stable(feature = "futures_api", since = "1.36.0")]
-
use crate::fmt;
use crate::marker::{PhantomData, Unpin}; |
|
☔ The latest upstream changes (presumably #83723) made this pull request unmergeable. Please resolve the merge conflicts. |
|
You said:
But all the errors are on public items. Maybe the problem is that some of the items are not actually private, because they're re-exported somewhere? If they're not private, then it seems worth making a cleanup PR to remove |
|
I'm not sure why this change doesn't work correctly, and I probably won't be able to look into it soon, so closing for now. I may try again in the future if someone else doesn't do it by then. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #83380.
It doesn't make sense for a private item to have a stability attribute,
and it is likely a mistake. Report an error when we see a case of this.
Inspired by #83372 (comment).
Also tweaked wording of "missing stability attribute" error.