Unsized marker for AsRef

[mexus]

Apparently AsRef implementation of Either lacks an "unsized" marker ?Sized on Target. It prevents of implementing AsRef<str> for the Either, for instance.

While default Either doesn't compile (without the marker on playground) ...

extern crate either; // 1.5.0
use either::Either;

fn get_default(s: Option<impl AsRef<str>>) -> impl AsRef<str> {
    s.map(Either::Left).unwrap_or_else(|| Either::Right("default"))
}

fn main() {
    println!("{}", get_default(None::<String>).as_ref());
    println!("{}", get_default(Some("not default")).as_ref());
}

... an example with the marker compiles just ok :) with the marker on playground

enum Either2<L, R> {
    Left(L),
    Right(R),
}

impl<L, R, T: ?Sized> AsRef<T> for Either2<L, R>
where
    L: AsRef<T>,
    R: AsRef<T>,
{
    fn as_ref(&self) -> &T {
        match self {
            Either2::Left(x) => x.as_ref(),
            Either2::Right(x) => x.as_ref(),
        }
    }
}

fn get_default(s: Option<impl AsRef<str>>) -> impl AsRef<str> {
    s.map(Either2::Left)
        .unwrap_or_else(|| Either2::Right("default"))
}

fn main() {
    println!("{}", get_default(None::<String>).as_ref());
    println!("{}", get_default(Some("not default")).as_ref());
}

So, my question is: was the ?Sized marker omitted on purpose, or should I make a PR which adds the marker? :)

[cuviper]

In principle I think it would be fine to have Target: ?Sized on our AsRef, and on AsMut too.

My only hesitation is whether this constitutes a breaking change to expand the scope of the impl. That is, if someone has their own unsized type Foo, can they currently write impl AsRef<Foo> for Either? If so, we'd be adding a conflicting implementation when we make ours unsized.

[mexus]

I believe it's impossible: playground

extern crate either; // 1.5.0
use either::Either;

struct Unsized {
    data: [u8],
}

impl<L, R> AsRef<Unsized> for Either<L, R>
where
    L: AsRef<Unsized>,
    R: AsRef<Unsized>,
{
    fn as_ref(&self) -> &Unsized {
        match self {
            Either::Left(x) => x.as_ref(),
            Either::Right(x) => x.as_ref(),
        }
    }
}

produces

error[E0210]: type parameter `L` must be used as the type parameter for some local type (e.g. `MyStruct<L>`)
  --> src/lib.rs:8:1
   |
8  | / impl<L, R> AsRef<Unsized> for Either<L, R>
9  | | where
10 | |     L: AsRef<Unsized>,
11 | |     R: AsRef<Unsized>,
...  |
18 | |     }
19 | | }
   | |_^ type parameter `L` must be used as the type parameter for some local type
   |
   = note: only traits defined in the current crate can be implemented for a type parameter

I've made an unsized type just to be more specific, but I believe it doesn't matter in this case.

[cuviper]

Your example works with concrete types though:

impl AsRef<Unsized> for Either<Box<Unsized>, Box<Unsized>> {
    fn as_ref(&self) -> &Unsized {
        match self {
            Either::Left(x) => x.as_ref(),
            Either::Right(x) => x.as_ref(),
        }
    }
}

[mexus]

Ah, true! I didn't look that far :( If we allow unsized types in AsRef it will rise a conflict: playground.

So it's a breaking change potentially 😢

[cuviper]

I think we could still implement AsRef<str> and AsRef<[T]> though.

[mexus]

Okay, sounds good! Will send a PR

[mexus]

Btw, isn't it a potential-2.0 change? (i mean adding the ?Sized marker)

[cuviper]

Sure, I've added the label.

[nvzqz]

It's a shame that expanding the blanket impl is a breaking change.

[BenjaminBrienen]

Could this be added under a feature gate to make it not technically a breaking change?

[jswrenn]

No; features are global in the dep tree. So a dependency that's added its own conflicting impl might be adversely affected by dep somewhere else in the tree that opts into the feature.

That said: custom DSTs are still extremely niche (and were even more so in 2018). I suspect that this change would cause much less actual breakage than the average MSRV bump.