[DO NOT MERGE] Expand core's prelude with more types #125107

ChrisDenton · 2024-05-14T06:37:34Z

This adds some more types to the core prelude, to explore the proposed prelude policy.

Without any further context, types in the standard library are strongly associated with the standard library so they are good candidates for the prelude, assuming their name doesn't require a module to make sense of. As a bonus this avoids some of the repetition required for cell::Cell, pin::Pin, atomic::Atomic*, etc.

Currently this includes some nightly types. These should be removed before this is merged.

In summary, this PR currently exports the following from the prelude:

pub use core::cell::{Cell, LazyCell, OnceCell, RefCell, SyncUnsafeCell, UnsafeCell};
pub use core::ffi::{
    c_char, c_double, c_float, c_int, c_long, c_longlong, c_ptrdiff_t, c_schar, c_short, c_size_t,
    c_ssize_t, c_str, c_uchar, c_uint, c_ulong, c_ulonglong, c_ushort, c_void, CStr,
};
pub use core::io::{BorrowedBuf, BorrowedCursor};
pub use core::marker::{PhantomData, PhantomPinned};
pub use core::mem::{ManuallyDrop, MaybeUninit};
pub use core::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6};
pub use core::num::{
    NonZero, NonZeroI128, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI8, NonZeroIsize, NonZeroU128,
    NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize,
};
pub use core::panic::PanicInfo;
pub use core::pin::Pin;
pub use core::ptr::NonNull;
pub use core::sync::atomic::{
    AtomicBool, AtomicI16, AtomicI32, AtomicI64, AtomicI8, AtomicIsize, AtomicPtr, AtomicU16,
    AtomicU32, AtomicU64, AtomicU8, AtomicUsize,
};
pub use core::time::Duration;
pub use core::ops::ControlFlow;

UPDATE:

There have so far been concerns raised about the following types:

Cell is maybe too generic a word to be in a prelude type
It is common for time libs to make their own Duration type so having it in the prelude may be confusing. This perhaps also suggests the design of std's Duration is not ideal but in any case that can't be changed at this point.

rustbot · 2024-05-14T06:37:42Z

r? @Nilstrieb

rustbot has assigned @Nilstrieb.
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

workingjubilee

We should probably bias most heavily to the "this is basically a kind of primitive to the language runtime" cases. i.e. there may be other reasonable impls of Cell out there, but there sure isn't another one of UnsafeCell.

workingjubilee · 2024-05-14T06:42:54Z

library/core/src/prelude/common.rs

+
+#[stable(feature = "core_prelude_extra", since = "CURRENT_RUSTC_VERSION")]
+#[doc(no_inline)]
+pub use crate::marker::{PhantomData, PhantomPinned};


...yeah, really want these two.

workingjubilee · 2024-05-14T06:46:03Z

library/core/src/prelude/common.rs

@@ -41,6 +41,67 @@ pub use crate::option::Option::{self, None, Some};
 #[doc(no_inline)]
 pub use crate::result::Result::{self, Err, Ok};

+#[stable(feature = "core_prelude_extra", since = "CURRENT_RUSTC_VERSION")]
+#[doc(no_inline)]
+pub use crate::cell::{Cell, LazyCell, OnceCell, RefCell, SyncUnsafeCell, UnsafeCell};


hmm, the OnceCell case seems like it may risk confusion due to once_cell.

Hm, I think that the parts of once_cell that overlap with std are considered deprecated in favour of std (MSRV willing).

I mean the author basically contributed the code, so yeah.

workingjubilee · 2024-05-14T06:48:52Z

library/core/src/prelude/common.rs

+
+#[stable(feature = "core_prelude_extra", since = "CURRENT_RUSTC_VERSION")]
+#[doc(no_inline)]
+pub use crate::time::Duration;


This seems like it would risk confusion because time::Duration exists.

That feels like an awkward one because, without further context, time::Duration is ambiguous too. I.e. is this the time crate or is there a use std::time somewhere?

right.

and don't forget chrono::Duration.

Ok, I'm willing to see if crater has anything to say about this 😛

workingjubilee · 2024-05-14T06:51:36Z

Obviously the crater run(?) will be educational in any case.

ChrisDenton · 2024-05-14T07:26:27Z

This will break src\tools\clippy\tests\ui\crashes\ice-6252.rs but I'm not sure if fixing would defeat the purpose of the test.

workingjubilee · 2024-05-14T07:34:58Z

This will break src\tools\clippy\tests\ui\crashes\ice-6252.rs but I'm not sure if fixing would defeat the purpose of the test.

ChrisDenton · 2024-05-14T18:31:29Z

@bors try

[DO NOT MERGE] Expand core's prelude with more types This adds some more types to the core prelude, to explore the [proposed prelude policy](rust-lang/std-dev-guide#66). Without any further context, types in the standard library are strongly associated with the standard library so they are good candidates for the prelude, assuming their name doesn't require a module to make sense of. As a bonus this avoids some of the repetition required for `cell::Cell`, `pin::Pin`, `atomic::Atomic*`, etc. Currently this includes some nightly types. These should be removed before this is merged.

bors · 2024-05-14T18:32:41Z

⌛ Trying commit a222427 with merge 9130c02...

bors · 2024-05-14T20:35:24Z

☀️ Try build successful - checks-actions
Build commit: 9130c02 (9130c02509ce15f69dc5da6359bb9d140d41d4ac)

ChrisDenton · 2024-05-15T04:53:57Z

@craterbot check

craterbot · 2024-05-15T04:54:04Z

👌 Experiment pr-125107 created and queued.
🤖 Automatically detected try build 9130c02
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-05-19T15:01:37Z

🚧 Experiment pr-125107 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-05-21T11:05:55Z

🎉 Experiment pr-125107 is completed!
📊 159 regressed and 2 fixed (462054 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

ChrisDenton · 2024-05-21T12:17:47Z

Huh, there does seem to be genuine regressions there. E.g. This is ambiguous if Cell is in the prelude:

use Cell::A;

// The derive here is important but it doesn't particularly matter which trait is being derived.
#[derive(PartialEq)]
pub enum Cell {
    A
}

Example error message from crater:

[INFO] [stdout] error[E0659]: `Cell` is ambiguous
[INFO] [stdout]   --> src/puzzle.rs:5:5
[INFO] [stdout]    |
[INFO] [stdout] 5  | use Cell::{Empty, Unknown, ValA, ValB, ValC, ValD};
[INFO] [stdout]    |     ^^^^ ambiguous name
[INFO] [stdout]    |
[INFO] [stdout]    = note: ambiguous because of a conflict between a macro-expanded name and a less macro-expanded name from outer scope during import or macro resolution
[INFO] [stdout] note: `Cell` could refer to the enum defined here
[INFO] [stdout]   --> src/puzzle.rs:8:1
[INFO] [stdout]    |
[INFO] [stdout] 8  | / pub enum Cell {
[INFO] [stdout] 9  | |     ValA,
[INFO] [stdout] 10 | |     ValB,
[INFO] [stdout] 11 | |     ValC,
[INFO] [stdout] ...  |
[INFO] [stdout] 14 | |     Unknown,
[INFO] [stdout] 15 | | }
[INFO] [stdout]    | |_^
[INFO] [stdout]    = help: use `self::Cell` to refer to this enum unambiguously
[INFO] [stdout] note: `Cell` could also refer to a struct from prelude
[INFO] [stdout]   --> /rustc/9130c02509ce15f69dc5da6359bb9d140d41d4ac/library/std/src/prelude/mod.rs:148:13

Which means that the prelude does have more of an effect on name resolution than I realized.

workingjubilee · 2024-05-21T19:55:37Z

Yeah, I figured Cell wouldn't be an option, not in a zillion years. It's not as ubiquitous as Node, but almost.

workingjubilee · 2024-05-21T19:59:20Z

people are denying unused_qualifications? on g-d?

ChrisDenton · 2024-05-21T20:02:21Z

Yeah, I figured Cell wouldn't be an option, not in a zillion years. It's not as ubiquitous as Node, but almost.

I was kinda hoping the prelude was special enough that this sort of issue didn't arise even if there are other good reasons not to have it in the prelude. But alas adding types to the prelude does come with some amount of risk.

people are denying unused_qualifications? on g-d?

I was kinda ignoring them because they're getting exactly what they're asking for 😉

workingjubilee · 2024-05-21T20:40:50Z

@ChrisDenton Can you remove Cell and Duration and anything else that seems dicey to you, and we retry this with the smaller list? There's a way to tell craterbot "rerun this on just the regressions".

ChrisDenton · 2024-05-21T21:21:04Z

Ok, I've removed a few things. I'm ignoring the unnecessary qualifications lint for now because if not breaking that is a requirement then we can never add anything to the prelude (except in a new edition).

I'm not sure about removing PhantomData. It's only two crates that break: one is outdated and the other one hasn't been updated in a couple of years.

ChrisDenton · 2024-05-21T21:22:49Z

@bors try while I figure out the crater magic

[DO NOT MERGE] Expand core's prelude with more types This adds some more types to the core prelude, to explore the [proposed prelude policy](rust-lang/std-dev-guide#66). Without any further context, types in the standard library are strongly associated with the standard library so they are good candidates for the prelude, assuming their name doesn't require a module to make sense of. As a bonus this avoids some of the repetition required for `cell::Cell`, `pin::Pin`, `atomic::Atomic*`, etc. Currently this includes some nightly types. These should be removed before this is merged. In summary, this PR currently exports the following from the prelude: ```rust pub use core::cell::{Cell, LazyCell, OnceCell, RefCell, SyncUnsafeCell, UnsafeCell}; pub use core::ffi::{ c_char, c_double, c_float, c_int, c_long, c_longlong, c_ptrdiff_t, c_schar, c_short, c_size_t, c_ssize_t, c_str, c_uchar, c_uint, c_ulong, c_ulonglong, c_ushort, c_void, CStr, }; pub use core::io::{BorrowedBuf, BorrowedCursor}; pub use core::marker::{PhantomData, PhantomPinned}; pub use core::mem::{ManuallyDrop, MaybeUninit}; pub use core::net::{IpAddr, Ipv4Addr, Ipv6Addr, SocketAddr, SocketAddrV4, SocketAddrV6}; pub use core::num::{ NonZero, NonZeroI128, NonZeroI16, NonZeroI32, NonZeroI64, NonZeroI8, NonZeroIsize, NonZeroU128, NonZeroU16, NonZeroU32, NonZeroU64, NonZeroU8, NonZeroUsize, }; pub use core::panic::PanicInfo; pub use core::pin::Pin; pub use core::ptr::NonNull; pub use core::sync::atomic::{ AtomicBool, AtomicI16, AtomicI32, AtomicI64, AtomicI8, AtomicIsize, AtomicPtr, AtomicU16, AtomicU32, AtomicU64, AtomicU8, AtomicUsize, }; pub use core::time::Duration; pub use core::ops::ControlFlow; ``` UPDATE: There have so far been concerns raised about the following types: - `Cell` is maybe too generic a word to be in a prelude type - It is common for time libs to make their own `Duration` type so having it in the prelude may be confusing. This perhaps also suggests the design of std's `Duration` is not ideal but in any case that can't be changed at this point.

bors · 2024-05-21T21:24:02Z

⌛ Trying commit 8b2e9b1 with merge 341f4bd...

workingjubilee · 2024-05-21T21:30:47Z

the relevant command looks like run mode=check crates=https://crater-reports.s3.amazonaws.com/pr-111063/retry-regressed-list.txt except I think you will want to pick uhhh the correct .txt instead of that one. :P

ChrisDenton · 2024-05-21T21:33:13Z

Ah, I'll try that once the try build is finished.

bors · 2024-05-21T22:59:54Z

☀️ Try build successful - checks-actions
Build commit: 341f4bd (341f4bdd14d8eff23af8ad0599c0e6f42d805538)

ChrisDenton · 2024-05-21T23:00:21Z

@craterbot check crates=https://crater-reports.s3.amazonaws.com/pr-125107/retry-regressed-list.txt

craterbot · 2024-05-21T23:00:24Z

👌 Experiment pr-125107-1 created and queued.
🤖 Automatically detected try build 341f4bd
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

ChrisDenton · 2024-05-22T09:24:17Z

Now that I look closer at the crater results, it seems all the Duration issues are caused by a single crate (embedded-time), which does the equivalent of:

mod duration {
    pub trait Duration {}
}
use duration::*;
use core::prelude::v1::*;

pub struct Timer<Dur: Duration> {
    dur: Dur
}

Which leads to "error[E0659]: Duration is ambiguous" and is responsible for a large chunk of the failures.

And just for completeness, here are minimized examples of the other failures...

error[E0530]: let bindings cannot shadow unit structs:

fn main() {
    let (PhantomData @ _) = ();
}

error[E0308]: mismatched types:

fn main() {
    let PhantomData = ();
}

craterbot · 2024-05-28T14:32:45Z

🚧 Experiment pr-125107-1 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2024-05-28T16:39:39Z

🎉 Experiment pr-125107-1 is completed!
📊 25 regressed and 0 fixed (159 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

ChrisDenton · 2024-05-28T17:14:38Z

Ok, that's down to 25 regressions. 18 of those are caused by using #![deny(unused_qualifications)] or worse #![deny(warnings)].

The other failures are all caused by something like let PhatomData = ... in the two crates: safer_ffi-proc_macros-0.1.8 (an old version) and fuzzcheck_mutators_derive-0.12.0 (latest but not updated in years).

workingjubilee · 2024-05-28T20:31:10Z

Time to send https://github.com/loiclec/fuzzcheck-rs a PR I guess.

ChrisDenton added 2 commits May 14, 2024 06:18

Expand core's prelude with more types

020292c

bless you

b0b75de

rustbot assigned Nilstrieb May 14, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels May 14, 2024

This comment has been minimized.

Sign in to view

workingjubilee reviewed May 14, 2024

View reviewed changes

clippy lints

bacb3e3

This comment has been minimized.

Sign in to view

ChrisDenton force-pushed the core-prelude branch from 54181dc to bacb3e3 Compare May 14, 2024 07:33

This comment has been minimized.

Sign in to view

fix ice-6252

fcbcdd4

ChrisDenton force-pushed the core-prelude branch from aaa017b to fcbcdd4 Compare May 14, 2024 09:57

Update conf_disallowed_types.stderr

a222427

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 21, 2024

ChrisDenton mentioned this pull request May 21, 2024

Add a policy for inclusion in the prelude rust-lang/std-dev-guide#66

Open

Remove some types from the prelude

8b2e9b1

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2024

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 28, 2024

[DO NOT MERGE] Expand core's prelude with more types #125107

Are you sure you want to change the base?

[DO NOT MERGE] Expand core's prelude with more types #125107

Conversation

ChrisDenton commented May 14, 2024 • edited

rustbot commented May 14, 2024

This comment has been minimized.

workingjubilee 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

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

workingjubilee commented May 14, 2024

This comment has been minimized.

ChrisDenton commented May 14, 2024

workingjubilee commented May 14, 2024

This comment has been minimized.

This comment has been minimized.

ChrisDenton commented May 14, 2024

bors commented May 14, 2024

bors commented May 14, 2024

ChrisDenton commented May 15, 2024

craterbot commented May 15, 2024

craterbot commented May 19, 2024

craterbot commented May 21, 2024

ChrisDenton commented May 21, 2024 • edited

workingjubilee commented May 21, 2024 • edited

workingjubilee commented May 21, 2024

ChrisDenton commented May 21, 2024

workingjubilee commented May 21, 2024 • edited

ChrisDenton commented May 21, 2024

ChrisDenton commented May 21, 2024

bors commented May 21, 2024

workingjubilee commented May 21, 2024

ChrisDenton commented May 21, 2024

bors commented May 21, 2024

ChrisDenton commented May 21, 2024

craterbot commented May 21, 2024

ChrisDenton commented May 22, 2024 • edited

craterbot commented May 28, 2024

craterbot commented May 28, 2024

ChrisDenton commented May 28, 2024 • edited

workingjubilee commented May 28, 2024

ChrisDenton commented May 14, 2024 •

edited

ChrisDenton commented May 21, 2024 •

edited

workingjubilee commented May 21, 2024 •

edited

workingjubilee commented May 21, 2024 •

edited

ChrisDenton commented May 22, 2024 •

edited

ChrisDenton commented May 28, 2024 •

edited