impl FromIterator<&char> for String

[withoutboats]

No description provided.

[withoutboats]

Was sad to observe that this impl is not backwards compatible:

impl<T> FromIterator<T> for String where String: Extend<T> {
    fn from_iter<I: Iterator<Item = T>>(iter: I) -> String {
        let mut string = String::new();
        string.extend(iter);
        string
    }
}

Nor this even more useful impl:

// something something monoid
impl<T, C> FromIterator<T> for C where C: Default + Extend<T> {
    fn from_iter<I: Iterator<Item = T>>(iter: I) -> String {
        let mut collection = C::default();
        collection.extend(iter);
        collection
    }
}

[alexcrichton]

@rfcbot fcp merge

Looks good to me!

[rfcbot]

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

• @BurntSushi
• @Kimundi
• @alexcrichton
• @aturon
• @brson
• @sfackler

No concerns currently listed.

Once these reviewers reach consensus, 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.

[sfackler]

Do we do this kind of implicit dereference elsewhere? Can I for example collect an Iterator<Item = &u8> to a Vec<u8>?

[withoutboats]

@sfackler we implement Extend<&char> for String (its used in the impl here!), seems to me that there should be a parity between those two (hence the reference to the non-backwards compatible impls). This is the only case where String: Extend<T> but not FromIterator<T>.

But I also would definitely support <T: Copy> FromIterator<&T> for Vec<T>.

[ollie27]

If impl FromIterator<&char> for Stringis added then impl FromIterator<&char> for Cow<str> should also be added to keep consistency between the FromIterator impls for String and Cox<str>.

I also think there should be parity between Extend and FromIterator but it may be too late to add the FromIterator<&T> impls to the collections as it might cause inference errors in existing code. For example [0u8, 1, 2].iter().collect::<Vec<_>>() would suddenly be ambiguous. That might also be why those impls don't already exist.

[withoutboats]

I can add that impl to this PR.

I also think there should be parity between Extend and FromIterator but it may be too late to add the FromIterator<&T> impls to the collections as it might cause inference errors in existing code. For example [0u8, 1, 2].iter().collect::<Vec<_>>() would suddenly be ambiguous. That might also be why those impls don't already exist.

(Note there's no extend impl so there's no parity in that case).

That impl would need a crater run, but I have trouble imagining code that would not have enough contextual information to distinguish a Vec<u8> from a Vec<&u8>. Anyway its a separate concept from this PR.

[alexcrichton]

@sfackler yes

[sfackler]

TIL

[rfcbot]

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

[alexcrichton]

@bors: r+

[bors]

📌 Commit 097398e has been approved by alexcrichton

[withoutboats]

I'll try to remember to put up another PR for the Cow impl

[cuviper]

@alexcrichton

@sfackler yes

AFAICS, that impl is only used for its spec_extend. There's still no FromIterator<&'a T> for Vec<T>, nor for any other collection. String is the only one doing this, and the rest only copy for Extend.

[alexcrichton]

Oh sorry I was thinking of this impl, namely:

impl<'a, T: 'a + Copy> Extend<&'a T> for Vec<T> {

I forgot that it was only Extend, not FromIterator.

[alexcrichton]

Unfortunately this was just released though so we don't have much time to rethink the merge...

[cuviper]

Yeah, I only got here because of the release notes.