[alejandro-vaz]

this PR revamps the allocator features

feature hierarchy

1. no features = custom allocator implemented
2. allocator-api = requires nightly, uses the API
3. allocator-api2 = pulls the API from crates.io

the allocator-api2 implies allocator-api, to make sure no weird case of both together when they should be disjoint exists

exported types

each of the three modes internally exports three types: Global, Vec and Box

which are then used in the rest of the API

that means that all conversions only happen to and from those types, not others

this is relevant to #649, though allocator-handling is a bit fucked up in the base case, so I'm not sure if it'd be easy to handle it there. and it unblocks #644

closes #655

closes #631

note: there is a regression in from_elem because I couldn't use the vec macro anymore without doing macro wizardry, so it now uses iterators again

[alejandro-vaz]

well I think it makes more sense to invert the dependency dependencies of the two allocator ones and have the allocator-api2 as the default

[alejandro-vaz]

ah no, can't do that because the nightly one then will randomly download allocator-api2 which is pointless

[alejandro-vaz]

I've disabled one fuzzing operation because it magically failed to compile

we have to remake fuzzing anyway so I'm not worried, see #657

[alejandro-vaz]

I will fix the fuzzing stuff once #658 lands

[bolshoytoster]

note: there is a regression in from_elem because I couldn't use the vec macro anymore without doing macro wizardry, so it now uses iterators again

I've been able to get it to work by just exporting the macro from the allocator/ modules. Although I have to add #[rustfmt::skip] to stop it from merging

pub use alloc::{
    vec::Vec,
    vec
};

into

pub use alloc::{
    vec::{
        self,
        Vec
    }
};

[alejandro-vaz]

yeah that's exactly what I didn't want to do

do we have any other alternative??

maybe

#[rustfmt::skip]
pub use alloc::vec;

[bolshoytoster]

Yeah I think that should work too.

[bolshoytoster]

Does this PR mean you can only From/Into the Vec exposed by allocator?

Given that the default would be allocator-api2::vec::Vec, would it be nice to at least make sure SmallVec<T, N, Global> from/into alloc::vec::Vec<T, Global> are supported?

[alejandro-vaz]

that's a good point

maybe by default we should disable all features and that's it honestly

very few people use custom allocators yet anyway

it'd also make CI simpler

[pedrodesu]

I've disabled one fuzzing operation because it magically failed to compile

we have to remake fuzzing anyway so I'm not worried, see #657

What did it complain about?

[alejandro-vaz]

actually I think it'll work now

[pedrodesu]

If we bring no features and want to use allocator-specific functions (such as SmallVec::new_in), what are we supposed to pass in the allocator parameter? I see that Global is exported out of the module allocator but allocator isn't publicly exported via lib.rs. Am I missing something?

Additionally, if we're under allocator-api2, we seem to have a problem: If the version of the consumer's allocator-api2 doesn't match the one we use internally (which is quite likely, even more so if the consumer is simply using an allocator that may as well integrate an arbitrary version of allocator-api2 (such as bumpalo)), the allocator API won't work at all because of a dependency version mismatch.

Closure on this is instrumental to implement #669 properly. As of now, it seems that I can only write tests that reliably compile for the nightly allocator_api case.

@alejandro-vaz

[alejandro-vaz]

yeah Global is not exported at crate-level and can only be passed to functions requiring an allocator when Global is not the export from alloc/allocator-api2

version mismatch with allocator-api2 is a concern, but do we have any better alternative?? I don't think so. why would it matter when implementing an allocator-api2 testing suite??

[pedrodesu]

This didn't answer my question - If I'm going to write a testing suite for custom allocators, surely they must pass with any and all flags, right? And so, they should adapt to each situation; As we should do anyway, independently of having this testing suite, as whatever can't compile in it also won't compile in real user code that attempts to use the same code and combination of features. As of now, it seems currently impossible for me to write custom allocator tests in the default case, with neither allocator-api and allocator-api2 enabled (as we use an internal struct that can't be accessed with consumer code), which means, thus, that a consumer will not be able to write code with custom allocators with the default flags. Additionally, as stated before, I can only write these tests with allocator-api2 if my test code uses the same allocator-api2 crate that the smallvec library does (which it does, of course. However, and this is outside of the scope of the testing, if a consumer attempts to use the allocator-api2 flag with a mismatched version in their project, whether it is because they want to do so or because they're using an allocator crate that internally uses another version of allocator-api2, they won't be able to use our allocator-api2 correctly either).

tl;dr - I can write tests that work with both allocator-api and (coincidentally) allocator-api2. I can't make the tests work if neither flag is present though. Additionally and perhaps more importantly, allocator-api2's use is fragile and difficult in practice and apparently the default combination of flags is impossible to use (with custom allocators).

I'm not sure on what would be the best solution but I believe this definitely shouldn't be as it is, as this means a) consumers might reach code that they can't use and b) I can't make a test case that will compile properly under all three circumstances. There's two different questions to tackle here. Perhaps a new issue or at least further, more detailed discussion in #669 is warranted.

[alejandro-vaz]

yeah the API is made so that without any features enabled, you can't use any allocator features. there's nothing to use anyway, it's just a dummy Global struct, because we are not going to export our custom Allocator trait that is auto-implemented

yes, with neither of the allocator features enabled I think the desired behavior should be that users can't simply touch allocator-related stuff. like it currently happens in Vec. because when no features are enabled, Global doesn't really exist, it is just something we made up ourselves for internal code consistency

I know about version sensitivity in allocator-api2, there's not much we can do, and implementing all minor versions of allocator-api2 is too much work for too little benefit

I don't think the custom allocator testing suite should work when you can't literally access allocators, so maybe cfg it

[bolshoytoster]

To compare with hashbrown, they don't seem to include the allocator API in any tests, aside from some doctests where they explicitly add a #[cfg(feature = "nightly")].

[pedrodesu]

yeah the API is made so that without any features enabled, you can't use any allocator features. there's nothing to use anyway, it's just a dummy Global struct, because we are not going to export our custom Allocator trait that is auto-implemented

yes, with neither of the allocator features enabled I think the desired behavior should be that users can't simply touch allocator-related stuff. like it currently happens in Vec. because when no features are enabled, Global doesn't really exist, it is just something we made up ourselves for internal code consistency

I can understand this, but then we just export and expose custom allocator functions that are impossible to use for the default use case? Is this what we want? Not that it's a no-op or that it panics or something similar, but that it's quite literally impossible to compile? Would it be possible to simply hide/not export these functions via a cfg or something?

I know about version sensitivity in allocator-api2, there's not much we can do, and implementing all minor versions of allocator-api2 is too much work for too little benefit

I suppose so. I think this becomes a losing battle when we pick a given version of allocator-api2 and integrate it in our API, and not the other way around. But I don't see how we could do 'the other way around'.

I don't think the custom allocator testing suite should work when you can't literally access allocators, so maybe cfg it

This seems like a possibility. This would perhaps solve question b).

To compare with hashbrown, they don't seem to include the allocator API in any tests, aside from some doctests where they explicitly add a #[cfg(feature = "nightly")].

In other words they don't handle at all custom allocator tests? This makes me wonder on whether we should in the first place. At least in this point in time.

[pedrodesu]

Would it be possible to simply hide/not export these functions via a cfg or something?

I understand that it would be much more difficult than that since the Global variants are just generally wrapping the custom allocator ones which makes this a headache. Still, I wonder whether this would be a possibility; And, more importantly perhaps, whether it's worth it.

Perhaps the way right now is to simply let the compile errors show up and only write these tests for the case of either of the two features? Or not write these tests at all as of now (@bolshoytoster's note made me wonder this)? What do you think?

[alejandro-vaz]

Would it be possible to simply hide/not export these functions via a cfg or something?

I like that idea

I suppose so. I think this becomes a losing battle when we pick a given version of allocator-api2 and integrate it in our API, and not the other way around. But I don't see how we could do 'the other way around'.

I don't think we can, so we can just make the educated guess that the downstream crate uses the latest version of allocator-api2

Perhaps the way right now is to simply let the compile errors show up and only write these tests for the case of either of the two features? Or not write these tests at all as of now (@bolshoytoster's note made me wonder this)? What do you think?

I think we should anyway, I think there's probably UB / failures around there. not testing it is not an option for us

what we can also do is choose to export Allocator and Global under internals, which we should do anyway probably

[alejandro-vaz]

wait if we export Allocator and Global under internals I don't think we should then hide the functions

we should only hide if no allocator feature is enabled and internals is disabled