rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
93.93k stars 12.09k forks source link

alloc: implement FromIterator for Box<str> #99969

Closed calebsander closed 2 weeks ago

calebsander commented 1 year ago

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

rust-highfive commented 1 year ago

r? @kennytm

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

rustbot commented 1 year ago

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:

calebsander commented 1 year ago

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

pitaj commented 1 year ago

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

calebsander commented 1 year ago

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

calebsander commented 1 year ago

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

dtolnay commented 4 months ago

@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
rfcbot commented 4 months ago

Team member @dtolnay has proposed to merge 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 commented 2 months ago

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

rfcbot commented 2 months ago

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.

dtolnay commented 2 months ago

@bors r+

dtolnay commented 2 months ago

@bors ping

dtolnay commented 2 months ago

@bors ping

bors commented 2 months ago

:sleepy: I'm awake I'm awake

dtolnay commented 2 months ago

@bors r+

bors commented 2 months ago

:pushpin: Commit 55ba9e72a5929f6e795dc8e2b2215bf6fc176933 has been approved by dtolnay

It is now in the queue for this repository.

rust-log-analyzer commented 2 months ago

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) ```plain 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... ```
matthiaskrgr commented 2 months ago

@bors r-

dtolnay commented 2 months ago

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.

calebsander commented 2 months ago

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?

dtolnay commented 2 months ago

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

dtolnay commented 1 month ago

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.

calebsander commented 4 weeks ago

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;
dtolnay commented 2 weeks ago

@bors r+ rollup=iffy

bors commented 2 weeks ago

:pushpin: Commit c92c22826015168581f82e8b0f870a8cef9209b9 has been approved by dtolnay

It is now in the queue for this repository.

bors commented 2 weeks ago

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

rust-log-analyzer commented 2 weeks ago

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) ```plain 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. ```
bors commented 2 weeks ago

:broken_heart: Test failed - checks-actions

dtolnay commented 2 weeks ago

Can't tell what failed.

@bors retry

bors commented 2 weeks ago

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

bors commented 2 weeks ago

:sunny: Test successful - checks-actions Approved by: dtolnay Pushing bfa3635df920454cdf03f6d268dfaac769375df3 to master...

rust-timer commented 2 weeks ago

Finished benchmarking commit (bfa3635df920454cdf03f6d268dfaac769375df3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (primary 4.1%) This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment. | | mean | range | count | |:----------------------------------:|:----:|:------------:|:-----:| | Regressions ❌
(primary) | 4.1% | [4.0%, 4.2%] | 2 | | Regressions ❌
(secondary) | - | - | 0 | | Improvements ✅
(primary) | - | - | 0 | | Improvements ✅
(secondary) | - | - | 0 | | All ❌✅ (primary) | 4.1% | [4.0%, 4.2%] | 2 |

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 667.801s -> 671.223s (0.51%) Artifact size: 316.10 MiB -> 316.05 MiB (-0.01%)