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 inherent methods in libcore for [T], [u8], str, f32, and f64 #49896

Merged
merged 7 commits into from Apr 22, 2018

Conversation

SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Apr 12, 2018

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 { /*…*/ }. 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 [u8] methods in libcore? #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 SimonSapin added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 12, 2018
@rust-highfive
Copy link
Collaborator

r? @dtolnay

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 12, 2018
@SimonSapin
Copy link
Contributor Author

r? @alexcrichton

@bors
Copy link
Contributor

bors commented Apr 13, 2018

☔ The latest upstream changes (presumably #49389) made this pull request unmergeable. Please resolve the merge conflicts.

@clarfonthey
Copy link
Contributor

  • The unstable StrExt and SliceExt traits are gone

I'd recommend doing a crater run for this; while people shouldn't be relying on the traits being exported as stable in the prelude, in practice someone might reference core::prelude::v1::StrExt, which is currently allowed on stable.

@SimonSapin
Copy link
Contributor Author

reference core::prelude::v1::StrExt, which is currently allowed on stable.

No it’s not, since the trait is unstable.

error[E0658]: use of unstable library feature 'core_str_ext': stable interface provided by `impl str` in later crates (see issue #32110)
 --> a.rs:2:5
  |
2 | use core::prelude::v1::StrExt;
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

@ollie27
Copy link
Member

ollie27 commented Apr 14, 2018

There is a stability hole (#15702) that means the following does compile on stable:

#![crate_type = "rlib"]
#![no_std]
pub fn foo() {
    assert!(::core::prelude::v1::CharExt::is_digit('1', 10));
    assert!(::core::prelude::v1::StrExt::is_empty(""));
    assert!(!::core::num::Float::is_nan(1.0));
}

@SimonSapin
Copy link
Contributor Author

@ollie27 That’s… kinda horrifying :( But also there’s an argument that the stability promise does not extend to cases where we accidentally fail to prevent usage of features documented as unstable?

@clarfonthey
Copy link
Contributor

That's mostly why I recommended a crater run; while this would definitely be a hack that's not guaranteed to work, it'd be best to see if any crates in the ecosystem abuse this.

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Apr 14, 2018

@bors: try

@rust-lang/infra Could you do a Crater run please?

@bors
Copy link
Contributor

bors commented Apr 14, 2018

⌛ Trying commit 5eea099 with merge 968de88...

bors added a commit that referenced this pull request 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
@kennytm kennytm 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 Apr 14, 2018
@bors
Copy link
Contributor

bors commented Apr 14, 2018

☀️ Test successful - status-travis
State: approved= try=True

@SimonSapin
Copy link
Contributor Author

Actually, let’s not wait on Crater for this. Abusing bugs in the stability should not be sufficient for stuff to be considered stable: rust-lang/rfcs#2405

@SimonSapin SimonSapin 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 Apr 17, 2018
@@ -48,13 +48,13 @@ macro_rules! fake_anon_field_expr {

macro_rules! real_method_stmt {
() => {
2.0.powi(2) //~ ERROR can't call method `powi` on ambiguous numeric type `{float}`
2.0.recip() //~ ERROR can't call method `recip` on ambiguous numeric type `{float}`
Copy link
Member

Choose a reason for hiding this comment

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

How come these changes ended up being necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At least one of the modified tests is #![no_std] and powi has been removed form libcore as discussed above. However this one isn’t and if I revert this diff chuck the error message is error[E0599]: no method named `powi` found for type `{float}` in the current scope instead of the expected error[E0689]: can't call method `powi` on ambiguous numeric type `{float}` . I don’t really understand why right now, I’ll need to investigate…

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok that makes sense. I think it's ok to not worry much about the error here, it probably has to do with trait vs inherent dispatch and doesn't seem to indicate a bug either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After much squinting at rustc code, I found out that not only E0599 only looks at traits and not inherent impls to see if the method exists (which explains this change), but it doesn’t even check if those traits are are implemented for any relevant type. And only error messages are affected, not actual name resolution, so leaving this as is is less worrying that I thought yesterday.

@kennytm kennytm 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 Apr 17, 2018
@bors
Copy link
Contributor

bors commented Apr 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d5616e1 to master...

@bors bors merged commit 70fdd1b into rust-lang:master Apr 22, 2018
@SimonSapin SimonSapin deleted the inherent branch April 22, 2018 08:06
phil-opp added a commit to embed-rs/stb_truetype-rs that referenced this pull request Apr 27, 2018
@bluss bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Apr 29, 2018
kngwyu added a commit to kngwyu/racer-nightly that referenced this pull request May 3, 2018
now by rust-lang/rust#49896 some string
methods like `starts_with` or `as_bytes` are declared in the macro
str_core_methods and racer can't complete these methods, so I modified tests.
kngwyu added a commit to kngwyu/racer-nightly that referenced this pull request May 11, 2018
Now by rust-lang/rust#49896 some string
methods like `starts_with` or `as_bytes` are declared in the macro
str_core_methods and racer can't complete these methods.
SimonSapin added a commit to SimonSapin/rust that referenced this pull request May 22, 2018
bors added a commit that referenced this pull request May 23, 2018
Remove the unstable Float trait

Following up to #49896 and #50629. Fixes #32110.
@XAMPPRocky
Copy link
Member

@bluss You gave this issue relnotes but seems like everything in it is unstable. Is there a specific reason you wanted this included?

@kennytm
Copy link
Member

kennytm commented May 29, 2018

@Aaronepower The relnote is that #49896 (comment) will no longer be accepted.

@SimonSapin
Copy link
Contributor Author

I don’t know if it’s relnotes-notable, but there’s this change:

For users on the Stable release channel:

[…]

@XAMPPRocky
Copy link
Member

@SimonSapin Ah okay, I thought the float & slice methods on core were still unstable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[u8] methods in libcore?