fix: correct bounds of unsafe send and sync implementation

[fereidani]

Small fix for missing allocator trait bounds.

[alejandro-vaz]

ufff I missed that during code review

beta.1 is going to deploy with the old one

[alejandro-vaz]

I'm drafting this PR, I'm waiting until beta.1 is released to merge anything into v2

[alejandro-vaz]

it should be released within 48 hours hopefully

[alejandro-vaz]

thanks @fereidani, this was a great catch, we really appreciate it

[fereidani]

@alejandro-vaz I've found something critical, is there anyway we delay the release?

    #[inline]
    pub fn into_boxed_slice(self) -> Box<[T]> {
        self.into_vec().into_boxed_slice()
    }

and also

pub fn into_vec(self) -> Vec<T> {

Both are UBs.

the allocator API feels a bit rushed and I suspect could have more UBs, and I'm also not sure why it is a default feature.

[alejandro-vaz]

yeah the vec isn't using the allocator API

and now that I'm looking at the conversions, there are missing conversions as well

the allocator API is default feature because the goal was to make it allocator-first instead of relying on the magic alloc / dealloc / realloc, and if the feature is disabled then it uses the nightly allocator API

I already told josh to release it, and since this is a beta I don't think it's a big deal, we are getting a few thousand downloads per day max, and this is the first release with the allocator API

but once we have the next beta ready for release we'll yank it to be safe

the allocator API feels rushed because it was rushed, it was an initial implementation

I'm also thinking of having custom-allocator testing with full suite as well to catch this kind of stuff. I do think there may be more UB hiding

[fereidani]

the allocator API is default feature because the goal was to make it allocator-first instead of relying on the magic alloc / dealloc / realloc, and if the feature is disabled then it uses the nightly allocator API

So no default feature is nightly only?
I'm not sure it is a right choice to force users to pay for allocator api when they don't need it.

I already told josh to release it, and since this is a beta I don't think it's a big deal, we are getting a few thousand downloads per day max, and this is the first release with the allocator API
but once we have the next beta ready for release we'll yank it to be safe

That would be good.

I'm also thinking of having custom-allocator testing with full suite as well to catch this kind of stuff. I do think there may be more UB hiding

I will look into it, I also think we have some good refactoring chances reducing code lines. to be honest code feels a bit hard to maintain/review with allocator api added now.

I'm also thinking of having custom-allocator testing with full suite as well to catch this kind of stuff. I do think there may be more UB hiding

Yeah, we must have it. Both to test current and future contributions. It is really easy to miss these things, the allocator API feels like C programming to me. It feels like manual memory management.

I'm not sure if there is a tool for it or not. But we should have a 100% coverage test suite if we are taking soundness seriously.
I only done 100% coverage testing for C and not for any rust project, but I've heard good things about https://github.com/taiki-e/cargo-llvm-cov

[alejandro-vaz]

yeah as of now, no default features = nightly only. once the allocator API stabilizes then it will use the proper API when the feature is disabled

it's not about needing or not allocator API, it's just what is correct. in the end, not using the allocator API is just using allocator API with a global allocator

I will take a look at cargo-llvm-cov and see what we can do with it

[fereidani]

yeah as of now, no default features = nightly only. once the allocator API stabilizes then it will use the proper API when the feature is disabled

I'm not sure but it seems wrong to me, I know no other crate that fails to compile with no default feature which usually means less strict, You might want to get some other feedback about it from the community.

[bolshoytoster]

Sorry, I remember I put a TODO to figure out if I needed to add these bounds to A in the original PR, but it seems like I just forgot.

[bolshoytoster]

    #[inline]
    pub fn into_boxed_slice(self) -> Box<[T]> {
        self.into_vec().into_boxed_slice()
    }

and also

pub fn into_vec(self) -> Vec<T> {

Both are UBs.

Whoops, I thought I left all the stdlib-interacting functions only implemented for SmallVec<_, Global> (from_vec is correct).

[fereidani]

Sorry, I remember I put a TODO to figure out if I needed to add these bounds to A in the original PR, but it seems like I just forgot.

@bolshoytoster No problem at all, these mistakes happens, I was confused because on the other same sync/send impl block you have made the right bound(maybe you actually did your todo but forgot about the other sync/send impl block).