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

Tracking issue for slice_concat_ext stabilization #27747

Closed
aturon opened this issue Aug 12, 2015 · 36 comments
Closed

Tracking issue for slice_concat_ext stabilization #27747

aturon opened this issue Aug 12, 2015 · 36 comments
Labels
A-slice Area: `[T]` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@aturon
Copy link
Member

aturon commented Aug 12, 2015

The SliceConcatExt trait offers methods concat and join on slices. For somewhat technical reasons, it wasn't possible to make these inherent methods.

The methods themselves are stable, but the trait isn't. However, since the trait is in the prelude, the methods are usable in stable today.

Ideally, the methods would also be available on iterators, but there are performance ramifications for doing so. Impl specialization may help.

@aturon aturon added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@teburd
Copy link

teburd commented Nov 16, 2015

Having just run into needing this today it would be great to see the documentation for this referenced in the Vec docs with some examples

@aturon
Copy link
Member Author

aturon commented Nov 18, 2015

@BFrog agreed. cc @steveklabnik

@steveklabnik
Copy link
Member

/cc #29380

@oconnor663
Copy link
Contributor

@aturon just reading along, and I'm curious if you have time to explain: What are the technical reasons you mentioned at the top? Is this related to why the SliceExt trait exists?

@aturon
Copy link
Member Author

aturon commented Feb 2, 2016

@oconnor663

So, the main problem with moving to inherent methods at this point is that the traits provide something akin to method overloading -- both apply to slice types varying only by some bounds (Clone and Borrow). I think originally they were less general, but there was some other holdup to making them inherent, which I no longer recall.

re: SliceExt: the reason for that trait is somewhat obscure. Basically, the std crate ends up re-exporting a number of things from the core crate, but in the case of types like slices, it also wants to add methods of its own. We set things up so that the set of inherent methods for any type in std has to be defined in exactly one of the crates making up std. So core's slices don't have any inherent items, because they're defined in collections. You can find the impl here: http://static.rust-lang.org/doc/master/src/collections/slice.rs.html#160

If you're just using std, you never need to see/work with SliceExt. But if you're using core, the SliceExt trait is how we provide any methods on slices. It's in the core prelude, so you generally don't notice the difference.

... Hope that helps!

@oconnor663
Copy link
Contributor

Good to know :)

@Dr-Emann
Copy link

It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

e.g.

let v = vec!["1", "2", "3"];
v.join(", ")

works, but

let v: Vec<&[u8]> = vec![b"1", b"2", b"3"];
v.join(b", ") // Error: expected u8, found array of 2 elements
v.join(&b',') // works, but is missing the space char

doesn't work

@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 22, 2017
@leoyvens
Copy link
Contributor

leoyvens commented Sep 12, 2017

I tried making the methods inherent with separate inherent impls. But we can't have multiple inherent impls for a primitive. I got the error:

"only a single inherent implementation marked with #[lang = "slice"] is allowed for the [T] primitive"

This is known of course, the issue is #32631.

@SimonSapin
Copy link
Contributor

@leodasvacas I think you mean multiple impl Type {…} blocks, but that’s not the main issue. We can add new methods in the existing impl blocks. However in this case, for example <&[String]>::concat and <&[Vec<T>]>::concat end up being the same method, so a helper trait would still be needed to generalize over both cases.

The libs team discussed this and consensus was to add #[doc(hidden)] to the deprecated (but stable) connect method, but make no further changes or stabilize anything at the moment. Inherent methods with a new helper trait doesn’t seem much better than the current trait methods, and still wouldn’t help with joining/concatenating iterators without first collecting into an intermediate Vec.

Design proposals to generalize this functionality to iterators are welcome.

@sanmai-NL
Copy link

The methods themselves are stable, but the trait isn't. However, since the trait is in the prelude, the methods are usable in stable today.

Does this mean concat() isn’t usable in a no_std module with explicit std imports and without the slice_concat_ext unstable feature enabled?

@SimonSapin
Copy link
Contributor

@sanmai-NL That is correct. (Modulo #15702, which is a bug and not considered part of the stability promise.)

@BurntSushi
Copy link
Member

Does there exist a path to stabilizing SliceConcatExt? Are there any specific outstanding issues associated with it? The specific use case I have for it is that I'd like for it to be able to work on other string types. e.g.,

impl<S: Borrow<BStr>> SliceConcatExt for [S] {
    type Output = BString;
    // ...
}

The work-around is to define a separate trait that is probably identical to SliceConcatExt, but in my crate. But that means users won't benefit from having it in the prelude.

@czipperz
Copy link
Contributor

Ping @aturon

@czipperz
Copy link
Contributor

Is there a reason this trait isn't implemented for OsString and CString?

@nico-abram
Copy link
Contributor

nico-abram commented Jul 5, 2019

Is there any stable way to use join in a no_std enviroment using alloc?

@SimonSapin
Copy link
Contributor

@nico-abram I believe there is none today, since liballoc does not have a prelude that gets implicitly imported.

I a previous comment, I wrote:

We can add new methods in the existing impl blocks. However in this case, for example <&[String]>::concat and <&[Vec<T>]>::concat end up being the same method, so a helper trait would still be needed to generalize over both cases.

[…]

Inherent methods with a new helper trait doesn’t seem much better than the current trait methods, and still wouldn’t help with joining/concatenating iterators without first collecting into an intermediate Vec.

This is however an improvement in that the new helper trait does not need to be in scope for the inherent methods to be used. In particular, it does not need to be in the prelude.

#62403 implements this.

@SimonSapin
Copy link
Contributor

impl<S: Borrow<BStr>> SliceConcatExt for [S] {
    type Output = BString;
    // ...
}

@BurntSushi, impl coherence rules do not allow this impl outside of the crate that defines the trait:

error[E0210]: type parameter `S` must be used as the type parameter for some local type (e.g., `MyStruct<S>`)

You would need separate impls for [BStr], for [BString], etc.

But that means users won't benefit from having it in the prelude.

FWIW, not having this benefit is rather typical of functionality defined outside of the standard library.

@SimonSapin
Copy link
Contributor

It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

@Dr-Emann I realize it’s two years later, but: #62528

Centril added a commit to Centril/rust that referenced this issue Jul 22, 2019
Add joining slices of slices with a slice separator, not just a single item

rust-lang#27747 (comment)
> It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

This turns out to be fixable, with some possible inference regressions.

# TL;DR

Related trait(s) are unstable and tracked at rust-lang#27747, but the `[T]::join` method that is being extended here is already stable.

Example use of the new insta-stable functionality:

```rust
let nested: Vec<Vec<Foo>> = /* … */;
let separator: &[Foo] = /* … */;  // Previously: could only be a single &Foo
nested.join(separator)
```

Complete API affected by this PR, after changes:

```rust
impl<T> [T] {
    pub fn concat<Item: ?Sized>(&self) -> <Self as Concat<Item>>::Output
        where Self: Concat<Item>
    {
        Concat::concat(self)
    }
    pub fn join<Separator>(&self, sep: Separator) -> <Self as Join<Separator>>::Output
        where Self: Join<Separator>
    {
        Join::join(self, sep)
    }
}

// The `Item` parameter is only useful for the the slice-of-slices impl.
pub trait Concat<Item: ?Sized> {
    type Output;
    fn concat(slice: &Self) -> Self::Output;
}

pub trait Join<Separator> {
    type Output;
    fn join(slice: &Self, sep: Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> Concat<T> for [V] {
    type Output = Vec<T>;
}

impl<T: Clone, V: Borrow<[T]>> Join<&'_ T> for [V] {
    type Output = Vec<T>;
}

// New functionality here!
impl<T: Clone, V: Borrow<[T]>> Join<&'_ [T]> for [V] {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> Concat<str> for [S] {
    type Output = String;
}

impl<S: Borrow<str>> Join<&'_ str> for [S] {
    type Output = String;
}
```

# Details

After rust-lang#62403 but before this PR, the API is:

```rust
impl<T> [T] {
    pub fn concat<Separator: ?Sized>(&self) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::concat(self)
    }

    pub fn join<Separator: ?Sized>(&self, sep: &Separator) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::join(self, sep)
    }
}

pub trait SliceConcat<Separator: ?Sized>: Sized {
    type Output;
    fn concat(slice: &[Self]) -> Self::Output;
    fn join(slice: &[Self], sep: &Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> SliceConcat<T> for V {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> SliceConcat<str> for S {
    type Output = String;
}
```

By adding a trait impl we should be able to accept a slice of `T` as the separator, as an alternative to a single `T` value.

In a `some_slice.join(some_separator)` call, trait resolution will pick an impl or the other based on the type of `some_separator`. In `some_slice.concat()` however there is no separator, so this call would become ambiguous. Some regression in type inference or trait resolution may be acceptable on principle, but requiring a turbofish for every single call to `concat` isn’t great.

The solution to that is splitting the `SliceConcat` trait into two `Concat` and `Join` traits, one for each eponymous method. Only `Join` would gain a new impl, so that `some_slice.concat()` would not become ambiguous.

Now, at the trait level the `Concat` trait does not need a `Separator` parameter anymore. However, simply removing it causes one of the impls not to be accepted anymore:

```rust
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
  --> src/liballoc/slice.rs:608:6
    |
608 | impl<T: Clone, V: Borrow<[T]>> Concat for [V] {
    |      ^ unconstrained type parameter
```

This makes sense: if `[V]::concat` is a method that is itself not generic, then its return type (which is the `Concat::Output` associated type) needs to be determined based on solely `V`. And although there is no such type in the standard library, there is nothing stopping another crate from defining a `V` type that implements both `Borrow<[Foo]>` and `Borrow<[Bar]>`. It might not be a good idea, but it’s possible. Both would apply here, and there would be no way to determine `T`.

This could be a warning sign that this API is too generic. Perhaps we’d be better off having one less type variable, and only implement `Concat for [&'_ [T]]` and `Concat for [Vec<T>]` etc. However this aspect of `[V]::concat` is already stable, so we’re stuck with it.

The solution is to keep a dummy type parameter on the `Concat` trait. That way, if a type has multiple `Borrow<[_]>` impls, it’ll end up with multiple corresponding `Concat<_>` impls.

In `impl<S: Borrow<str>> Concat<str> for [S]`, the second occurrence of `str` is not meaningful. It could be any type. As long as there is only once such type with an applicable impl, trait resolution will be appeased without demanding turbofishes.

# Joining strings with `char`

For symmetry I also tried adding this impl (because why not):

```rust
impl<S: Borrow<str>> Join<char> for [S] {
    type Output = String;
}
```

This immediately caused an inference regression in a dependency of rustc:

```rust
error[E0277]: the trait bound `std::string::String: std::borrow::Borrow<[std::string::String]>` is not satisfied
   --> /home/simon/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/getopts-0.2.19/src/lib.rs:595:37
    |
595 |             row.push_str(&desc_rows.join(&desc_sep));
    |                                     ^^^^ the trait `std::borrow::Borrow<[std::string::String]>` is not implemented for `std::string::String`
    |
    = help: the following implementations were found:
              <std::string::String as std::borrow::Borrow<str>>
    = note: required because of the requirements on the impl of `std::slice::Join<&std::string::String>` for `[std::string::String]`
```

In the context of this code, two facts are known:

* `desc_rows` is a `Vec<String>`
* `desc_sep` is a `String`

Previously the first fact alone reduces the resolution of `join` to only one solution, where its argument it expected to be `&str`. Then, `&String` is coerced to `&str`.

With the new `Join` impl, the first fact leavs two applicable impls where the separator can be either `&str` or `char`. But `&String` is neither of these things. It appears that possible coercions are not accounted for, in the search for a solution in trait resolution.

I have not included this new impl in this PR. It’s still possible to add later, but the `getopts` breakage does not need to block the rest of the PR. And the functionality easy for end-user to duplicate: `slice_of_strings.join(&*char_separator.encode_utf8(&mut [0_u8, 4]))`

The `&*` part of that last code snippet is another case of the same issue: `encode_utf8` returns `&mut str` which can be coerced to `&str`, but isn’t when trait resolution is ambiguous.
Centril added a commit to Centril/rust that referenced this issue Jul 23, 2019
Add joining slices of slices with a slice separator, not just a single item

rust-lang#27747 (comment)
> It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

This turns out to be fixable, with some possible inference regressions.

# TL;DR

Related trait(s) are unstable and tracked at rust-lang#27747, but the `[T]::join` method that is being extended here is already stable.

Example use of the new insta-stable functionality:

```rust
let nested: Vec<Vec<Foo>> = /* … */;
let separator: &[Foo] = /* … */;  // Previously: could only be a single &Foo
nested.join(separator)
```

Complete API affected by this PR, after changes:

```rust
impl<T> [T] {
    pub fn concat<Item: ?Sized>(&self) -> <Self as Concat<Item>>::Output
        where Self: Concat<Item>
    {
        Concat::concat(self)
    }
    pub fn join<Separator>(&self, sep: Separator) -> <Self as Join<Separator>>::Output
        where Self: Join<Separator>
    {
        Join::join(self, sep)
    }
}

// The `Item` parameter is only useful for the the slice-of-slices impl.
pub trait Concat<Item: ?Sized> {
    type Output;
    fn concat(slice: &Self) -> Self::Output;
}

pub trait Join<Separator> {
    type Output;
    fn join(slice: &Self, sep: Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> Concat<T> for [V] {
    type Output = Vec<T>;
}

impl<T: Clone, V: Borrow<[T]>> Join<&'_ T> for [V] {
    type Output = Vec<T>;
}

// New functionality here!
impl<T: Clone, V: Borrow<[T]>> Join<&'_ [T]> for [V] {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> Concat<str> for [S] {
    type Output = String;
}

impl<S: Borrow<str>> Join<&'_ str> for [S] {
    type Output = String;
}
```

# Details

After rust-lang#62403 but before this PR, the API is:

```rust
impl<T> [T] {
    pub fn concat<Separator: ?Sized>(&self) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::concat(self)
    }

    pub fn join<Separator: ?Sized>(&self, sep: &Separator) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::join(self, sep)
    }
}

pub trait SliceConcat<Separator: ?Sized>: Sized {
    type Output;
    fn concat(slice: &[Self]) -> Self::Output;
    fn join(slice: &[Self], sep: &Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> SliceConcat<T> for V {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> SliceConcat<str> for S {
    type Output = String;
}
```

By adding a trait impl we should be able to accept a slice of `T` as the separator, as an alternative to a single `T` value.

In a `some_slice.join(some_separator)` call, trait resolution will pick an impl or the other based on the type of `some_separator`. In `some_slice.concat()` however there is no separator, so this call would become ambiguous. Some regression in type inference or trait resolution may be acceptable on principle, but requiring a turbofish for every single call to `concat` isn’t great.

The solution to that is splitting the `SliceConcat` trait into two `Concat` and `Join` traits, one for each eponymous method. Only `Join` would gain a new impl, so that `some_slice.concat()` would not become ambiguous.

Now, at the trait level the `Concat` trait does not need a `Separator` parameter anymore. However, simply removing it causes one of the impls not to be accepted anymore:

```rust
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
  --> src/liballoc/slice.rs:608:6
    |
608 | impl<T: Clone, V: Borrow<[T]>> Concat for [V] {
    |      ^ unconstrained type parameter
```

This makes sense: if `[V]::concat` is a method that is itself not generic, then its return type (which is the `Concat::Output` associated type) needs to be determined based on solely `V`. And although there is no such type in the standard library, there is nothing stopping another crate from defining a `V` type that implements both `Borrow<[Foo]>` and `Borrow<[Bar]>`. It might not be a good idea, but it’s possible. Both would apply here, and there would be no way to determine `T`.

This could be a warning sign that this API is too generic. Perhaps we’d be better off having one less type variable, and only implement `Concat for [&'_ [T]]` and `Concat for [Vec<T>]` etc. However this aspect of `[V]::concat` is already stable, so we’re stuck with it.

The solution is to keep a dummy type parameter on the `Concat` trait. That way, if a type has multiple `Borrow<[_]>` impls, it’ll end up with multiple corresponding `Concat<_>` impls.

In `impl<S: Borrow<str>> Concat<str> for [S]`, the second occurrence of `str` is not meaningful. It could be any type. As long as there is only once such type with an applicable impl, trait resolution will be appeased without demanding turbofishes.

# Joining strings with `char`

For symmetry I also tried adding this impl (because why not):

```rust
impl<S: Borrow<str>> Join<char> for [S] {
    type Output = String;
}
```

This immediately caused an inference regression in a dependency of rustc:

```rust
error[E0277]: the trait bound `std::string::String: std::borrow::Borrow<[std::string::String]>` is not satisfied
   --> /home/simon/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/getopts-0.2.19/src/lib.rs:595:37
    |
595 |             row.push_str(&desc_rows.join(&desc_sep));
    |                                     ^^^^ the trait `std::borrow::Borrow<[std::string::String]>` is not implemented for `std::string::String`
    |
    = help: the following implementations were found:
              <std::string::String as std::borrow::Borrow<str>>
    = note: required because of the requirements on the impl of `std::slice::Join<&std::string::String>` for `[std::string::String]`
```

In the context of this code, two facts are known:

* `desc_rows` is a `Vec<String>`
* `desc_sep` is a `String`

Previously the first fact alone reduces the resolution of `join` to only one solution, where its argument it expected to be `&str`. Then, `&String` is coerced to `&str`.

With the new `Join` impl, the first fact leavs two applicable impls where the separator can be either `&str` or `char`. But `&String` is neither of these things. It appears that possible coercions are not accounted for, in the search for a solution in trait resolution.

I have not included this new impl in this PR. It’s still possible to add later, but the `getopts` breakage does not need to block the rest of the PR. And the functionality easy for end-user to duplicate: `slice_of_strings.join(&*char_separator.encode_utf8(&mut [0_u8, 4]))`

The `&*` part of that last code snippet is another case of the same issue: `encode_utf8` returns `&mut str` which can be coerced to `&str`, but isn’t when trait resolution is ambiguous.
Centril added a commit to Centril/rust that referenced this issue Jul 24, 2019
Add joining slices of slices with a slice separator, not just a single item

rust-lang#27747 (comment)
> It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

This turns out to be fixable, with some possible inference regressions.

# TL;DR

Related trait(s) are unstable and tracked at rust-lang#27747, but the `[T]::join` method that is being extended here is already stable.

Example use of the new insta-stable functionality:

```rust
let nested: Vec<Vec<Foo>> = /* … */;
let separator: &[Foo] = /* … */;  // Previously: could only be a single &Foo
nested.join(separator)
```

Complete API affected by this PR, after changes:

```rust
impl<T> [T] {
    pub fn concat<Item: ?Sized>(&self) -> <Self as Concat<Item>>::Output
        where Self: Concat<Item>
    {
        Concat::concat(self)
    }
    pub fn join<Separator>(&self, sep: Separator) -> <Self as Join<Separator>>::Output
        where Self: Join<Separator>
    {
        Join::join(self, sep)
    }
}

// The `Item` parameter is only useful for the the slice-of-slices impl.
pub trait Concat<Item: ?Sized> {
    type Output;
    fn concat(slice: &Self) -> Self::Output;
}

pub trait Join<Separator> {
    type Output;
    fn join(slice: &Self, sep: Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> Concat<T> for [V] {
    type Output = Vec<T>;
}

impl<T: Clone, V: Borrow<[T]>> Join<&'_ T> for [V] {
    type Output = Vec<T>;
}

// New functionality here!
impl<T: Clone, V: Borrow<[T]>> Join<&'_ [T]> for [V] {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> Concat<str> for [S] {
    type Output = String;
}

impl<S: Borrow<str>> Join<&'_ str> for [S] {
    type Output = String;
}
```

# Details

After rust-lang#62403 but before this PR, the API is:

```rust
impl<T> [T] {
    pub fn concat<Separator: ?Sized>(&self) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::concat(self)
    }

    pub fn join<Separator: ?Sized>(&self, sep: &Separator) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::join(self, sep)
    }
}

pub trait SliceConcat<Separator: ?Sized>: Sized {
    type Output;
    fn concat(slice: &[Self]) -> Self::Output;
    fn join(slice: &[Self], sep: &Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> SliceConcat<T> for V {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> SliceConcat<str> for S {
    type Output = String;
}
```

By adding a trait impl we should be able to accept a slice of `T` as the separator, as an alternative to a single `T` value.

In a `some_slice.join(some_separator)` call, trait resolution will pick an impl or the other based on the type of `some_separator`. In `some_slice.concat()` however there is no separator, so this call would become ambiguous. Some regression in type inference or trait resolution may be acceptable on principle, but requiring a turbofish for every single call to `concat` isn’t great.

The solution to that is splitting the `SliceConcat` trait into two `Concat` and `Join` traits, one for each eponymous method. Only `Join` would gain a new impl, so that `some_slice.concat()` would not become ambiguous.

Now, at the trait level the `Concat` trait does not need a `Separator` parameter anymore. However, simply removing it causes one of the impls not to be accepted anymore:

```rust
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
  --> src/liballoc/slice.rs:608:6
    |
608 | impl<T: Clone, V: Borrow<[T]>> Concat for [V] {
    |      ^ unconstrained type parameter
```

This makes sense: if `[V]::concat` is a method that is itself not generic, then its return type (which is the `Concat::Output` associated type) needs to be determined based on solely `V`. And although there is no such type in the standard library, there is nothing stopping another crate from defining a `V` type that implements both `Borrow<[Foo]>` and `Borrow<[Bar]>`. It might not be a good idea, but it’s possible. Both would apply here, and there would be no way to determine `T`.

This could be a warning sign that this API is too generic. Perhaps we’d be better off having one less type variable, and only implement `Concat for [&'_ [T]]` and `Concat for [Vec<T>]` etc. However this aspect of `[V]::concat` is already stable, so we’re stuck with it.

The solution is to keep a dummy type parameter on the `Concat` trait. That way, if a type has multiple `Borrow<[_]>` impls, it’ll end up with multiple corresponding `Concat<_>` impls.

In `impl<S: Borrow<str>> Concat<str> for [S]`, the second occurrence of `str` is not meaningful. It could be any type. As long as there is only once such type with an applicable impl, trait resolution will be appeased without demanding turbofishes.

# Joining strings with `char`

For symmetry I also tried adding this impl (because why not):

```rust
impl<S: Borrow<str>> Join<char> for [S] {
    type Output = String;
}
```

This immediately caused an inference regression in a dependency of rustc:

```rust
error[E0277]: the trait bound `std::string::String: std::borrow::Borrow<[std::string::String]>` is not satisfied
   --> /home/simon/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/getopts-0.2.19/src/lib.rs:595:37
    |
595 |             row.push_str(&desc_rows.join(&desc_sep));
    |                                     ^^^^ the trait `std::borrow::Borrow<[std::string::String]>` is not implemented for `std::string::String`
    |
    = help: the following implementations were found:
              <std::string::String as std::borrow::Borrow<str>>
    = note: required because of the requirements on the impl of `std::slice::Join<&std::string::String>` for `[std::string::String]`
```

In the context of this code, two facts are known:

* `desc_rows` is a `Vec<String>`
* `desc_sep` is a `String`

Previously the first fact alone reduces the resolution of `join` to only one solution, where its argument it expected to be `&str`. Then, `&String` is coerced to `&str`.

With the new `Join` impl, the first fact leavs two applicable impls where the separator can be either `&str` or `char`. But `&String` is neither of these things. It appears that possible coercions are not accounted for, in the search for a solution in trait resolution.

I have not included this new impl in this PR. It’s still possible to add later, but the `getopts` breakage does not need to block the rest of the PR. And the functionality easy for end-user to duplicate: `slice_of_strings.join(&*char_separator.encode_utf8(&mut [0_u8, 4]))`

The `&*` part of that last code snippet is another case of the same issue: `encode_utf8` returns `&mut str` which can be coerced to `&str`, but isn’t when trait resolution is ambiguous.
Centril added a commit to Centril/rust that referenced this issue Jul 24, 2019
Add joining slices of slices with a slice separator, not just a single item

rust-lang#27747 (comment)
> It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

This turns out to be fixable, with some possible inference regressions.

# TL;DR

Related trait(s) are unstable and tracked at rust-lang#27747, but the `[T]::join` method that is being extended here is already stable.

Example use of the new insta-stable functionality:

```rust
let nested: Vec<Vec<Foo>> = /* … */;
let separator: &[Foo] = /* … */;  // Previously: could only be a single &Foo
nested.join(separator)
```

Complete API affected by this PR, after changes:

```rust
impl<T> [T] {
    pub fn concat<Item: ?Sized>(&self) -> <Self as Concat<Item>>::Output
        where Self: Concat<Item>
    {
        Concat::concat(self)
    }
    pub fn join<Separator>(&self, sep: Separator) -> <Self as Join<Separator>>::Output
        where Self: Join<Separator>
    {
        Join::join(self, sep)
    }
}

// The `Item` parameter is only useful for the the slice-of-slices impl.
pub trait Concat<Item: ?Sized> {
    type Output;
    fn concat(slice: &Self) -> Self::Output;
}

pub trait Join<Separator> {
    type Output;
    fn join(slice: &Self, sep: Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> Concat<T> for [V] {
    type Output = Vec<T>;
}

impl<T: Clone, V: Borrow<[T]>> Join<&'_ T> for [V] {
    type Output = Vec<T>;
}

// New functionality here!
impl<T: Clone, V: Borrow<[T]>> Join<&'_ [T]> for [V] {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> Concat<str> for [S] {
    type Output = String;
}

impl<S: Borrow<str>> Join<&'_ str> for [S] {
    type Output = String;
}
```

# Details

After rust-lang#62403 but before this PR, the API is:

```rust
impl<T> [T] {
    pub fn concat<Separator: ?Sized>(&self) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::concat(self)
    }

    pub fn join<Separator: ?Sized>(&self, sep: &Separator) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::join(self, sep)
    }
}

pub trait SliceConcat<Separator: ?Sized>: Sized {
    type Output;
    fn concat(slice: &[Self]) -> Self::Output;
    fn join(slice: &[Self], sep: &Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> SliceConcat<T> for V {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> SliceConcat<str> for S {
    type Output = String;
}
```

By adding a trait impl we should be able to accept a slice of `T` as the separator, as an alternative to a single `T` value.

In a `some_slice.join(some_separator)` call, trait resolution will pick an impl or the other based on the type of `some_separator`. In `some_slice.concat()` however there is no separator, so this call would become ambiguous. Some regression in type inference or trait resolution may be acceptable on principle, but requiring a turbofish for every single call to `concat` isn’t great.

The solution to that is splitting the `SliceConcat` trait into two `Concat` and `Join` traits, one for each eponymous method. Only `Join` would gain a new impl, so that `some_slice.concat()` would not become ambiguous.

Now, at the trait level the `Concat` trait does not need a `Separator` parameter anymore. However, simply removing it causes one of the impls not to be accepted anymore:

```rust
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
  --> src/liballoc/slice.rs:608:6
    |
608 | impl<T: Clone, V: Borrow<[T]>> Concat for [V] {
    |      ^ unconstrained type parameter
```

This makes sense: if `[V]::concat` is a method that is itself not generic, then its return type (which is the `Concat::Output` associated type) needs to be determined based on solely `V`. And although there is no such type in the standard library, there is nothing stopping another crate from defining a `V` type that implements both `Borrow<[Foo]>` and `Borrow<[Bar]>`. It might not be a good idea, but it’s possible. Both would apply here, and there would be no way to determine `T`.

This could be a warning sign that this API is too generic. Perhaps we’d be better off having one less type variable, and only implement `Concat for [&'_ [T]]` and `Concat for [Vec<T>]` etc. However this aspect of `[V]::concat` is already stable, so we’re stuck with it.

The solution is to keep a dummy type parameter on the `Concat` trait. That way, if a type has multiple `Borrow<[_]>` impls, it’ll end up with multiple corresponding `Concat<_>` impls.

In `impl<S: Borrow<str>> Concat<str> for [S]`, the second occurrence of `str` is not meaningful. It could be any type. As long as there is only once such type with an applicable impl, trait resolution will be appeased without demanding turbofishes.

# Joining strings with `char`

For symmetry I also tried adding this impl (because why not):

```rust
impl<S: Borrow<str>> Join<char> for [S] {
    type Output = String;
}
```

This immediately caused an inference regression in a dependency of rustc:

```rust
error[E0277]: the trait bound `std::string::String: std::borrow::Borrow<[std::string::String]>` is not satisfied
   --> /home/simon/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/getopts-0.2.19/src/lib.rs:595:37
    |
595 |             row.push_str(&desc_rows.join(&desc_sep));
    |                                     ^^^^ the trait `std::borrow::Borrow<[std::string::String]>` is not implemented for `std::string::String`
    |
    = help: the following implementations were found:
              <std::string::String as std::borrow::Borrow<str>>
    = note: required because of the requirements on the impl of `std::slice::Join<&std::string::String>` for `[std::string::String]`
```

In the context of this code, two facts are known:

* `desc_rows` is a `Vec<String>`
* `desc_sep` is a `String`

Previously the first fact alone reduces the resolution of `join` to only one solution, where its argument it expected to be `&str`. Then, `&String` is coerced to `&str`.

With the new `Join` impl, the first fact leavs two applicable impls where the separator can be either `&str` or `char`. But `&String` is neither of these things. It appears that possible coercions are not accounted for, in the search for a solution in trait resolution.

I have not included this new impl in this PR. It’s still possible to add later, but the `getopts` breakage does not need to block the rest of the PR. And the functionality easy for end-user to duplicate: `slice_of_strings.join(&*char_separator.encode_utf8(&mut [0_u8, 4]))`

The `&*` part of that last code snippet is another case of the same issue: `encode_utf8` returns `&mut str` which can be coerced to `&str`, but isn’t when trait resolution is ambiguous.
Centril added a commit to Centril/rust that referenced this issue Jul 24, 2019
Add joining slices of slices with a slice separator, not just a single item

rust-lang#27747 (comment)
> It's kinda annoying to be able to join strings with a str (which can have multiple chars), but joining a slice of slices, you can only join with a single element.

This turns out to be fixable, with some possible inference regressions.

# TL;DR

Related trait(s) are unstable and tracked at rust-lang#27747, but the `[T]::join` method that is being extended here is already stable.

Example use of the new insta-stable functionality:

```rust
let nested: Vec<Vec<Foo>> = /* … */;
let separator: &[Foo] = /* … */;  // Previously: could only be a single &Foo
nested.join(separator)
```

Complete API affected by this PR, after changes:

```rust
impl<T> [T] {
    pub fn concat<Item: ?Sized>(&self) -> <Self as Concat<Item>>::Output
        where Self: Concat<Item>
    {
        Concat::concat(self)
    }
    pub fn join<Separator>(&self, sep: Separator) -> <Self as Join<Separator>>::Output
        where Self: Join<Separator>
    {
        Join::join(self, sep)
    }
}

// The `Item` parameter is only useful for the the slice-of-slices impl.
pub trait Concat<Item: ?Sized> {
    type Output;
    fn concat(slice: &Self) -> Self::Output;
}

pub trait Join<Separator> {
    type Output;
    fn join(slice: &Self, sep: Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> Concat<T> for [V] {
    type Output = Vec<T>;
}

impl<T: Clone, V: Borrow<[T]>> Join<&'_ T> for [V] {
    type Output = Vec<T>;
}

// New functionality here!
impl<T: Clone, V: Borrow<[T]>> Join<&'_ [T]> for [V] {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> Concat<str> for [S] {
    type Output = String;
}

impl<S: Borrow<str>> Join<&'_ str> for [S] {
    type Output = String;
}
```

# Details

After rust-lang#62403 but before this PR, the API is:

```rust
impl<T> [T] {
    pub fn concat<Separator: ?Sized>(&self) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::concat(self)
    }

    pub fn join<Separator: ?Sized>(&self, sep: &Separator) -> T::Output
        where T: SliceConcat<Separator>
    {
        SliceConcat::join(self, sep)
    }
}

pub trait SliceConcat<Separator: ?Sized>: Sized {
    type Output;
    fn concat(slice: &[Self]) -> Self::Output;
    fn join(slice: &[Self], sep: &Separator) -> Self::Output;
}

impl<T: Clone, V: Borrow<[T]>> SliceConcat<T> for V {
    type Output = Vec<T>;
}

impl<S: Borrow<str>> SliceConcat<str> for S {
    type Output = String;
}
```

By adding a trait impl we should be able to accept a slice of `T` as the separator, as an alternative to a single `T` value.

In a `some_slice.join(some_separator)` call, trait resolution will pick an impl or the other based on the type of `some_separator`. In `some_slice.concat()` however there is no separator, so this call would become ambiguous. Some regression in type inference or trait resolution may be acceptable on principle, but requiring a turbofish for every single call to `concat` isn’t great.

The solution to that is splitting the `SliceConcat` trait into two `Concat` and `Join` traits, one for each eponymous method. Only `Join` would gain a new impl, so that `some_slice.concat()` would not become ambiguous.

Now, at the trait level the `Concat` trait does not need a `Separator` parameter anymore. However, simply removing it causes one of the impls not to be accepted anymore:

```rust
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
  --> src/liballoc/slice.rs:608:6
    |
608 | impl<T: Clone, V: Borrow<[T]>> Concat for [V] {
    |      ^ unconstrained type parameter
```

This makes sense: if `[V]::concat` is a method that is itself not generic, then its return type (which is the `Concat::Output` associated type) needs to be determined based on solely `V`. And although there is no such type in the standard library, there is nothing stopping another crate from defining a `V` type that implements both `Borrow<[Foo]>` and `Borrow<[Bar]>`. It might not be a good idea, but it’s possible. Both would apply here, and there would be no way to determine `T`.

This could be a warning sign that this API is too generic. Perhaps we’d be better off having one less type variable, and only implement `Concat for [&'_ [T]]` and `Concat for [Vec<T>]` etc. However this aspect of `[V]::concat` is already stable, so we’re stuck with it.

The solution is to keep a dummy type parameter on the `Concat` trait. That way, if a type has multiple `Borrow<[_]>` impls, it’ll end up with multiple corresponding `Concat<_>` impls.

In `impl<S: Borrow<str>> Concat<str> for [S]`, the second occurrence of `str` is not meaningful. It could be any type. As long as there is only once such type with an applicable impl, trait resolution will be appeased without demanding turbofishes.

# Joining strings with `char`

For symmetry I also tried adding this impl (because why not):

```rust
impl<S: Borrow<str>> Join<char> for [S] {
    type Output = String;
}
```

This immediately caused an inference regression in a dependency of rustc:

```rust
error[E0277]: the trait bound `std::string::String: std::borrow::Borrow<[std::string::String]>` is not satisfied
   --> /home/simon/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/getopts-0.2.19/src/lib.rs:595:37
    |
595 |             row.push_str(&desc_rows.join(&desc_sep));
    |                                     ^^^^ the trait `std::borrow::Borrow<[std::string::String]>` is not implemented for `std::string::String`
    |
    = help: the following implementations were found:
              <std::string::String as std::borrow::Borrow<str>>
    = note: required because of the requirements on the impl of `std::slice::Join<&std::string::String>` for `[std::string::String]`
```

In the context of this code, two facts are known:

* `desc_rows` is a `Vec<String>`
* `desc_sep` is a `String`

Previously the first fact alone reduces the resolution of `join` to only one solution, where its argument it expected to be `&str`. Then, `&String` is coerced to `&str`.

With the new `Join` impl, the first fact leavs two applicable impls where the separator can be either `&str` or `char`. But `&String` is neither of these things. It appears that possible coercions are not accounted for, in the search for a solution in trait resolution.

I have not included this new impl in this PR. It’s still possible to add later, but the `getopts` breakage does not need to block the rest of the PR. And the functionality easy for end-user to duplicate: `slice_of_strings.join(&*char_separator.encode_utf8(&mut [0_u8, 4]))`

The `&*` part of that last code snippet is another case of the same issue: `encode_utf8` returns `&mut str` which can be coerced to `&str`, but isn’t when trait resolution is ambiguous.
@ariasuni
Copy link
Contributor

Well I’m getting that

error[E0119]: conflicting implementations of trait `slice::Join<&str>` for type `[char]`:
  --> src/liballoc/str.rs:94:1
   |
85 | impl<S: Borrow<str>> Join<&str> for [S] {
   | --------------------------------------- first implementation here
...
94 | impl Join<&str> for [char] {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `[char]`
   |
   = note: upstream crates may add a new impl of trait `core::borrow::Borrow<str>` for type `char` in future versions

So it maybe not possible? I guess I should I take this somewhere else if it is.

@SimonSapin
Copy link
Contributor

Interesting, I would have thought those wouldn’t conflict since char does not implement Borrow<str>. It suppose this is because Borrow is defined in libcore and so is a "foreign" trait for liballoc which is therefore not allowed to assume that impl Borrow<str> for char won’t be added in the future.

So yeah, it looks like this impl is not possible without changing trait coherence rules.

@camsteffen
Copy link
Contributor

Just a thought. Would it be possible to work in something like:

format!("Here we are: {}", big_array.lazy_join(", "));

where lazy_join returns a impl Display?

@SimonSapin
Copy link
Contributor

Yeah that sounds possible. It’s a different feature though. And one you could implement on crates.io with an extension trait.

@coolreader18
Copy link
Contributor

@camsteffen you might be looking for Itertools::format

@KodrAus KodrAus added A-slice Area: `[T]` Libs-Tracked Libs issues that are tracked on the team's project board. labels Jul 29, 2020
@Amanieu
Copy link
Member

Amanieu commented Sep 23, 2021

We discussed this in the libs team and the consensus is that we will likely never stabilize the Concat and Join traits: these will remain implementation details of the join and concat methods.

@rfcbot close

@rfcbot
Copy link

rfcbot commented Sep 23, 2021

Team member @Amanieu has proposed to close this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. labels Sep 23, 2021
@BurntSushi
Copy link
Member

Note that AFAIK, this is still an issue: #27747 (comment)

@yaahc
Copy link
Member

yaahc commented Sep 28, 2021

Note that AFAIK, this is still an issue: #27747 (comment)

I'm confused by what you mean @BurntSushi. Isn't that impl still an orphan rules violation as pointed out in #27747 (comment)? What's the issue and how does this impact the FCP?

@BurntSushi
Copy link
Member

@yaahc Ah sorry, yeah, I forgot about that. Yeah, regrettably, I agree to close this. It would be nice to improve things here, but it looks like a bigger issue than I thought and isn't going to be solved by this trait alone.

@rfcbot
Copy link

rfcbot commented Oct 18, 2021

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Oct 18, 2021
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Oct 28, 2021
@rfcbot
Copy link

rfcbot commented Oct 28, 2021

The final comment period, with a disposition to close, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Oct 28, 2021
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Nov 11, 2021
@jplatte
Copy link
Contributor

jplatte commented Mar 1, 2022

This issue is still referenced in at least two #[unstable] attributes here:

#[unstable(feature = "slice_concat_ext", issue = "27747")]

It seems like those need to be rewritten, right?

@kornelski
Copy link
Contributor

kornelski commented Nov 24, 2022

Is there a need for this trait to use Borrow rather than AsRef? The documentation for Borrow recommends using AsRef when Eq/Ord equivalence isn't required.

@thomcc
Copy link
Member

thomcc commented Nov 24, 2022

That does sound like a mistake, but changing it now sounds impossible to do without breaking code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-slice Area: `[T]` B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC disposition-close This PR / issue is in PFCP or FCP with a disposition to close it. finished-final-comment-period The final comment period is finished for this PR / Issue. Libs-Tracked Libs issues that are tracked on the team's project board. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests