rust icon indicating copy to clipboard operation
rust copied to clipboard

alloc: implement FromIterator for Box<str>

Open calebsander opened this issue 3 years ago • 9 comments

Box<[T]> implements FromIterator<T> using Vec<T> + into_boxed_slice(). Add analogous FromIterator implementations for Box<str> matching the current implementations for String. Remove the Global allocator requirement for FromIterator<Box<str>> too.

ACP: https://github.com/rust-lang/libs-team/issues/196

calebsander avatar Jul 30 '22 23:07 calebsander

r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

rust-highfive avatar Jul 30 '22 23:07 rust-highfive

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

rustbot avatar Jul 30 '22 23:07 rustbot

@rustbot label +T-libs-api -T-libs

calebsander avatar Jul 30 '22 23:07 calebsander

I believe this does require an ACP (insta-stable new implementation of stable trait on stable type). Please create one if you haven't already. Link it here and then label this PR as S-waiting-on-ACP.

@rustbot label -S-waiting-on-review +S-waiting-on-author

pitaj avatar Jan 29 '23 03:01 pitaj

ACP: https://github.com/rust-lang/libs-team/issues/196

calebsander avatar Mar 25 '23 17:03 calebsander

@rustbot label +S-waiting-on-review -S-waiting-on-author

calebsander avatar Mar 25 '23 17:03 calebsander

@rust-lang/libs-api: @rfcbot fcp merge

+ impl FromIterator<char> for Box<str>
+ impl FromIterator<&char> for Box<str>
+ impl FromIterator<&str> for Box<str>
+ impl FromIterator<String> for Box<str>
+ impl<A: Allocator> FromIterator<Box<str, A>> for Box<str>
+ impl FromIterator<Cow<'_, str>> for Box<str>

- impl FromIterator<Box<str>> for String
+ impl<A: Allocator> FromIterator<Box<str, A>> for String

- impl Extend<Box<str>> for String
+ impl<A: Allocator> Extend<Box<str, A>> for String

dtolnay avatar Jan 26 '24 03:01 dtolnay

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

  • [x] @Amanieu
  • [x] @BurntSushi
  • [x] @dtolnay
  • [x] @joshtriplett
  • [ ] @m-ou-se

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 avatar Jan 26 '24 03:01 rfcbot

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

rfcbot avatar Mar 05 '24 17:03 rfcbot

The final comment period, with a disposition to merge, 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.

This will be merged soon.

rfcbot avatar Mar 15 '24 17:03 rfcbot

@bors r+

dtolnay avatar Mar 15 '24 19:03 dtolnay

@bors ping

dtolnay avatar Mar 15 '24 19:03 dtolnay

@bors ping

dtolnay avatar Mar 15 '24 19:03 dtolnay

:sleepy: I'm awake I'm awake

bors avatar Mar 15 '24 19:03 bors

@bors r+

dtolnay avatar Mar 15 '24 19:03 dtolnay

:pushpin: Commit 55ba9e72a5929f6e795dc8e2b2215bf6fc176933 has been approved by dtolnay

It is now in the queue for this repository.

bors avatar Mar 15 '24 19:03 bors

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
GITHUB_ENV=/home/runner/work/_temp/_runner_file_commands/set_env_58d0d23c-d396-4ff9-a7c7-d5965706ac68
GITHUB_EVENT_NAME=pull_request
GITHUB_EVENT_PATH=/home/runner/work/_temp/_github_workflow/event.json
GITHUB_GRAPHQL_URL=https://api.github.com/graphql
GITHUB_HEAD_REF=feature/collect-box-str
GITHUB_JOB=pr
GITHUB_PATH=/home/runner/work/_temp/_runner_file_commands/add_path_58d0d23c-d396-4ff9-a7c7-d5965706ac68
GITHUB_REF=refs/pull/99969/merge
GITHUB_REF_NAME=99969/merge
GITHUB_REF_PROTECTED=false
---
#13 3.690 Building wheels for collected packages: reuse
#13 3.691   Building wheel for reuse (pyproject.toml): started
#13 4.021   Building wheel for reuse (pyproject.toml): finished with status 'done'
#13 4.022   Created wheel for reuse: filename=reuse-1.1.0-cp310-cp310-manylinux_2_35_x86_64.whl size=181117 sha256=f5f58750481f69515c2c0d1d503daf565e2565c370d07fc6aeb95fe3498b4269
#13 4.022   Stored in directory: /tmp/pip-ephem-wheel-cache-6av8qthb/wheels/c2/3c/b9/1120c2ab4bd82694f7e6f0537dc5b9a085c13e2c69a8d0c76d
#13 4.025 Installing collected packages: boolean-py, binaryornot, setuptools, reuse, python-debian, markupsafe, license-expression, jinja2, chardet
#13 4.046   Attempting uninstall: setuptools
#13 4.046     Found existing installation: setuptools 59.6.0
#13 4.047     Not uninstalling setuptools at /usr/lib/python3/dist-packages, outside environment /usr
---
    Checking rustc_log v0.0.0 (/checkout/compiler/rustc_log)
    Checking shlex v1.3.0
    Checking time v0.3.34
   Compiling rustc-main v0.0.0 (/checkout/compiler/rustc)
error[E0282]: type annotations needed for `Box<_>`
  --> /cargo/registry/src/index.crates.io-6f17d22bba15001f/time-0.3.34/src/format_description/parse/mod.rs:83:9
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items

For more information about this error, try `rustc --explain E0282`.
error: could not compile `time` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...

rust-log-analyzer avatar Mar 15 '24 19:03 rust-log-analyzer

@bors r-

matthiaskrgr avatar Mar 15 '24 20:03 matthiaskrgr

https://github.com/time-rs/time/blob/v0.3.34/time/src/format_description/parse/mod.rs#L83-L86

pub fn parse_owned(...) -> Result<OwnedFormatItem, InvalidFormatDescription> {
    ...
    let format_items: impl Iterator<Item = Result<Item<'_>, Error>> = /*...*/;
    let items = format_items
        .map(|res| res.map(Into::into))
        .collect::<Result<Box<_>, _>>()?;
    Ok(items.into())
}
error[E0282]: type annotations needed for `Box<_>`
  --> time-0.3.34/src/format_description/parse/mod.rs:83:9
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
help: consider giving `items` an explicit type, where the placeholders `_` are specified
   |
83 |     let items: Box<_> = format_items

Previously, it inferred impl<T> From<T> for T for the first Into conversion (it's redundant; you can comment the line containing the two maps, and it still compiles), and impl From<Box<[Item<'_>]>> for OwnedFormatItem for the second Into conversion.

I tested that the addition of just a single impl FromIterator<char> for Box<str> is enough to make it fail as above.

dtolnay avatar Mar 16 '24 03:03 dtolnay

Yeah, I guess things tend to break if they aren't tested for 2 years :) Thanks for looking into the failure, @dtolnay! What do you recommend as next steps here? Looks like you've already gotten a fix merged to time: https://github.com/time-rs/time/pull/671. Should I update the version rustc is depending on?

calebsander avatar Mar 16 '24 13:03 calebsander

This is blocked on a time release for now, then maybe a crater run.

dtolnay avatar Mar 16 '24 21:03 dtolnay

The time fix got published in 0.3.35. @calebsander, could you please open a separate PR that just updates the version of time in this repo's Cargo.lock? cargo +nightly update time. That should fix the failure from https://github.com/rust-lang/rust/pull/99969#issuecomment-2000296785

According to rg 'name = "time"' -A1 there is also an old time in src/tools/rust-analyzer/Cargo.lock. I believe we need a PR to https://github.com/rust-lang/rust-analyzer, and then that will get synced back here in a PR that looks like https://github.com/rust-lang/rust/pull/124202.

dtolnay avatar May 01 '24 18:05 dtolnay

Thanks for the heads up, @dtolnay! I posted #124736 to upgrade the version of the time dependency. As far as I can tell, the version of time used by rust-analyzer doesn't need an update because it doesn't have any optional features enabled:

time = { version = "0.3", default-features = false }

The time::format_description module (where the compilation error occurred) is conditionally compiled only if the formatting or parsing features are requested:

#[cfg(any(feature = "formatting", feature = "parsing"))]
pub mod format_description;

calebsander avatar May 05 '24 04:05 calebsander

@bors r+ rollup=iffy

dtolnay avatar May 18 '24 19:05 dtolnay

:pushpin: Commit c92c22826015168581f82e8b0f870a8cef9209b9 has been approved by dtolnay

It is now in the queue for this repository.

bors avatar May 18 '24 19:05 bors

:hourglass: Testing commit c92c22826015168581f82e8b0f870a8cef9209b9 with merge ce8c2dcd0306dc11e1f54d5aebb6ed30691bd01b...

bors avatar May 18 '24 23:05 bors

The job aarch64-gnu failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling rustc_lint v0.0.0 (/checkout/compiler/rustc_lint)
[RUSTC-TIMING] rustc_metadata test:false 43.201
   Compiling rustc_ty_utils v0.0.0 (/checkout/compiler/rustc_ty_utils)

Session terminated, killing shell...::group::Clock drift check
##[error]The runner has received a shutdown signal. This can happen when the runner service is stopped, or a manually started runner is canceled.
  network time: Sun, 19 May 2024 00:01:52 GMT
##[endgroup]
 ...killed.
##[error]The operation was canceled.

rust-log-analyzer avatar May 19 '24 00:05 rust-log-analyzer

:broken_heart: Test failed - checks-actions

bors avatar May 19 '24 00:05 bors

Can't tell what failed.

@bors retry

dtolnay avatar May 19 '24 00:05 dtolnay

:hourglass: Testing commit c92c22826015168581f82e8b0f870a8cef9209b9 with merge bfa3635df920454cdf03f6d268dfaac769375df3...

bors avatar May 19 '24 02:05 bors