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

don't use force-unstable-if-unmarked with x test in standard library doctests #115186

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Aug 24, 2023

Using force-unstable-if-unmarked on library/ makes bootstrap ignoring doctests errors, potentially leading to the merging of PRs that do not compile successfully.

For testing this change, apply the following patch(or remove one of the error_in_core in library/core/src/error.rs):

diff --git a/library/core/src/error.rs b/library/core/src/error.rs
index 1170221c10c..b0ab2b04032 100644
--- a/library/core/src/error.rs
+++ b/library/core/src/error.rs
@@ -130,7 +130,6 @@ fn cause(&self) -> Option<&dyn Error> {
     ///
     /// ```rust
     /// #![feature(error_generic_member_access)]
-    /// #![feature(error_in_core)]
     /// use core::fmt;
     /// use core::error::{request_ref, Request};
     ///
@@ -361,7 +360,6 @@ pub fn sources(&self) -> Source<'_> {
 ///
 /// ```rust
 /// # #![feature(error_generic_member_access)]
-/// # #![feature(error_in_core)]
 /// use std::error::Error;
 /// use core::error::request_value;
 ///
@@ -385,7 +383,6 @@ pub fn request_value<'a, T>(err: &'a (impl Error + ?Sized)) -> Option<T>
 ///
 /// ```rust
 /// # #![feature(error_generic_member_access)]
-/// # #![feature(error_in_core)]
 /// use core::error::Error;
 /// use core::error::request_ref;
 ///
@@ -457,7 +454,6 @@ fn request_by_type_tag<'a, I>(err: &'a (impl Error + ?Sized)) -> Option<I::Reifi
 ///
 /// ```
 /// #![feature(error_generic_member_access)]
-/// #![feature(error_in_core)]
 /// use core::fmt;
 /// use core::error::Request;
 /// use core::error::request_ref;
@@ -528,7 +524,6 @@ fn new<'b>(erased: &'b mut (dyn Erased<'a> + 'a)) -> &'b mut Request<'a> {
     ///
     /// ```rust
     /// #![feature(error_generic_member_access)]
-    /// #![feature(error_in_core)]
     ///
     /// use core::error::Request;
     ///
@@ -563,7 +558,6 @@ pub fn provide_value<T>(&mut self, value: T) -> &mut Self
     ///
     /// ```rust
     /// #![feature(error_generic_member_access)]
-    /// #![feature(error_in_core)]
     ///
     /// use core::error::Request;
     ///
@@ -599,7 +593,6 @@ pub fn provide_value_with<T>(&mut self, fulfil: impl FnOnce() -> T) -> &mut Self
     ///
     /// ```rust
     /// #![feature(error_generic_member_access)]
-    /// #![feature(error_in_core)]
     ///
     /// use core::error::Request;
     ///
@@ -632,7 +625,6 @@ pub fn provide_ref<T: ?Sized + 'static>(&mut self, value: &'a T) -> &mut Self {
     ///
     /// ```rust
     /// #![feature(error_generic_member_access)]
-    /// #![feature(error_in_core)]
     ///
     /// use core::error::Request;
     ///
@@ -698,7 +690,6 @@ fn provide_with<I>(&mut self, fulfil: impl FnOnce() -> I::Reified) -> &mut Self
     /// it.
     ///
     /// ```rust
-    /// #![feature(error_generic_member_access)]
     /// #![feature(error_in_core)]
     ///
     /// use core::error::Request;
@@ -787,7 +778,6 @@ pub fn would_be_satisfied_by_value_of<T>(&self) -> bool
     ///
     /// ```rust
     /// #![feature(error_generic_member_access)]
-    /// #![feature(error_in_core)]
     ///
     /// use core::error::Request;
     /// use core::error::request_ref;

then run x test library/core --doc

Related #114838

@rustbot
Copy link
Collaborator

rustbot commented Aug 24, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 24, 2023
@onur-ozkan
Copy link
Member Author

onur-ozkan commented Aug 24, 2023

I don't think the block I removed makes any sense.. But still, I am not certain about regressions with this change.

@bors rollup=never

@rust-log-analyzer

This comment has been minimized.

@onur-ozkan
Copy link
Member Author

Okay I see how it makes sense...

@@ -1722,10 +1722,6 @@ impl<'a> Builder<'a> {
cargo.env("UPDATE_EXPECT", "1");
}

if !mode.is_tool() {
cargo.env("RUSTC_FORCE_UNSTABLE", "1");
}
Copy link
Member

Choose a reason for hiding this comment

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

It certainly makes no sense to remove this without also removing the code that receives this env var

if env::var_os("RUSTC_FORCE_UNSTABLE").is_some() {
cmd.arg("-Z").arg("force-unstable-if-unmarked");
}

if env::var_os("RUSTC_FORCE_UNSTABLE").is_some() {
cmd.arg("-Z").arg("force-unstable-if-unmarked");
}

But this is old code, and I think it is very important. For instance it ensures that if people load the libc crate from the sysroot, they get stability errors, even though libc does not contain any unstable markers.

It's generally a bad idea to remove code if you cannot explain why it was there. :)

Copy link
Member

Choose a reason for hiding this comment

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

However, if removing this env var helps, that should hopefully get us one step closer to solving the puzzle. Just, the env var has to stay.

Copy link
Member Author

@onur-ozkan onur-ozkan Aug 24, 2023

Choose a reason for hiding this comment

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

Yeah, removing it indeed was a bad idea. Will fix the problem by updating the condition instead of removing it.

It's generally a bad idea to remove code if you cannot explain why it was there. :)

Very true, at the first look it didn't make any sense. But there is the CI report so.. 😄

@onur-ozkan onur-ozkan force-pushed the do-not-force-unstable-if-unmarked branch from 774cac3 to aa255bd Compare August 24, 2023 20:34
@rust-log-analyzer

This comment has been minimized.

@onur-ozkan onur-ozkan added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 24, 2023
@onur-ozkan onur-ozkan marked this pull request as draft August 24, 2023 20:43
@onur-ozkan onur-ozkan removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 24, 2023
@onur-ozkan
Copy link
Member Author

Just put the PR mode in draft. Tomorrow I will consider the best way to handle this.

…ary doctests.

Using `force-unstable-if-unmarked makes` bootstrap process ignoring errors, potentially leading to the merging of PRs
that do not compile successfully.

Signed-off-by: ozkanonur <[email protected]>
@onur-ozkan onur-ozkan force-pushed the do-not-force-unstable-if-unmarked branch from aa255bd to 6288739 Compare August 25, 2023 05:00
@onur-ozkan
Copy link
Member Author

@rustbot ready

@rustbot rustbot removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 25, 2023
@onur-ozkan onur-ozkan marked this pull request as ready for review August 25, 2023 05:30
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 25, 2023
@onur-ozkan onur-ozkan changed the title do not force unstable if unmarked on bootstrap don't use force-unstable-if-unmarked with x test in standard library doctests Aug 25, 2023
@RalfJung
Copy link
Member

I'm not sure this is the right appraoch. Shouldn't rustc be fixed to still complain about the missing feature gate despite -Zforce-unstable-if-unmarked?

@onur-ozkan
Copy link
Member Author

onur-ozkan commented Aug 25, 2023

I'm not sure this is the right appraoch. Shouldn't rustc be fixed to still complain about the missing feature gate despite -Zforce-unstable-if-unmarked?

Regarding to your comment here #114838 (comment), no it should not.

If you meaning fixing this from the compiler side, I can add a FIXME note to the change I did. This PR at least ensures not merging the broken changes until the rustc fix

@onur-ozkan
Copy link
Member Author

If you meaning fixing this from the compiler side, I can add a FIXME note to the change I did. This PR at least ensures not merging the broken changes until the rustc fix

I guess this will make things more complicated. I will keep this PR draft and let the compiler team fix this problem with the right approach

@onur-ozkan onur-ozkan marked this pull request as draft August 25, 2023 06:28
@onur-ozkan onur-ozkan added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 25, 2023
@onur-ozkan onur-ozkan closed this Sep 12, 2023
@onur-ozkan onur-ozkan deleted the do-not-force-unstable-if-unmarked branch January 27, 2024 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants