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

[u8] methods in libcore? #45803

Closed
SimonSapin opened this issue Nov 6, 2017 · 5 comments · Fixed by #49896
Closed

[u8] methods in libcore? #45803

SimonSapin opened this issue Nov 6, 2017 · 5 comments · Fixed by #49896
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@SimonSapin
Copy link
Contributor

Background on [T] methods

Inherent methods for slices are currently defined in liballoc with:

#[lang(slice)]
impl<T> [T] {
    // …
}

#[lang(slice)] is an escape hatch to allow that impl to exist. Normally it wouldn’t be allowed since liballoc is not the crate that defined slices types. (No such crate exists.) The compiler only allows one #[lang(slice)] item to exist, so we have it in liballoc where it can define every method we want slices to have.

So in #![no_std] crates that do not depend on liballoc, slices have no inherent methods. Instead, libcore has a core::slice::SliceExt trait implemented for [T] that has a subset of the methods and needs to be imported into scope to be used.

Background on [u8] methods

#44042 added inherent methods specific to byte slices. They’re also in liballoc, with a new dedicated lang item:

#[lang(slice_u8)]
impl [u8] {
    // …
}

But they’re not available without liballoc, although they don’t all depend on allocation.

Now

It’d be nice to have some of these [u8] methods without liballoc. The simple thing would be to continue this pattern and add a core::slice::SliceU8Ext trait, but that doesn’t seem like very good design.

Maybe it’s time to revisit SliceExt. Could the “only one #[lang(slice)]” restriction be lifted? Or could we add new core_slice and core_slice_u8 lang items?

CC @eddyb

@dtolnay dtolnay added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Nov 6, 2017
@eddyb
Copy link
Member

eddyb commented Nov 6, 2017

Ideally none of this would be done with lang items - maybe if we make inherent impls on builtin types unstable and check them like lang items ("link-time coherence") at the method level, instead of the whole impl - after all, only having methods with the same name conflicts.

@eddyb
Copy link
Member

eddyb commented Nov 6, 2017

cc @rust-lang/compiler

@kennytm kennytm added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 7, 2017
@TimNN TimNN added the C-feature-request Category: A feature request, i.e: not implemented / a PR. label Nov 7, 2017
@dtolnay dtolnay added C-feature-accepted Category: A feature request that has been accepted pending implementation. and removed C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Nov 18, 2017
@SimonSapin
Copy link
Contributor Author

Fixed in #49698 (with a new lang item).

@SimonSapin
Copy link
Contributor Author

Wait no. I’m confusing two pull requests, sorry.

@SimonSapin
Copy link
Contributor Author

#49896 fixes this.

SimonSapin added a commit to SimonSapin/rust that referenced this issue Apr 13, 2018
bors added a commit that referenced this issue Apr 14, 2018
Add inherent methods in libcore for [T], [u8], str, f32, and f64

# Background

Primitive types are defined by the language, they don’t have a type definition like `pub struct Foo { … }` in any crate. So they don’t “belong” to any crate as far as `impl` coherence is concerned, and on principle no crate would be able to define inherent methods for them, without a trait. Since we want these types to have inherent methods anyway, the standard library (with cooperation from the compiler) bends this rule with code like [`#[lang = "u8"] impl u8 { /*…*/ }`](https://github.com/rust-lang/rust/blob/1.25.0/src/libcore/num/mod.rs#L2244-L2245). The `#[lang]` attribute is permanently-unstable and never intended to be used outside of the standard library.

Each lang item can only be defined once. Before this PR there is one impl-coherence-rule-bending lang item per primitive type (plus one for `[u8]`, which overlaps with `[T]`). And so one `impl` block each. These blocks for `str`, `[T]` and `[u8]` are in liballoc rather than libcore because *some* of the methods (like `<[T]>::to_vec(&self) -> Vec<T> where T: Clone`) need a global memory allocator which we don’t want to make a requirement in libcore. Similarly, `impl f32` and `impl f64` are in libstd because some of the methods are based on FFI calls to C’s `libm` and we want, as much as possible, libcore not to require “runtime support”.

In libcore, the methods of `str` and `[T]` that don’t allocate are made available through two **unstable traits** `StrExt` and `SliceExt` (so the traits can’t be *named* by programs on the Stable release channel) that have **stable methods** and are re-exported in the libcore prelude (so that programs on Stable can *call* these methods anyway). Non-allocating `[u8]` methods are not available in libcore: #45803. Some `f32` and `f64` methods are in an unstable `core::num::Float` trait with stable methods, but that one is **not in the libcore prelude**. (So as far as Stable programs are concerns it doesn’t exist, and I don’t know what the point was to mark these methods `#[stable]`.)

#32110 is the tracking issue for these unstable traits.

# High-level proposal

Since the standard library is already bending the rules, why not bend them *a little more*? By defining a few additional lang items, the compiler can allow the standard library to have *two* `impl` blocks (in different crates) for some primitive types.

The `StrExt` and `SliceExt` traits still exist for now so that we can bootstrap from a previous-version compiler that doesn’t have these lang items yet, but they can be removed in next release cycle. (`Float` is used internally and needs to be public for libcore unit tests, but was already `#[doc(hidden)]`.) I don’t know if #32110 should be closed by this PR, or only when the traits are entirely removed after we make a new bootstrap compiler.

# Float methods

Among the methods of the `core::num::Float` trait, three are based on LLVM intrinsics: `abs`, `signum`, and `powi`. PR #27823 “Remove dependencies on libm functions from libcore” moved a bunch of `core::num::Float` methods back to libstd, but left these three behind. However they aren’t specifically discussed in the PR thread. The `compiler_builtins` crate defines `__powisf2` and `__powidf2` functions that look like implementations of `powi`, but I couldn’t find a connection with the `llvm.powi.f32` and `llvm.powi.f32` intrinsics by grepping through LLVM’s code.

In discussion starting at #32110 (comment) Alex says that we do not want methods in libcore that require “runtime support”, but it’s not clear whether that applies to these `abs`, `signum`, or `powi`. In doubt, I’ve **removed** them for the trait and moved them to inherent methods in libstd for now. We can move them back later (or in this PR) if we decide that’s appropriate.

# Change details

For users on the Stable release channel:

* I believe this PR does not make any breaking change
* Some methods for `[u8]`, `f32`, and `f64` are newly available to `#![no_std]` users (fixes #45803)
* There should be no visible change for `std` users in terms of what programs compile or what their behavior is. (Only in compiler error messages, possibly.)

For Nightly users, additionally:

* The unstable `StrExt` and `SliceExt` traits are gone
* Their methods are now inherent methods of `str` and `[T]` (so only code that explicitly named the traits should be affected, not "normal" method calls)
* The `abs`, `signum` and `powi` methods of the `Float` trait are gone
* The `Float` trait’s unstable feature name changed to `float_internals` with no associated tracking issue, to reflect it being a permanently unstable implementation detail rather than a public API on a path to stabilization.
* Its remaining methods are now inherent methods of `f32` and `f64`.

-----

CC @rust-lang/libs for the API changes, @rust-lang/compiler for the new lang items
SimonSapin added a commit to SimonSapin/rust that referenced this issue Apr 21, 2018
bors added a commit that referenced this issue Apr 22, 2018
Add inherent methods in libcore for [T], [u8], str, f32, and f64

# Background

Primitive types are defined by the language, they don’t have a type definition like `pub struct Foo { … }` in any crate. So they don’t “belong” to any crate as far as `impl` coherence is concerned, and on principle no crate would be able to define inherent methods for them, without a trait. Since we want these types to have inherent methods anyway, the standard library (with cooperation from the compiler) bends this rule with code like [`#[lang = "u8"] impl u8 { /*…*/ }`](https://github.com/rust-lang/rust/blob/1.25.0/src/libcore/num/mod.rs#L2244-L2245). The `#[lang]` attribute is permanently-unstable and never intended to be used outside of the standard library.

Each lang item can only be defined once. Before this PR there is one impl-coherence-rule-bending lang item per primitive type (plus one for `[u8]`, which overlaps with `[T]`). And so one `impl` block each. These blocks for `str`, `[T]` and `[u8]` are in liballoc rather than libcore because *some* of the methods (like `<[T]>::to_vec(&self) -> Vec<T> where T: Clone`) need a global memory allocator which we don’t want to make a requirement in libcore. Similarly, `impl f32` and `impl f64` are in libstd because some of the methods are based on FFI calls to C’s `libm` and we want, as much as possible, libcore not to require “runtime support”.

In libcore, the methods of `str` and `[T]` that don’t allocate are made available through two **unstable traits** `StrExt` and `SliceExt` (so the traits can’t be *named* by programs on the Stable release channel) that have **stable methods** and are re-exported in the libcore prelude (so that programs on Stable can *call* these methods anyway). Non-allocating `[u8]` methods are not available in libcore: #45803. Some `f32` and `f64` methods are in an unstable `core::num::Float` trait with stable methods, but that one is **not in the libcore prelude**. (So as far as Stable programs are concerns it doesn’t exist, and I don’t know what the point was to mark these methods `#[stable]`.)

#32110 is the tracking issue for these unstable traits.

# High-level proposal

Since the standard library is already bending the rules, why not bend them *a little more*? By defining a few additional lang items, the compiler can allow the standard library to have *two* `impl` blocks (in different crates) for some primitive types.

The `StrExt` and `SliceExt` traits still exist for now so that we can bootstrap from a previous-version compiler that doesn’t have these lang items yet, but they can be removed in next release cycle. (`Float` is used internally and needs to be public for libcore unit tests, but was already `#[doc(hidden)]`.) I don’t know if #32110 should be closed by this PR, or only when the traits are entirely removed after we make a new bootstrap compiler.

# Float methods

Among the methods of the `core::num::Float` trait, three are based on LLVM intrinsics: `abs`, `signum`, and `powi`. PR #27823 “Remove dependencies on libm functions from libcore” moved a bunch of `core::num::Float` methods back to libstd, but left these three behind. However they aren’t specifically discussed in the PR thread. The `compiler_builtins` crate defines `__powisf2` and `__powidf2` functions that look like implementations of `powi`, but I couldn’t find a connection with the `llvm.powi.f32` and `llvm.powi.f32` intrinsics by grepping through LLVM’s code.

In discussion starting at #32110 (comment) Alex says that we do not want methods in libcore that require “runtime support”, but it’s not clear whether that applies to these `abs`, `signum`, or `powi`. In doubt, I’ve **removed** them for the trait and moved them to inherent methods in libstd for now. We can move them back later (or in this PR) if we decide that’s appropriate.

# Change details

For users on the Stable release channel:

* I believe this PR does not make any breaking change
* Some methods for `[u8]`, `f32`, and `f64` are newly available to `#![no_std]` users (fixes #45803)
* There should be no visible change for `std` users in terms of what programs compile or what their behavior is. (Only in compiler error messages, possibly.)

For Nightly users, additionally:

* The unstable `StrExt` and `SliceExt` traits are gone
* Their methods are now inherent methods of `str` and `[T]` (so only code that explicitly named the traits should be affected, not "normal" method calls)
* The `abs`, `signum` and `powi` methods of the `Float` trait are gone
* The `Float` trait’s unstable feature name changed to `float_internals` with no associated tracking issue, to reflect it being a permanently unstable implementation detail rather than a public API on a path to stabilization.
* Its remaining methods are now inherent methods of `f32` and `f64`.

-----

CC @rust-lang/libs for the API changes, @rust-lang/compiler for the new lang items
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-accepted Category: A feature request that has been accepted pending implementation. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants