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

allow inherent implementations on primitives, remove some extension traits #23104

Merged
merged 15 commits into from Mar 17, 2015

Conversation

@japaric
Copy link
Member

commented Mar 6, 2015

  • Allow inherent implementations on char, str, [T], *const T, *mut T and all the numeric primitives.
  • copy unicode::char::CharExt methods into impl char
  • remove unicode::char::CharExt, its re-export std::char::CharExt and CharExt from the prelude
  • copy collections::str::StrExt methods into impl str
  • remove collections::str::StrExt its re-export std::str::StrExt, and StrExt from the prelude
  • copy collections::slice::SliceExt methods into impl<T> [T]
  • remove collections::slice::SliceExt its re-export std::slice::SliceExt, and SliceExt from the prelude
  • copy core::ptr::PtrExt methods into impl<T> *const T
  • remove core::ptr::PtrExt its re-export std::ptr::PtrExt, and PtrExt from the prelude
  • copy core::ptr::PtrExt and core::ptr::MutPtrExt methods into impl<T> *mut T
  • remove core::ptr::MutPtrExt its re-export std::ptr::MutPtrExt, and MutPtrExt from the prelude
  • copy core::num::Int and core::num::SignedInt methods into impl i{8,16,32,64,size}
  • copy core::num::Int and core::num::UnsignedInt methods into impl u{8,16,32,64,size}
  • remove core::num::UnsignedInt and its re-export std::num::UnsignedInt
  • move collections tests into its own crate: collectionstest
  • copy std::num::Float methods into impl f{32,64}

Because this PR removes several traits, this is a [breaking-change], however functionality remains unchanged and breakage due to unresolved imports should be minimal. If you encounter an error due to an unresolved import, simply remove the import:

  fn main() {
-     use std::num::UnsignedInt;  //~ error: unresolved import `std::num::UnsignedInt`.
-
      println!("{}", 8_usize.is_power_of_two());
  }

cc #16862
preview docs
unicode::char
collections::str
std::f32

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Mar 6, 2015

r? @brson

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

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2015

Oh, and I haven't check what the documentation looks like. I expect that either (a) the inherent impl won't show up, (b) rustdoc will ICE or (c) the inherent impl will magically appear in libunicode.

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2015

re: docs. The char primitive appears in libunicode but it only shows the impl char methods. There is another char primitive in libstd, that one shows the impl char methods and other methods provided by traits.

@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2015

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

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2015

This...is remarkably simple. I'm in favor.

@aturon

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

@japaric Sorry for not leaving a comment earlier -- I thought I had.

I'm delighted to see this! It will definitely help remove a few warts in the final days of stabilization. I talked with @alexcrichton about it at lunch today and he's also on board, so since @nikomatsakis approves I'd say let's roll this out across the full set of primitive types (including numerics). Thanks!

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

One part I'll also point out is that in the past when I dabbled with inherent methods on primitives I had to add special code to rustdoc to get things to work out. That may no longer be necessary, but could you give the primitive pages a quick glance to make sure all the methods are listed?

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2015

@aturon Alright, I'll look into it. One worry is that I think it may not be possible to Int/Float right off the bat, because they are used as bounds in some places (libtest comes to mind). So, I'll add the impl u8/f32/etc but leave those traits around. We can decide how to deal with them later, perhaps move them to rust-lang/num? Since I think libtest will also be moved out of rust-lang/rust.

@alexcrichton

could you give the primitive pages a quick glance to make sure all the methods are listed?

I can't check right now, but I did check them before, and IIRC all the methods were listed. The issue was that there were two primitive char pages (see my previous comment). I'll look into this, and include a link a github page with all the docs for the review of the final version of this PR. Thanks for the heads up!

@aturon

This comment has been minimized.

Copy link
Member

commented Mar 10, 2015

@japaric

Alright, I'll look into it. One worry is that I think it may not be possible to Int/Float right off the bat, because they are used as bounds in some places (libtest comes to mind). So, I'll add the impl u8/f32/etc but leave those traits around. We can decide how to deal with them later, perhaps move them to rust-lang/num? Since I think libtest will also be moved out of rust-lang/rust.

Right, that's pretty much exactly the rollout plan I had in mind. Part of the motivation here is to do away with those traits within std itself, pushing this concern to the crates.io ecosystem. If you can introduce the inherent methods, I can take care of the rest.

@japaric japaric force-pushed the japaric:inherent branch from b0f87e5 to 3acfd1b Mar 11, 2015

@japaric japaric changed the title [POC] Allow inherent `impl char`, remove `unicode::CharExt` allow inherent implementations on primitives, remove some extension traits Mar 11, 2015

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2015

OK, this is mostly done. There is a summary of the changes at the top, along with a preview of the docs (I haven't look at them closely yet). This branch passes check-stage1 as it is but needs a rebase.

There is one caveat: Because libcollections now contains lang items, is not easy to run its unit tests with rustc --test because it results in conflicting lang items - this is a known issue. I discussed this with @alexcrichton, and we decided that easiest path forward was to split the unit tests into its own crate (a la core and coretest). However, after the split some unit tests no longer compile because they require access to the private fields of some structs. For now I've commented out these tests, but these tests need to be re-enabled before landing this.

How to deal with these tests? I see two alternatives right now: (a) we rewrite the tests to not access private fields, not sure if this is possible for all the tests, or (b) we move these tests back to libcollections, making this work can be tricky due to the original problem of conflicting lang items.

I'll take a look at the second option tonight.

cc @gankro Do you think it would be possible to modify the commented out tests [1] to not access private fields while retaining the intention of the tests?

[1] See the "extract libcollections tests into libcollectionstest" commit, the tests are marked with "FIXME(japaric) privacy".

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 11, 2015

we rewrite the tests to not access private fields, not sure if this is possible for all the tests

I would favor this wherever possible, although it probably depends on the test.

we move these tests back to libcollections, making this work can be tricky due to the original problem of conflicting lang items

For a particular test it may not be too hard to get it to work, even with lang items. But it also depends on the test :)

@Gankra

This comment has been minimized.

Copy link
Contributor

commented Mar 11, 2015

Oof, migrating some of these tests would be really nasty. One option is to expose privates in doc(hidden) methods. The maintenance burden would be minimal, and anyone who uses them would be super at fault.

For specific tests:

The Vec::permutations one I'd probably just toss the offending check as "whatever" (I'd also toss the permutations api myself but that's just me).

LinkedList's integrity check could be made a doc(hidden) test, although honestly I don't think it's particularly necessary to perform the check at all. LinkedList is my nemesis but it's not exactly mind-blowing complexity. Also you can emulate the checks by just providing a slice of the expected values and iterating forwards and backwards.

VecDeque is probably the toughest case. We totally abuse access to the internals for two things: minimum-cost bench resets, and enumerating every possible internal state of the Deque. In theory you could do a whole mess of pushy-pops to slide the head of the list to exactly the right position, but that would perhaps be a bit fragile on top of being slower and more tedious. The check that head < tail is mostly gratuitous, although I think it was added in response to an actual impl bug.

I don't see why enum_set is commented out; it looks like everything there is using the public API?

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

One option is to expose privates in doc(hidden) methods.

I'm generally opposed to this idea, #[doc(hidden)] public API is still public API no matter how many underscores you add. I understand this is a necessary workaround for things like macros, but I'd like to avoid it in this case.

I don't see why enum_set is commented out; it looks like everything there is using the public API?

My bad, I was importing std::collections::EnumSet, but just realized that I want collections::EnumSet because there is no std re-export in this case.

Oof, migrating some of these tests would be really nasty.

Ok, I'll try to move these tests back to libcollections then.

Thanks for the input @gankro!

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

I've re-enabled the tests after moving them back to libcollections (see two last commits). I'll double check the docs and rebase after that. But the review process can already start ;-).

String { vec: <[_]>::to_vec(string.as_bytes()) }
}

// HACK: `impl [T]` is not available in cfg(test), use `::slice::to_vec` instead of

This comment has been minimized.

Copy link
@sfackler

sfackler Mar 12, 2015

Member

Is this just a staging issue?

This comment has been minimized.

Copy link
@japaric

japaric Mar 12, 2015

Author Member

Sadly, no. To avoid conflicting lang items during unit testing, I had to mark the impl [T] with cfg(not(test)). Therefore during testing the methods that were in impl [T] but not in core::slice::SliceExt need to be resupplied as free functions. These free functions are only ever available and used during testing - so they shouldn't leak into the public API. (I left a comment explaining this in libcollections/slice.rs - but you can't see that file here beacuse of the huge diff)

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

Woot!

Does rustdoc handle these impls in some non-terrible way?

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

@sfackler

Does rustdoc handle these impls in some non-terrible way?

Yes, they appear as a a normal impl str in the primitive page. There is a link to the full preview docs in the top comment.

@sfackler

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

Sweet. Looks good to me, but someone else should take a look at the rustc changes.

@japaric japaric force-pushed the japaric:inherent branch from c5dad2b to db1ee35 Mar 12, 2015

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2015

@sfackler Thanks for taking a look!

This has been carefully rebased over the char stabilization PR, now all the impl char methods have documentation. The preview docs will get updated in half an hour or so. This should be ready to go, who wants to r?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

I can r?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2015

@rust-highfive rust-highfive assigned nikomatsakis and unassigned brson Mar 12, 2015

#[unstable(feature = "collections",
reason = "needs investigation to see if to_string() can match perf")]
#[cfg(not(test))]
pub fn from_str(string: &str) -> String {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 12, 2015

Contributor

I am a bit confused -- why is this PR adding these fns?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 12, 2015

Contributor

oh, well, I guess they already exist -- I didn't (and don't) see them in the deletions though, which is weird.

mk/tests.mk Outdated
$(eval $(call RUST_CRATE,collectionstest))

TEST_TARGET_CRATES = $(filter-out core unicode,$(TARGET_CRATES)) \
collectionstest coretest

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Mar 12, 2015

Member

This spacing seems a little off, perhaps an editor gone awry?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Mar 12, 2015

I'm starting to somewhat have second thoughts about this from a compiler implementation perspective. From an API standpoint I think this change is fine largely because it doesn't really change much. Overall, however, this seems like it's all a bit of a hack.

We're basically bending the language to our whim to enable this sort of functionality because we control the compiler, but from the perspective of some other hypothetical compiler implementor this becomes quite a pain. We're adding all these special cases for these very implementation-specific set of impl blocks for each primitive. Put another way, we've found the language inadequate for our desires so we're getting around that with some slight of hand that doesn't come up too much but likely will at some point.

I think the biggest pain points for me when reading this are:

  • Such a huge number of lang items were added.
  • Multiple inherent implementations on primitives is allowed

The lang item problem can probably be considered an implementation detail of the compiler, but allowing multiple inherent implementations makes me feel like something has gone awry somewhere. We don't have the luxury of always being able to bend the language so API design is a little easier, and I felt that the previous trait-per-primitive-in-the-prelude solution was quite adequate: it worked out well in terms of documentation, functionality, extensibility, and it was also consistent with the rest of the language.

@japaric japaric force-pushed the japaric:inherent branch from 5a2833f to b65ebc4 Mar 17, 2015

@japaric

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2015

@bors: r=nikomatsakis b65ebc4

bors added a commit that referenced this pull request Mar 17, 2015
Auto merge of #23104 - japaric:inherent, r=nikomatsakis
- Allow inherent implementations on `char`, `str`, `[T]`, `*const T`, `*mut T` and all the numeric primitives.
- copy `unicode::char::CharExt` methods into `impl char`
- remove `unicode::char::CharExt`, its re-export `std::char::CharExt` and `CharExt` from the prelude
- copy `collections::str::StrExt` methods into `impl str`
- remove `collections::str::StrExt` its re-export `std::str::StrExt`, and `StrExt` from the prelude
- copy `collections::slice::SliceExt` methods into `impl<T> [T]`
- remove `collections::slice::SliceExt` its re-export `std::slice::SliceExt`, and `SliceExt` from the prelude
- copy `core::ptr::PtrExt` methods into `impl<T> *const T`
- remove `core::ptr::PtrExt` its re-export `std::ptr::PtrExt`, and `PtrExt` from the prelude
- copy `core::ptr::PtrExt` and `core::ptr::MutPtrExt` methods into `impl<T> *mut T`
- remove `core::ptr::MutPtrExt` its re-export `std::ptr::MutPtrExt`, and `MutPtrExt` from the prelude
- copy `core::num::Int` and `core::num::SignedInt` methods into `impl i{8,16,32,64,size}`
- copy `core::num::Int` and `core::num::UnsignedInt` methods into `impl u{8,16,32,64,size}`
- remove `core::num::UnsignedInt` and its re-export `std::num::UnsignedInt`
- move `collections` tests into its own crate: `collectionstest`
- copy `core::num::Float` methods into `impl f{32,64}`

Because this PR removes several traits, this is a [breaking-change], however functionality remains unchanged and breakage due to unresolved imports should be minimal. If you encounter an error due to an unresolved import, simply remove the import:

``` diff
  fn main() {
-     use std::num::UnsignedInt;  //~ error: unresolved import `std::num::UnsignedInt`.
-
      println!("{}", 8_usize.is_power_of_two());
  }
```

---

cc  #16862
[preview docs](http://japaric.github.io/inherent/std/index.html)
[unicode::char](http://japaric.github.io/inherent/unicode/primitive.char.html)
[collections::str](http://japaric.github.io/inherent/collections/primitive.str.html)
[std::f32](http://japaric.github.io/inherent/std/primitive.f32.html)
@bors

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2015

⌛️ Testing commit b65ebc4 with merge e466109...

@bors bors merged commit b65ebc4 into rust-lang:master Mar 17, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bombless bombless referenced this pull request Mar 19, 2015
@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2015

Are any crate now allowed to have inherent implementations for primitive types? If not, what is the rule that allows e.g. libunicode to do it?

@aturon

This comment has been minimized.

Copy link
Member

commented Mar 20, 2015

@SimonSapin The rule is effectively: a single crate within the std facade can define the implementation.

@eddyb

This comment has been minimized.

Copy link
Member

commented Mar 20, 2015

@SimonSapin To expand on @aturon's answer: it's done with #[lang=...] on the impls themselves.
Any lang item can be defined at most once in a crate hierarchy, and libstd (and all of its constituents, behind the facade) "happens" to have all the lang items taken for itself.

That means no crate using libstd can add methods to any of the primitives supporting this mechanism.
However, crates depending on only libcore can implement any of the lang items that are not in libcore (e.g. the ones for char and str).

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2015

That sounds reasonable. Thanks for the explanation!

mbrubeck added a commit to mbrubeck/rust that referenced this pull request Mar 23, 2015
Update docs for ptr module.
PR rust-lang#23104 moved `is_null` and `offset` to an inherent impl on the raw pointer
type.
mbrubeck added a commit to mbrubeck/rust that referenced this pull request Mar 23, 2015
Update docs for ptr module.
PR rust-lang#23104 moved `is_null` and `offset` to an inherent impl on the raw pointer
type.
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 24, 2015
rollup merge of rust-lang#23644: mbrubeck/doc-edit
PR rust-lang#23104 moved `is_null` and `offset` to an inherent impl on the raw pointer type.

I'm not sure whether or how it's possible to link to docs for that impl.

r? @steveklabnik
Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 24, 2015
Rollup merge of rust-lang#23644 - mbrubeck:doc-edit, r=steveklabnik
PR rust-lang#23104 moved `is_null` and `offset` to an inherent impl on the raw pointer type.

I'm not sure whether or how it's possible to link to docs for that impl.

r? @steveklabnik
kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Feb 22, 2017
TY: Improve autocompletion for primitive types
In Rust primitive types are only allowed a single
impl block that must be annotated with lang attribute
We do not enforce this yet an accept user defined impl blocks.

See rust-lang/rust#23104
and the error code E0390.

The primitive type bool does not have an impl block / lang attribute.

TODO:
* Clean up a lot
* Combine the 2 find methods
* Missing primitive types: slice, const_ptr, mut_ptr
* Autocompletions on u8, i8, ... do not work
  as they are implemented by a macro
* What fingerprint/string should we use for primitive types
  (to avoid overlap with user defined types)
* Incomplete tests
kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request Feb 24, 2017
TY: Improve autocompletion for primitive types
In Rust primitive types are only allowed a single
impl block that must be annotated with lang attribute
We do not enforce this yet an accept user defined impl blocks.

See rust-lang/rust#23104
and the error code E0390.

The primitive type bool does not have an impl block / lang attribute.

TODO:
* Clean up a lot
* Combine the 2 find methods
* Missing primitive types: slice, const_ptr, mut_ptr
* Autocompletions on u8, i8, ... do not work
  as they are implemented by a macro
* What fingerprint/string should we use for primitive types
  (to avoid overlap with user defined types)
* Incomplete tests
kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request May 21, 2017
TY: Improve indexing of primitive types
impl blocks for primitive types are not correctly indexed.
This causes problems with autocompletion and method resolving
around primitive types.

In Rust primitive types are only allowed a single
impl block that must be annotated with the "lang" attribute.
The primitive types bool does not have an inherent impl.

See rust-lang/rust#23104
and the error code E0390 for more details.

Our implementation does not enforce this limitation
and also accept user defined impl blocks.
kumbayo added a commit to kumbayo/intellij-rust that referenced this pull request May 21, 2017
TY: Improve indexing of primitive types
impl blocks for primitive types are not correctly indexed.
This causes problems with autocompletion and method resolving
around primitive types.

In Rust primitive types are only allowed a single
impl block that must be annotated with the "lang" attribute.
The primitive types bool does not have an inherent impl.

See rust-lang/rust#23104
and the error code E0390 for more details.

Our implementation does not enforce this limitation
and also accept user defined impl blocks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.