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

Add a policy for inclusion in the prelude #66

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

joshtriplett
Copy link
Member

Thus far, we've added items to the prelude via ad-hoc policy, and we have not
necessarily had consensus on the underlying unwritten policy, only on
the application of that unwritten policy to individual prelude proposals.

This PR adds a proposed policy for including items into the prelude. This
policy also covers when we should add items to the common prelude versus an
edition-specific prelude.

@joshtriplett joshtriplett added T-libs-api S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 13, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 14, 2024
[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.
@ChrisDenton
Copy link
Member

It seems that there can be some issues with name resolution when adding types to the prelude: rust-lang/rust#125107 (comment)

The crater run has Cell and Duration in the prelude. While these types probably have too a generic a name to be included, they do make for a good stress test. I think the fact that they lead to errors with disambiguation suggests we do perhaps need to exercise some caution when adding types to the prelude.

bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2024
[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.
@BurntSushi
Copy link
Member

We should only add an item to the prelude if some reasonable number of crates
are likely to use the item. It need not be an item used by the majority of
crates, but it should be reasonably frequent across the ecosystem.

I think I agree with everything in this policy, but for me personally, I consider this is a necessary but sufficient criteria. In particular, in my opinion, I think there are costs to having a large prelude and I'm not sure this policy accounts for that at all. That is, as the prelude grows in size, I think our threshold for adding to it should also go up. That is, our standards should become higher.

I don't know whether there is consensus on that point such that it can be added to the policy as written though. Perhaps just the "necessary but not sufficient" language would work. I'm not sure.

@joshtriplett
Copy link
Member Author

@ChrisDenton wrote:

It seems that there can be some issues with name resolution when adding types to the prelude: rust-lang/rust#125107 (comment)

The crater run has Cell and Duration in the prelude. While these types probably have too a generic a name to be included, they do make for a good stress test. I think the fact that they lead to errors with disambiguation suggests we do perhaps need to exercise some caution when adding types to the prelude.

Fascinating.

I think we should leave aside the issue of conflicts caused by naming a local variable the same as a unit struct (e.g. let PhantomData = (); or let (PhantomData @ _) = ();). Those don't seem particularly likely for multiple reasons, and I'd propose that we should add them to "acceptable breakage" since there's an obvious way to avoid ever encountering that (don't name a variable LikeAType).

The issue of use EnumType::Variant; for an EnumType that conflicts with a type in the prelude is absolutely real breakage. I'm going to file an issue for about investigating that source of breakage and what we could potentially do about it: rust-lang/rust#127738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants