Reduce use of the pub-in-private hack

[cuviper]

A recent thread on internals got me thinking whether we could avoid the private_impl!{} hack after all. With a few extra constraints, this does work!

With ParallelString: Borrow<str>, we can provide default implementations of all its methods, and should be free to add new methods the same way. Then a blanket impl only has to meet the constraints, so there's also less repetition defining these methods.

Similarly ParallelSlice<T: Sync>: Borrow<[T]> works for slices, and a new ParallelSliceMut<T: Send>: BorrowMut<[T]> for mutable slices. The latter is also added to the prelude.

I also considered AsRef and AsMut, which are very similar to borrows, but I decided against it for the single fact that strings implement both AsRef<str> and AsRef<[u8]>. This would make it ambiguous if ParallelString and ParallelSlice shared any method names, and to drive that point home I went ahead and added par_split and par_split_mut on slices.

The only remaining private_impl!{} is in rayon::str::Pattern. I don't think we can use a similar trick on that one, but the more I think about it, the more I think we should just hide Pattern altogether as pub-in-private itself. Its API is not great, and not something we really want folks to call directly. All users really need to know are which types implement it, and we can document that on the public splitter functions that use it.

[cuviper]

I went ahead and added par_split and par_split_mut on slices.

Sorry if that makes this PR a little scatter-brained... the more I look through the standard library, the more I find that we can parallelize! 😄

[nikomatsakis]

This looks great. I am mostly wondering whether Borrow trait is the right choice, vs (say) Deref. Did this come up on the internals thread you cited (I'm behind on following internals atm, have to catch up).

[nikomatsakis]

This looks great. I am mostly wondering whether Borrow trait is the right choice, vs (say) Deref. Did this come up on the internals thread you cited (I'm behind on following internals atm, have to catch up).

[nikomatsakis]

Hmm, so I wonder -- is Borrow a better choice than Deref? The overlap between those two traits is fairly extreme, iirc the major difference has to do with the constraints that the hash of the "borrow" value and the hash of the original have to be equal. But it's also possible (in theory, at least) for a single type T to support multiple Borrow<U> implementations (i.e., you may have some FooString that implements Borrow<str> but also Borrow<Vec<_>> and so forth), so using Borrow may add flexibility.

I wonder if anyone on @rust-lang/libs might have thoughts here -- the question is, if we want to implement parallel operations on anything that can "yield up" a str, which of the various traits ought we to use? @cuviper selected Borrow, but Deref seems potentially suitable; I think @cuviper rejected AsRef because of the ambiguity it can induce:

I also considered AsRef and AsMut, which are very similar to borrows, but I decided against it for the single fact that strings implement both AsRef and AsRef<[u8]>. This would make it ambiguous if ParallelString and ParallelSlice shared any method names, and to drive that point home I went ahead and added par_split and par_split_mut on slices.

That same point might be used as an argument in favor of Deref<Target = str>, no?

[cuviper]

The linked internals thread was more about calling pub-in-private a negative pattern, to the point that it might deserve a warning or error - especially for cases like this that couldn't just use pub(restricted).

I didn't consider Deref, but I see how that's an even tighter choice since there can be only one Target for a given type. I'm in favor.

[nikomatsakis]

This is basically analogous to the split we do on strings right now? Seems very familiar. =)

[cuviper]

Yes indeed, literally copied and tweaked to fit the new use cases. 😀

Which I agree is not ideal, but the code is just small enough that I fear an abstraction would weigh as much or more in boilerplate. But maybe the abstracted parts will still be simple enough compared to the tricky parts that could be shared. I'll try it.

[cuviper]

When switching ParallelString to Deref, I get a lifetime error that I don't understand. In this code:

/// Parallel iterator over lines in a string
pub struct Lines<'ch>(&'ch str);

impl<'ch> ParallelIterator for Lines<'ch> {
    type Item = &'ch str;

    fn drive_unindexed<C>(self, consumer: C) -> C::Result
        where C: UnindexedConsumer<Self::Item>
    {
        self.0
            .par_split_terminator('\n')
            .map(|line| if line.ends_with('\r') {
                     &line[..line.len() - 1]
                 } else {
                     line
                 })
            .drive_unindexed(consumer)
    }
}

I get:

error: `self.0` does not live long enough
   --> src/str.rs:407:9
    |
407 |         self.0
    |         ^^^^^^ does not live long enough
...
415 |     }
    |     - borrowed value only lives until here
    |
note: borrowed value must be valid for the lifetime 'ch as defined on the body at 406:4...
   --> src/str.rs:406:5
    |
406 | /     {
407 | |         self.0
408 | |             .par_split_terminator('\n')
409 | |             .map(|line| if line.ends_with('\r') {
...   |
414 | |             .drive_unindexed(consumer)
415 | |     }
    | |_____^

Why does self.0 not have its full 'ch lifetime as declared? Especially when the same code works under Borrow. Even if I write let s: &'ch str = self.0; it tells me s does not live long enough.

And there's a similar error in SplitWhitespace which also redirects to an internal split the same way.

[cuviper]

They're happy if I use these type definitions instead:

pub struct Lines<'ch>(SplitTerminator<'ch, char>);
pub struct SplitWhitespace<'ch>(Split<'ch, fn(char) -> bool>);

i.e. creating the inner splits up front. But I worry there's a real gotcha lingering here...

[cuviper]

Is there something about Deref being a lang item that makes the lifetime of its returned references strictly less than the type itself? That's my best hand-waving guess here...

[cuviper]

If I try to give that updated type a new(), I get more errors:

impl<'ch> Lines<'ch> {
    fn new(s: &str) -> Self {
        Lines(s.par_split_terminator('\n'))
    }
}

error[E0495]: cannot infer an appropriate lifetime for autoref due to conflicting requirements
   --> src/str.rs:403:17
    |
403 |         Lines(s.par_split_terminator('\n'))
    |                 ^^^^^^^^^^^^^^^^^^^^
    |
note: first, the lifetime cannot outlive the anonymous lifetime #1 defined on the body at 402:28...
   --> src/str.rs:402:29
    |
402 |       fn new(s: &str) -> Self {
    |  _____________________________^
403 | |         Lines(s.par_split_terminator('\n'))
404 | |     }
    | |_____^
note: ...so that the type `&str` is not borrowed for too long
   --> src/str.rs:403:15
    |
403 |         Lines(s.par_split_terminator('\n'))
    |               ^
note: but, the lifetime must be valid for the lifetime 'ch as defined on the body at 402:28...
   --> src/str.rs:402:29
    |
402 |       fn new(s: &str) -> Self {
    |  _____________________________^
403 | |         Lines(s.par_split_terminator('\n'))
404 | |     }
    | |_____^
note: ...so that expression is assignable (expected str::Lines<'ch>, found str::Lines<'_>)
   --> src/str.rs:403:9
    |
403 |         Lines(s.par_split_terminator('\n'))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[cuviper]

Backing up, one reason ParallelSlice might prefer Borrow is that fixed-size arrays don't implement Deref. Of course, they currently only implement Borrow up to [T; 32], but it's something.

error: no method named `par_split` found for type `[{integer}; 4]` in the current scope
   --> src/iter/test.rs:821:27
    |
821 |     let v: Vec<_> = slice.par_split(|num| num % 3 == 0).collect();
    |                           ^^^^^^^^^
    |
    = note: the method `par_split` exists but the following trait bounds were not satisfied: `[{integer}; 4] : std::ops::Deref`, `[{integer}; 4] : std::ops::Deref`, `[{integer}] : std::ops::Deref`, `[{integer}] : std::ops::Deref`
    = help: items from traits can only be used if the trait is implemented and in scope; the following traits define an item `par_split`, perhaps you need to implement one of them:
    = help: candidate #1: `slice::ParallelSlice`
    = help: candidate #2: `str::ParallelString`

[cuviper]

(on abstracting par_split for &str/&[T] and par_split_mut for &mut [T])

Which I agree is not ideal, but the code is just small enough that I fear an abstraction would weigh as much or more in boilerplate. But maybe the abstracted parts will still be simple enough compared to the tricky parts that could be shared. I'll try it.

I started slightly down that path, but I'm really not feeling it. It's a mess of slightly different ways to find, rfind, split_at, index with different ranges, turn into split iterators, and probably more...

[nikomatsakis]

Why does self.0 not have its full 'ch lifetime as declared?

I'll have to look. I'm not sure why Deref and Borrow would behave differently in this respect off hand, though it may be how the auto-deref winds up working out. If you try (*self.0).par_split_whitespace or something, does it work better?

[cuviper]

If you try (*self.0).par_split_whitespace or something, does it work better?

With (*self.0) it complains that str : Deref is not satisfied -- maybe that's part of the explanation, since Borrow is implemented for values. Then again, AsRef is not implemented for values AFAICS, yet that worked fine here too.

I also tried (&*self.0) and {self.0}, and these still report not living long enough.

[cuviper]

Hmm. I think because the methods use &self, then the lifetime of the returned object is getting tied to the lifetime of the reference itself. Minimal reproducer (playground):

use std::ops::Deref;
trait Foo: Deref<Target = str> {
    fn string(&self) -> &str { self }
}
impl<T: ?Sized + Deref<Target = str>> Foo for T {}

fn main() {
    let x = "".string();
}

error: borrowed value does not live long enough
 --> <anon>:8:24
  |
8 |     let x = "".string();
  |             --         ^ temporary value dropped here while still borrowed
  |             |
  |             temporary value created here
9 | }
  | - temporary value needs to live until here
  |
  = note: consider using a `let` binding to increase its lifetime

I think that &self function is ending up like: fn string<'a, 'b>(&'a &'b str) -> &'a str.

And in the actual use case, we're getting tied to the lifetime of &self.0.

[cuviper]

Then again, AsRef is not implemented for values AFAICS, yet that worked fine here too.

Oh, not for all T, but there is impl AsRef<str> for str which suffices here.

[cuviper]

I figured out a split abstraction that I don't hate. :)
Only a little shorter overall, but the more-complicated parts are better contained.

[cuviper]

@nikomatsakis How do you feel about making Pattern wholly pub-in-private? I think we don't really even want its interface visible to users. They only need to know what implements it.

[nikomatsakis]

How do you feel about making Pattern wholly pub-in-private? I think we don't really even want its interface visible to users. They only need to know what implements it.

Well, there is a downside, in that users then cannot "layer" on top of the API. i.e., they may be able to call split, but they can't write a function that takes "some pattern" and calls split with it.

[cuviper]

FWIW, that's also the case for std::str::pattern::Pattern, only it uses #[unstable] instead of privacy hacks. Otherwise I probably would have just layered on top of that here in rayon!

[nikomatsakis]

@cuviper well i'm not opposed to doing it for now.

[cuviper]

OK, it's hidden now.

[nikomatsakis]

OK, so, I was thinking about it, and I think I see why Deref<Target=XXX> was a bad choice -- we want to implement (e.g.) ParallelSlice for [T]. But [T] does not implement Deref<Target=[T]>! In other words, Deref is not reflexive.

To that end, AsRef or Borrow are better choices. Borrow still feels kind of wrong to me though.

I considered an alternative design that feels "more right" to me -- adding a fn as_str(&self) -> &str and then implementing the trait twice, once for str and once for T: Deref<Target=str> -- but the coherence rules prevent me from doing that (in particular, I am not allowed to rely on the fact that str: !Deref<Target=str>). Annoying.

JFTR, partial diff for the alternate design:

diff --git a/src/slice.rs b/src/slice.rs
index 471343d..4695b90 100644
--- a/src/slice.rs
+++ b/src/slice.rs
@@ -9,14 +9,14 @@ use std::borrow::{Borrow, BorrowMut};
 use std::cmp;
 
 /// Parallel extensions for slices.
-pub trait ParallelSlice<T: Sync>: Borrow<[T]> {
+pub trait ParallelSlice<T: Sync>: AsRef<[T]> {
     /// Returns a parallel iterator over subslices separated by elements that
     /// match the separator.
     fn par_split<P>(&self, separator: P) -> Split<T, P>
         where P: Fn(&T) -> bool + Sync
     {
         Split {
-            slice: self.borrow(),
+            slice: self.as_ref(),
             separator: separator,
         }
     }
@@ -26,7 +26,7 @@ pub trait ParallelSlice<T: Sync>: Borrow<[T]> {
     fn par_windows(&self, window_size: usize) -> Windows<T> {
         Windows {
             window_size: window_size,
-            slice: self.borrow(),
+            slice: self.as_ref(),
         }
     }
 
@@ -35,12 +35,12 @@ pub trait ParallelSlice<T: Sync>: Borrow<[T]> {
     fn par_chunks(&self, chunk_size: usize) -> Chunks<T> {
         Chunks {
             chunk_size: chunk_size,
-            slice: self.borrow(),
+            slice: self.as_ref(),
         }
     }
 }
 
-impl<T: Sync, V: ?Sized + Borrow<[T]>> ParallelSlice<T> for V {}
+impl<T: Sync, V: ?Sized + AsRef<[T]>> ParallelSlice<T> for V {}
 
 
 /// Parallel extensions for mutable slices.
diff --git a/src/str.rs b/src/str.rs
index 3c63541..29024f1 100644
--- a/src/str.rs
+++ b/src/str.rs
@@ -43,10 +43,12 @@ fn find_char_midpoint(chars: &str) -> usize {
 
 
 /// Parallel extensions for strings.
-pub trait ParallelString: Borrow<str> {
+pub trait ParallelString {
+    fn as_str(&self) -> &str;
+
     /// Returns a parallel iterator over the characters of a string.
     fn par_chars(&self) -> Chars {
-        Chars { chars: self.borrow() }
+        Chars { chars: self.as_str() }
     }
 
     /// Returns a parallel iterator over substrings separated by a
@@ -55,7 +57,7 @@ pub trait ParallelString: Borrow<str> {
     /// Note: the `Pattern` trait is private, for use only by Rayon itself.
     /// It is implemented for `char` and any `F: Fn(char) -> bool + Sync`.
     fn par_split<P: Pattern>(&self, separator: P) -> Split<P> {
-        Split::new(self.borrow(), separator)
+        Split::new(self.as_str(), separator)
     }
 
     /// Returns a parallel iterator over substrings terminated by a
@@ -66,7 +68,7 @@ pub trait ParallelString: Borrow<str> {
     /// Note: the `Pattern` trait is private, for use only by Rayon itself.
     /// It is implemented for `char` and any `F: Fn(char) -> bool + Sync`.
     fn par_split_terminator<P: Pattern>(&self, terminator: P) -> SplitTerminator<P> {
-        SplitTerminator::new(self.borrow(), terminator)
+        SplitTerminator::new(self.as_str(), terminator)
     }
 
     /// Returns a parallel iterator over the lines of a string, ending with an
@@ -74,7 +76,7 @@ pub trait ParallelString: Borrow<str> {
     /// The final line ending is optional, and line endings are not included in
     /// the output strings.
     fn par_lines(&self) -> Lines {
-        Lines(self.borrow())
+        Lines(self.as_str())
     }
 
     /// Returns a parallel iterator over the sub-slices of a string that are
@@ -83,11 +85,21 @@ pub trait ParallelString: Borrow<str> {
     /// As with `str::split_whitespace`, 'whitespace' is defined according to
     /// the terms of the Unicode Derived Core Property `White_Space`.
     fn par_split_whitespace(&self) -> SplitWhitespace {
-        SplitWhitespace(self.borrow())
+        SplitWhitespace(self.as_str())
+    }
+}
+
+impl ParallelString for str {
+    fn as_str(&self) -> &str {
+        self
     }
 }
 
-impl<T: ?Sized + Borrow<str>> ParallelString for T {}
+impl<T: ?Sized + ::std::ops::Deref<Target=str>> ParallelString for T {
+    fn as_str(&self) -> &str {
+        self
+    }
+}
 
 
 // /////////////////////////////////////////////////////////////////////////

[nikomatsakis]

So I'm a bit torn:

• AsRef feels like the right trait, but it is ambiguous.
• Borrow works, but it is tied to hashtables and not obviously the trait we want. Moreover, it is also potentially ambiguous -- if libstd were to add more borrow impls in the future, which imo would be reasonable, then our APIs would become ambiguous.
• Deref is unambiguous, which is good, but it's not reflexive and the coherence rules prevent us from using it.

An alternative might be to just add (undefaulted) as_str() methods and implement ParallelString ourselves for str, String, and whatever other types feel appropriate. I sort of forget, is that where we started? :)

[nikomatsakis]

I sort of forget, is that where we started? :)

Just checked. Basically yes. We were relying on method call's auto-deref instead of manually implementing for String and Vec though.

That said, I sort of like the idea of factoring things slightly differently, kind of a fourth proposal:

• Some form of user-visible, stable trait that includes as_str() (and as_slice()).
• An extension trait (that is not implementable) that has our extension methods.

The idea is that you can make things usable by ParallelString or ParallelSlice by implementing the stable trait, but we can add more methods later if we want without disturbing you.

[cuviper]

pub trait ParallelString {
    fn as_str(&self) -> &str;
...
}

impl ParallelString for str {
   fn as_str(&self) -> &str {
       self
    }
}

I like the simplicity of this a lot, and I think I'd do the same for slices -- maybe just bikeshedded to as_parallel_string() so we don't collide with all the other as_str() methods in the world.

An alternative might be to just add (undefaulted) as_str() methods and implement ParallelString ourselves for str, String, and whatever other types feel appropriate. I sort of forget, is that where we started? :)

It is where we started, but I think that's a good thing. I'd even leave these implemented just for str and [T], so we don't have to get into the dereferencing business at all. Auto-Deref should handle the basics for us, and folks can use their own as_ref() or whatever for other cases.

[cuviper]

OK, pushed.

We can do it with "stable" AsParallelString etc. if you like, but I think it's just as good with required methods. The only difference is that we might not put AsParallel* in the prelude, but I gave the methods more specific names, so I think it's fine this way.

[nikomatsakis]

@cuviper 👍 I like it.

[nikomatsakis]

but I think it's just as good with required methods

So the only caveat, actually: I was wondering what we would do if we wanted to extend this trait with new utilities. I guess that as long as they have defaults, it's not really a breaking change (modulo potential method conflicts). (Same, presumably, as something like Iterator.)

[cuviper]

I guess that as long as they have defaults, it's not really a breaking change (modulo potential method conflicts). (Same, presumably, as something like Iterator.)

Right, having the required as_parallel_string() etc. means we always have a way to implement the defaults. Besides, the returned types all have private constructors, so the only thing one could do on their own type is forward to the same method on some other string.

It's a breaking change only in the sense that it's a breaking change to add a public method anywhere. As long as we're adding to the public namespace, especially in a prelude trait, there's a chance we could collide.

The only way I see around that is if we flipped these to inherent methods on a newtype of our own, so you'd write something more like my_string.as_parallel_string().split(c) instead. Hmm... that actually has some appeal, to get rid of all the par_ prefixes. If we want to go this route, I'd shorten it for the user's sake, perhaps as_par_str() for strings, as_par_ref() and as_par_mut() for slices.

(obligatory "API design is hard")

[nikomatsakis]

@cuviper

The only way I see around that is if we flipped these to inherent methods on a newtype of our own, so you'd write something more like my_string.as_parallel_string().split(c) instead. Hmm... that actually has some appeal, to get rid of all the par_ prefixes. If we want to go this route, I'd shorten it for the user's sake, perhaps as_par_str() for strings, as_par_ref() and as_par_mut() for slices.

Huh, interesting thought. I'm not too worried about adding methods, but I sort of like the "par mode" style, where you can use the API names you were accustomed to. But I'm not sure about the names. as_par_ref() feels kind of confusing -- I think of as_ref() as &Foo<T> to Foo<&T>, but this is &[T] to &[T] -- I mean, we are changing the type, but not in a way that the user wants to be aware of, right?

I'm sort of tempted by just par() -- do we need the par_mut()? It seems like we could make our newtype generic over &[T] and &mut [T] (that just sort of pushes us back to the same question of what traits to use, but there we could probably use Deref<Target=[T]> and DerefMut<Target=[T]>).

I'm envisioning something like

pub struct ParallelSlice<T, U>
where T: Deref<Target=[U]>
{
    slice: T
}

impl ParallelSlice<T>
where T: Deref<Target=[U]>
{
    fn new(self: T) -> Self { ... }

    pub fn split(...) { ... }

    pub fn split_mut(...)
    where T: DerefMut<Target=U>
    { ... }
}

If we went this way, though, I have to wonder if we could do it more universally. i.e., instead of par_iter() and into_par_iter() and so forth we just do foo.par().iter() and foo.par().into_iter() (I guess that for ranges then it would be (0..5).par().map(), which is sort of nice.)

If this were Rust project, I think I'd say "this needs an RFC"...

[nikomatsakis]

If this were Rust project, I think I'd say "this needs an RFC"...

No reason we can't have RFCs...and maybe we should.

[nikomatsakis]

I think probably we should land this PR, which is mostly a refactoring, and then consider this par() concept, which seems to have broader implications, separately.

[cuviper]

I tried out as_par_str() etc. locally. It felt verbose. I'll have to think about par()...

But yeah, I'm happy to see this PR land if you're ready. :)