Stabilize slice::strip_prefix and slice::strip_suffix

Contributor

rust-highfive commented on Oct 12, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

r? @Amanieu for t-libs approval

Amanieu commented on Oct 16, 2020

@rfcbot fcp merge

This API seems fine for now, but rust-lang/rfcs#2500 plans to generalize them to take a Pattern as an argument instead.

rfcbot commented on Oct 16, 2020 •

Team member @Amanieu 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.

That seems like it will be at least an inference break - do we want to try to future proof these somehow?

KodrAus commented on Oct 23, 2020

The stabilized str variants expose the unstable Pattern trait. Is there anything stopping us from doing that here too?

lzutao commented on Oct 23, 2020

@KodrAus std::str::pattern::Pattern is str oriented, .i.e. it only supports string type.
There are plans to generalize Pattern to other types. But those are not implemented.

ijackson commented on Oct 24, 2020

KodrAus commented on Nov 26, 2020

We discussed this a bit in the recent Libs meeting and felt that if we wanted to bring this forward and stabilize it ahead of reworking the pattern API to be non-str specific then we could reduce some churn by making the signatures generic on a trivial unstable trait that we replace with Pattern when we can.

Something like:

#[unstable] // We never stabilize this trait
pub trait SlicePattern {
    type Item;
    fn as_slice(&self) -> &[Self::Item];
}

impl<T> SlicePattern for [T] {
    type Item = T;

    fn as_slice(&self) -> &[Self::Item] {
        self
    }
}

impl<T> [T] {
    #[stable]
    pub fn strip_prefix<P: SlicePattern<Item = T>>(&self, prefix: P) -> Option<&[T]>;

    #[stable]
    pub fn strip_suffix<P: SlicePattern<Item = T>>(&self, suffix: P) -> Option<&[T]>;
}

We may still get breakage from inference failures, but bring the API closer to what we actually want in the long term, and only need to change the unstable backing trait.

What do you think?

ijackson commented on Nov 26, 2020

ijackson commented on Dec 3, 2020

Hi. I have updated this now. I wrote a bunch of stuff in the commit message which github likes to hide so here it is for your err delectation and delight?

Stablize slice::strip_prefix and strip_suffix, with SlicePattern

We hope later to extend `core::str::Pattern` to slices too, perhaps as
part of stabilising that.  We want to minimise the amount of type
inference breakage when we do that, so we don't want to stabilise
strip_prefix and strip_suffix taking a simple `&[T]`.

@KodrAus suggested the approach of introducing a new perma-unstable
trait, which reduces this future inference break risk.

I found it necessary to make two impls of this trait, as the unsize
coercion don't apply when hunting for trait implementations.

Since SlicePattern's only method returns a reference, and the whole
trait is just a wrapper for slices, I made the trait type be the
non-reference type [T] or [T;N] rather than the reference.  Otherwise
the trait would have a lifetime parameter.

I marked both the no-op conversion functions `#[inline]`.  I'm not
sure if that is necessary but it seemed at the very least harmless.

library/core/src/slice/mod.rs

Outdated

Show resolved

Amanieu commented 12 days ago

@rust-lang/libs could you fill in checkboxes in #77853 (comment) considering the updated proposal with a trait bound?

rfcbot commented 7 days ago

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

rust-log-analyzer commented 7 days ago

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

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking hashbrown v0.9.0
    Checking object v0.22.0
    Checking miniz_oxide v0.4.0
    Checking addr2line v0.14.0
error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
   --> library/std/src/sys/windows/path.rs:103:37
    |
103 |     match path.bytes().strip_prefix(prefix.as_bytes()) {
    |                                     ^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
    = help: the trait `Sized` is not implemented for `[u8]`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
error: could not compile `std`
error: could not compile `std`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:30

ijackson commented 7 days ago

Investigating.

ijackson commented 7 days ago

How embarrassing. I forgot the ?Sized bound on the type parameters, so this new version didn't work when the pattern was a slice! All the tests were for arrays, so the tests didn't spot it. Luckily something on Windows was added in the meantime which used strip_prefix with an actual slice, so that found the problem

I have rebased onto master, and fixed this bug, and also added a doctest which passes a slice (playing about a bit with byte strings). LMK if you would prefer some different test.