impl FromIterator<char> for Box<str>

[andersk]

This is analogous to impl FromIterator<A> for Box<[A]> (#55843).

Fixes #65163.

[rust-highfive]

r? @LukasKalbertodt

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

[LukasKalbertodt]

This seems like a reasonable addition to me. Thanks!

Please add four tests for the other four impls. And see my inline comment on the test.

[andersk]

Sure, now all five impls are tested.

[LukasKalbertodt]

Looks good to me!

[LukasKalbertodt]

This PR adds the following stable impls:

• impl<'a> FromIterator<&'a char> for Box<str>
• impl<'a> FromIterator<&'a str> for Box<str>
• impl<'a> FromIterator<Cow<'a, str>> for Box<str>
• impl FromIterator<String> for Box<str>
• impl FromIterator<char> for Box<str>

All these impls already exist for String. Thus, the implementation is always simply: iter.into_iter().collect::<String>().into_boxed_str(). We also already had impl FromIterator<A> for Box<[A]> in std for a long time. There was an attempt at this already in #65168, which was closed due to inactivity.

---

As I cannot start FCP merges, reassigning.

[LukasKalbertodt]

r? @Amanieu

[andersk]

Note: I saw in #40028 that @withoutboats regretted that the five String impls could not be generalized to a single impl<T> FromIterator<T> for String where String: Extend<T> due to backwards compatibility (it might overlap with impl FromIterator<Foo> for String for a custom type Foo). I believe the same concern prevents a similar simplification for Box<str>, but I figured I’d mention it in case my understanding is wrong.

[Centril]

As I cannot start FCP merges, reassigning.

You should send in a PR to https://github.com/rust-lang/team to fix that btw. :)

[Amanieu]

@rfcbot fcp merge

[rfcbot]

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

• @Amanieu
• @KodrAus
• @SimonSapin
• @dtolnay
• @sfackler
• @withoutboats

Concerns:

• is Box<str> a string or a niche string representation (impl FromIterator<char> for Box<str> #70094 (comment))

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.

[dtolnay]

I am not enthusiastic about this addition. I would prefer not to go any more in the direction of making Box<str> a fully fledged string type. I see it as a niche alternative representation for owned strings for use cases in which 2 words of size instead of 3 words matters for memory usage, such as a big intern table. In general I'd like users to reach for String when operating with owned strings, and use into_boxed_str if their use case demands the representation of a boxed str.

I'm fine with the necessary APIs for Box<str> as a niche string representation, including things like From<String> and Default. I'm less fine with growing more string-related surface area around Box<str> than that, like FromIterator, Extend, Add, etc. If someone wants to collect into an owned string and use Box<str> as the representation, I think it's fine to have them write collect::<String>().into_boxed_str() instead of collect::<Box<str>>().

@rfcbot concern is Box<str> a string or a niche string representation

[bors]

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

[dtolnay]

I don't know whether procedurally I should fcp close, but I see a 👍 on #70094 (comment) from Lukas who originally proposed the fcp and it doesn't seem like people are planning to make the opposite case so I'll go ahead and directly close the PR. We can reopen as needed if someone is interested in sticking up for the opposite point of view.

Thanks anyway @andersk!