Fixed-size byte string literals (RFC 339)

[petrochenkov]

This patch changes the type of byte string literals from &[u8] to &[u8; N].
It also implements some necessary traits (IntoBytes, Seek, Read, BufRead) for fixed-size arrays (also related to #21725) and adds test for #17233, which seems to be resolved.

Fixes#18465
[breaking-change]

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[petrochenkov]

Notes:
This patch highlights some small but unpleasant issues in the language and in the library:

• lack of heterogeneous comparison for Option - Option<A> is not comparable to Option<B> if A is comparable to B
• as can't convert &[T; N] to &[T] (although type ascription Type ascription rfcs#803 would be more handy here)
• DST coercions &[T; N] => &[T] don't apply to patterns in match

[alexcrichton]

I personally considered the breakage of this behavior to be a showstopper for the previous attempt:

match foo {b"foo" => {/* ... */}}

Others may have a different opinion though! I'm also getting quite worried about how we're just spraying impls of traits for [T; N] where 0 <= N <= 32, it doesn't feel quite right to me and I'm particularly worried that our metadata size is just growing and growing...

cc @nrc, @nikomatsakis on the coercions aspect, do we perhaps have any RFCs in the pipeline to help here? This relatively small change ended up having quite a large fallout!

[nrc]

re coercions: as for implicit coercions is proposed to work in 401 and re-proposed to not work anymore (customisable lint, warn by default in 803 (type ascription). Type ascription would let you do this coercion much more painlessly (and converting concrete type to trait which should break soon, with those lints).

There's nothing in the pipeline re coercions in matches - there was some RFC recently about array patterns in matches, but I think that makes things stricter. I'm not quite sure what it means to coerce in a pattern - if it means that match e ... where the match arms have type T and we need to coerce e to T then that should be simple to change (I don't think it needs an RFC, but it probably deserves a little thought), but we don't have any immediate plans to do so. If we need to coerce per-arm, rather than just once for the whole match, that is more complex.

[petrochenkov]

@alexcrichton I don't think it's a showstopper for several reasons:

• Unlike RFC 339 itself the fix for patterns is backward compatible and can be done any time before or after 1.0
• The issue is relatively independent from the literals themselves, it's about arrays of fixed size in general and lies in a wider context of RFC 495
• Byte string literals are very rarely used in match - once in the whole Rust codebase (excluding the test for byte string literals themselves) - so they can wait a bit.

So, I'd like to land this PR until it's too late and to work on the fix for patterns independently of it (I currently study the implementation of pattern matching in the compiler).

[petrochenkov]

@nrc I can't say for sure until I understand the implementation, but it does seem a bit strange that (using the tweaked syntax from RFC 495)

let a: &[_] = [1, 2, 3];
match a {
&[2, 3, 4] => ...
}

works but

let a: &[_] = [1, 2, 3];
const A: &'static [i32; 3] = &[2, 3, 4];
match a {
A => ...
}

doesn't. If not general coercions, then some special-casing for arrays and slices should apply here.

[petrochenkov]

it doesn't feel quite right to me and I'm particularly worried that our metadata size is just growing and growing...

Everything about fixed-size arrays feels not quite right until some form of rust-lang/rfcs#884 is implemented, but it doesn't mean that they should be second class citizens. In principle I can kill the new impls and spend some more time manually coercing &[T; N] to &[T] in tests.

[petrochenkov]

@alexcrichton I think it's possible to remove some of the metadata bloat with the next trick (where the coherence rules allow it):

impl<T> ArrayTrait for [T; $N] {} // with macros, for 0 <= $N <= 32, one-time bloat
impl<T> BufRead for T where T: ArrayTrait {} // no bloat

[alexcrichton]

@petrochenkov that's a pretty good idea! I like how it's turned out for fixed-size arrays.

I'm not super happy that this has &b"..."[..] cropping up as something definitely feels wrong about that... It's not the end of the world though.

cc @aturon, I'm curious what you think about the ergonomic fall out here as well as the FixedSizeArray trait. I'd be fine for now leaving FixedSizeArray as our own internal trait which is #[unstable] for now in hope of getting a better system later.

[petrochenkov]

I think I know what to do with byte string literal patterns - they will behave exactly like array patterns - they will denote &[u8; N] when the discriminant of match has type &[u8; M] and &[u8] otherwise. In this regard byte string literal patterns are not quite byte string literals, just like array patterns are not quite array literals. (The implementation is not ready though.)

[bors]

☔ The latest upstream changes (presumably #23107) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

Fixed byte string literal patterns and rebased

[alexcrichton]

Wow, nice work @petrochenkov!

cc @aturon, I'm curious as to your opinion of FixedSizeArray as well. I feel like it may be a little too overzealously used, and it may be a good case for some form of conversion trait, although I'm not quite sure. It is certainly somewhat annoying to have to write &foo[..] just to get an array to go into a slice.

[aturon]

@alexcrichton@petrochenkov Sorry for the delay giving feedback on this.

I'm not too worried about FixedSizeArray at this point, since it's not exported at the std level. That said, it does seem like it shouldn't require a new trait -- either going through slice notation or else a conversion trait.

In any case, I don't think we should hold up the PR just for that reason, since it's a minor impl detail that can be improved later on.

[alexcrichton]

@aturon while the trait itself remains stable, we are leaking stable functionality via the trait itself. Right now the stability pass does not consider the stability of an impl of a trait when it is used, it only considers the stability of the trait itself. This essentially means that a stable trait forces all implementations to be defacto stable today. I'm not sure what our prospects are of fixing this before 1.0, but in the meantime it's basically committing that for arrays of size 0-32 we're committing to provide impls of:

• IntoBytes - e.g. you can construct a CString with them
• io::{Seek, Read, BufRead} for Cursor<[u8; N]>

I'm a little uncomfortable with the two and would prefer coercions to take over, but just want to confirm that you're on board with this information as well.

[petrochenkov]

@alexcrichton

would prefer coercions to take over

What coercions exactly? I remember someone (maybe @japaric or @eddyb ?) proposing "impl lookup with coercions" - if we are not able to find an impl of trait Tr for type T then we try to search it for types coerce(T) (every type T has finite amount of coerced types (right?), and some ordering can be introduced on them), but I don't know how realistic that proposal is and how soon it can be implemented. If it is implemented, then I guess all the impls for &[T; N] and &mut [T; N] could be dropped. Note however, that impls for [T; N] will stay as well as impls of Tr<[T; N]> for some U.

[eddyb]

@blaenk came up with that idea, I merely gave it a name (bound-targeting coercions).
There's a description of this potential feature in the type ascriptions RFC discussion.

[alexcrichton]

@petrochenkov yeah that's basically what I was thinking, although I'm not sure if it's actually plausible. I'm just wary of committing to providing impls for these traits for all these fixed size arrays.

[petrochenkov]

@alexcrichton
I have removed the unwanted impls in the last commit and used &b"..."[..] instead.
I can't say I like the result, but at least it is the most forward compatible solution.
When the conventions about conversion traits and slice traits are worked out (hopefully soon), the impls can be reintroduced in some form, but that will be a separate story.
(I've kept FixedSizeArray in place, I'm planning to use it later to reduce code bloat from some already existing array impls.)

[alexcrichton]

Gah sorry about the delay on this @petrochenkov! I meant to discuss this with @aturon in more depth in the meantime, but we never got around to it :(. Due to the conservative nature of this, however, I'm fine landing this for now, thanks for being patient!

As an aside, CString::new(b"foo") can be replaced with CString::new("foo") nowadays that we can create it from something other than a byte slice (e.g. avoiding the &a[..]). No need to worry for this PR though!

[alexcrichton]

@bors: r+ a7e1c1d

[bors]

⌛ Testing commit a7e1c1d with merge 4f63ae2...

[bors]

💔 Test failed - auto-win-64-nopt-t

[petrochenkov]

Fixed the failed test.

@bors: r=alexcrichton dccd17d
^^^ This won't work, right?

[alexcrichton]

@bors: r+ dccd17d

It does indeed work! It just has to come from a whitelist of reviewers :)

[bors]

⌛ Testing commit dccd17d with merge 46f649c...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt