Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Extend and stabilize the FixedSizeArray trait #1915

Closed
wants to merge 1 commit into from

Conversation

SimonSapin
Copy link
Contributor

Extend and stabilize the FixedSizeArray trait, as a stop-gap solution for integer parameters in generics.

Rendered

@glaebhoerl
Copy link
Contributor

Can we somehow make it so that a FixedSizeArray bound also implies Index and IndexMut? That is, so that this is legal:

fn example<Array: FixedSizeArray<Item=Foo>>(array: Array) -> Foo {
    array[42]
}

If so, I suggest a little syntax sugar:

fn example<Array: [Foo]>(array: Array) -> Foo {
    array[42]
}

I.e., [T] as a bound would mean the same thing as FixedSizeArray<Item=T>. The [T] bound would be satisfied by all built-in array types containing Ts. Makes sense to me.

@SimonSapin
Copy link
Contributor Author

@glaebhoerl Arrays don’t implement Index:

use std::ops::*;

fn a<T: Index<usize>>(_: &T) {}

fn main() {
    a(&[0_u8; 10])
}
error[E0277]: the trait bound `[u8; 10]: std::ops::Index<usize>` is not satisfied
 --> a.rs:6:5
  |
6 |     a(&[0_u8; 10])
  |     ^ the trait `std::ops::Index<usize>` is not implemented for `[u8; 10]`
  |
  = note: the type `[u8; 10]` cannot be indexed by `usize`
  = note: required by `a`

error: aborting due to previous error

I think that what happens when you use the array[index] syntax is that the array is auto-referenced, and that reference is implicitly coerced to a slice.

@glaebhoerl
Copy link
Contributor

glaebhoerl commented Feb 21, 2017

Interesting. Together with the observation that the original formulation of the FixedSizeArray trait was basically "anything that can be implicitly coerced ('unsized') to a slice", it seems it should be possible to make this work? Can we do e.g. unsafe trait FixedSizedArray: Unsize<[Self::Item]> { ... }?

(To be clear what I'm interested in is indexing syntax working once you have a FixedSizeArray bound, not what type Index is actually implemented for; it seems I was accidentally too specific in my previous comment.)

@SimonSapin
Copy link
Contributor Author

This compiles:

#![feature(unsize)]
fn foo<T, A: ::std::marker::Unsize<[T]>>(a: &A) {
    &(a as &[T])[0];
}

But &a[0] doesn’t. That is, in a generic context, having an Unsize bound is not enough to make the coercion implicit. So it’s not much of an improvement over explicitly calling the as_slice method from the FixedSizedArray trait.

@glaebhoerl
Copy link
Contributor

Humbug. I still think it'd be nice if it could be made to work somehow. :)

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the RFC. label Feb 21, 2017
@burdges
Copy link

burdges commented Feb 21, 2017

If I understand, you could not write [A::Item; A::LENGTH] because associated constants still cannot be used to define types, but associated constants not working when defining types is considered a bug and will simply be fixed eventually as changes related to MIR progress, right?

@eddyb
Copy link
Member

eddyb commented Feb 21, 2017

@burdges There are two problems to solve, rust-lang/rust#40008 makes [T; Struct::CONST] work just fine, but if the constant depends on parameters it won't work (because array types still take pure integers).

@SimonSapin
Copy link
Contributor Author

If the LENGTH associated constant is mostly useless, or is significantly difficult to implement, I’m happy to drop it from the RFC and propose stabilizing the FixedSizedArray trait without it.

(Separately, I don’t know if an associated type is preferable over a type parameter in this case. Any opinion?)

@sgrif
Copy link
Contributor

sgrif commented Feb 22, 2017

I think we should mention that this only enables a limited set of generic code in the drawbacks. T: FixedSizeArray<Item=U> is much more likely to cause a coherence issue than [U; 1] or a hypothetical [U; Size] type would.

For example, impl<T> NonLocalTrait for T: FixedSizedArray<Item=LocalType> would not be allowed, but impl NonLocalTrait for [LocalType; 1] would.

A slightly more concrete example which is similar to something we do in Diesel would be if you have a trait where you want to take ownership, and abstract over single/multiple values. You might write something like this:

fn insert<T: Insertable>(records: T) -> IncompleteInsertStatement<T> {
    // ...
}

impl<T: Insertable> Insertable for Vec<T> {
    // ...
}

You would not be able to write impl<T: FixedSizeArray> Insertable for T where T::Item: Insertable, as it would conflict with the implementation for Vec<T>.

@plietar
Copy link

plietar commented Feb 22, 2017

@sgrif FixedSizedArray could be marked as #[fundamental], which IIUC would allow this.

@sgrif
Copy link
Contributor

sgrif commented Feb 22, 2017

@plietar That would fix the Vec<T> collision, but it would not fix the first example. It would also still conflict with any other blanket impls that are present. Another impl that we might want to write in Diesel would be:

impl<ST, T: FixedSizedArray> AsExpression<Array<ST>> for T where {
}

But that would conflict with

impl<ST, T: Expression<SqlType=ST>> AsExpression<ST> for T {
}

regardless of whether FixedSizedArray is #[fundamental] or not.

@glaebhoerl
Copy link
Contributor

If we're making the trait work through "compiler magic" anyways, as proposed in the RFC, implemented only by the built-in array types, we might as well let the compiler's left hand know what its right hand is doing and use this information for coherence as well.

@burdges
Copy link

burdges commented Feb 23, 2017

As an aside, the ::LENGTH associated constant, and the ::Item associated type, will prove quite useful even when not programming generically because they let you bundle all the information about the fixed length array type into the type. I've written stuff like

pub const FOO_NAME_LENGTH : Length = 16;
pub type FooNameBytes = [u8; FOO_NAME_LENGTH];
#[derive(Debug, Copy, Clone, Default)]
pub struct FooName(pub FooNameBytes);

often enough to consider writing a macro like

pub trait Fixy {
    const LENGTH: usize;
    type Item;
}

macro_rules! make_Fixy { ($id:ident,$ty:type,$len:expr) => {
    pub struct $id(pub [$ty; $len]);
    impl Fixy for $id {
        const LENGTH: usize = $len;
        type Item = $ty;
    }
} }

except it's not quite that homogeneous.

Additionally, it does not prevent a more general solution from also happening in the future.

Arrays are the only case where integers occur in types, in current Rust.
Therefore a solution specifically for arrays
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just handling arrays doesn't enable things like C++'s std::chrono, so it's definitely not "all". Maybe scope the claim to "std's current trait implementations for arrays" or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what C++’s std::chrono is and how it relates to arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On re-reading you comment it sounds like it doesn’t involve arrays, but it might involve type-level integers? Yes, this RFC does not entirely remove the need for type-level integers, but it doesn’t prevent adding them later either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, apparently I put this comment on a poor choice of line; I thought it had the next one in the context too.

I agree with your comment, just think the motivation is over-selling itself by talking about covering "many (all?) of the use cases for type-level integers". Probably an ignorable nitpick.

Hmm, are associated constants const enough to be used in type checking? Would this enable fn pop_array<T:Array>(x : T) -> [T::Item; T::LENGTH - 1] { ... }?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked this up thread and @eddyb answered : #1915 (comment) This now works when T is some fixed type, but it does not yet work if T is a type parameter like in your question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where "now" is whenever rust-lang/rust#40008 gets merged.

@scottmcm
Copy link
Member

Remove ≥496 impls and make things more consistent? 👍

I think there are two trait impls on array this can't replace, though: Clone and Default.

Maybe also have this function on the trait: fn generate<F:Fn(usize)->Self::Item>(f:F) -> Self;?

Then Default is Array::generate(|_| Default::default()) and Clone is Array::generate(|i| x[i]) (or Array::generate(|i| x[i].clone()) if we wanted to weaken the bound to Clone instead of Copy).

Could even be useful elsewhere for initialization. Silly example: let foo: [_; 4] = Array::generate(|i| i.to_string());

And, bikeshedding, I like Array for the trait.

@SimonSapin
Copy link
Contributor Author

I think that Clone and Default could be implemented with the generate method you describe, which in turn could be implemented as ManuallyDrop::new(mem::uninitialized::<Self>()) + initialization + ManuallyDrop::into_inner. (ManuallyDrop is required in case the initialization code panics, we don’t want to drop uninitialized items.)

@burdges
Copy link

burdges commented Feb 24, 2017

I suppose that could be done roughly as follows :

struct GenerateDropGuard<A> where A: FixedSizedArray {
    array: ManuallyDrop<A>, 
    index: usize
}

impl<A> Drop for GenerateDropGuard<A> where A: FixedSizedArray {
    drop(&mut self) {
        while self.array > 0 {
            self.index -= 1;
            unsafe { manually_drop(&mut self.array[self.index]); }
        }
    }
}

pub fn generate<A,F>(f: F) -> [A::Item; A::LENGTH] 
  where A: FixedSizedArray, F: Fn(usize) -> A::Item, A::Item::DropSafe
{
    let mut a = GenerateDropGuard {
        array: [unsafe { mem::uninitialized::<A::Item>() }; A::LENGTH],
        index: 0
    }
    while a.index < A::LENGTH { 
        unsafe { ptr::write(a[i], f(i)); }
        a.index += 1;
    }
    a.into_inner().array
}

@burdges
Copy link

burdges commented Feb 27, 2017

I found the line "doing this shouldn’t be necessary if we assume that Rust is going to have type-level integers .. eventually" kinda tricky. I think it's true because you can do type equality tests like Digest<Output=[T; n]> but one might need tricks like helper traits for some cases.

Also, I suspect he associated type Item and/or associated constant LENGTH might encourage more flexible APIs by encouraging traits to express real associated types as opposed to the parameters from which they construct those types. In other words, I'd expect that

trait Fun {
    type Output;
    ...
    fn run(&mut self) -> Output;
}

would usually be preferable to

trait Fun {
    const OutputSize : usize;
    ...
    fn run(&mut self) -> [u8; OutputSize];
}

even if you have type-level integers.

@aturon aturon self-assigned this Feb 28, 2017
@burdges
Copy link

burdges commented Mar 28, 2017

Right now, I suppose the cleanest way to get ::LENGTH is to use Unsize bounds everywhere, like

#![feature(unsize)]
use std::marker::Unsize;

fn array_length<T,A>() -> usize where A: Unsize<[T]> {
    ::std::mem::size_of::<A>() / ::std::mem::size_of::<T>()
}

@SimonSapin
Copy link
Contributor Author

I’ve toned down the wording of this RFC.

Also, #1931 has since been submitted and https://internals.rust-lang.org/t/lang-team-minutes-const-generics/5090 gives a very positive signal:

We believe that we can have an RFC accepted and const generics available on nightly by the end of 2017. 🎉

Since proper const generic now have a much clearer path forward than it seemed when I submitted this RFC, I hereby rescind it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs-api Relevant to the library API team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants