rust-lang / wg-allocators

Home of the Allocators working group: Paving a path for a standard set of allocator traits to be used in collections!
http://bit.ly/hello-wg-allocators
207 stars 9 forks source link

Unstable type parameters on stable types #2

Closed SimonSapin closed 3 years ago

SimonSapin commented 5 years ago

This is related to #1 but not identical. When adding a defaulted type parameter to a stable type, for example changing:

struct Vec<T> {…}

To:

struct Vec<T, A: Alloc = Global> {…}

We would like to experiment with this change on Rust Nightly before it affects users one the Stable channel. That is, we would like to keep the type parameter unstable. Ideally, even the existence of this type parameter should not affect the stable channel at all.

As far as I understand this is not possible today. Ways forward include:

TimDiekmann commented 5 years ago

As far as I understand this is not possible today.

It is possible:

struct Vec<
    T, 
    #[unstable(feature = "allocator_api", issue = "32838")] A: Alloc = Global
> {…}
glandium commented 5 years ago

I tested this for Box<T, A> just right now, and it doesn't actually work:

use std::alloc::System;

fn main() {
    let x = Box::<(), System>::default();
}

Just compiles without an error.

So either this needs to be fixed first, or we have to limit the changes to only expose unstable methods, which would make it less useful.

TimDiekmann commented 5 years ago

Oh damn, I think I tested it with Box::<(), Global> and forgot that Global itself isn't stable either... I'm sorry for the confusion...

TimDiekmann commented 5 years ago

A fourth option would be another Alloc trait, which is unstable and will only be implemented for unstable types. This will then be replaced as soon as either 1. becomes possible or when allocator_api becomes stable. With this we could also test other options for the alloc trait without breaking the current allocator api every time.

glandium commented 5 years ago

Taking a step back, the only stable type that implements Alloc is System. There is nothing that says that System should implement Alloc. The only thing currently documented is that it implements GlobalAlloc. And Global is what implements Alloc, and, practically speaking, it wraps System. So it doesn't seem to actually be useful for System to implement Alloc.

So how about we remove the Alloc impl for System?

TimDiekmann commented 5 years ago

Global uses the #[global_allocator] and System the system allocator but this doesn't matter here. Removing Alloc for System is maybe the most simple option. The implementation cannot be used on stable anyway.

On May 3, 2019, 12:41, at 12:41, Mike Hommey notifications@github.com wrote:

Taking a step back, the only stable type that implements Alloc is System. There is nothing that says that System should implement Alloc. The only thing currently documented is that it implements GlobalAlloc. And Global is what implements Alloc, and, practically speaking, it wraps System. So it doesn't seem to actually be useful for System to implement Alloc.

So how about we remove the Alloc impl for System?

-- You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/rust-lang/wg-allocators/issues/2#issuecomment-489055454

Ericson2314 commented 5 years ago

I think unstable params on stable types is good thing to have in general: I think a lot of Rust code ought be more polymorphic (in ways we haven't even thought of yet!) and this is the best way to unblock experimentation on that front.

Now I don't want to hold up Box<T, A> much longer, either, so how hard would this be?

TimDiekmann commented 5 years ago

Now I don't want to hold up Box<T, A> much longer, either, so how hard would this be?

With https://github.com/glandium/rust/commit/4395ce1744173d406a25efb71eee629187d86e1a this issue should be solved. @SimonSapin?

I think unstable params on stable types is good thing to have in general

Absolutely! Probably an issue should be filed in rust repo, if there isn't one yet.

SimonSapin commented 5 years ago

@TimDiekmann I assume you mean that this commit “solves” this issue on the premise that since there is a bound on the type parameter with an unstable trait that has no impl with stable types, then users on Stable cannot write Box<T, Foo> for any Foo.

Does this satisfy the criteria that the parameter addition has no observable effect for users on Stable? I’m not sure. For example, what about Box<T, _>? (With a literal underscore.)

TimDiekmann commented 5 years ago

Oh, the literal underscore is a good point. But, as far as I can see, this case should be safe. There will definitely a second parameter but a user on Stable cannot make any assumptions about it, as he cannot implement the trait. (Even typing Box<T, System> would be relatively safe, but Alloc should be removed from System anyway. If a nightly user really needs this, he may implement it in a newtype struct). With Box<T, _> it would even be possible to change the second parameter bound or default without breaking anything.

SimonSapin commented 5 years ago

Just to be clear, you’re arguing that the visible effects of this type parameter on Stable are benign enough that we should accept them as insta-stable. This is not the same as there being no such effect. The former would require some @rust-lang/libs buy-in before landing at all.

Ericson2314 commented 5 years ago

Yeah I want something that's seamless enough that teams / WGs don't need to synchronize, so this and other parameter experiments can take place entirely in parallel.

TimDiekmann commented 5 years ago

So what's the prefered way to go?

gnzlbg commented 5 years ago

Extend the stability mechanism in the language to support unstable type parameters. This itself would likely take significant time to design and implement.

I think we should at least try to do this, and only if this proves to be too hard, then evaluate other options. I also think extending the stability mechanism to this won't be trivial, but I like to know more about how hard it actually is.

SimonSapin commented 5 years ago

Anyone wants to volunteer to go lobby the language and compiler teams for adding unstable type parameter to the language? I’m not sure a formal RFC would needed since it would not be part of the stable language.

gnzlbg commented 5 years ago

I've just asked them in Zulip if it would be possible for them to let us know whether this could be feasible for a sufficiently interested party to implement, or whether given the current implementation, this is something that just cannot happen in the near future:

https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/Unstable.20type.20parameters.20on.20stable.20types

I think having this information would already be useful to know what to do next (e.g. whether we should try implementing this ourself, get somebody else to prioritize this, or whether this cannot be done at all and just consider this a constraint that we have to satisfy).

phil-opp commented 5 years ago

What's the status of this? From the Zulip thread it seems like it's feasable. Is anyone working on it?

gnzlbg commented 5 years ago

Somebody would need to implement them in nightly. I recommend asking @varkor for guidance.

Avi-D-coder commented 4 years ago

I would like to implement this, but I did not see anything in the rustc-guide about how the stability system is implemented?

@varkor I'm going to need some guidance.

varkor commented 4 years ago

@Avi-D-coder: the place to start is https://github.com/rust-lang/rust/blob/master/src/librustc/middle/stability.rs. It's already possible to provide annotations for generic parameters, so you just need to integrate the stability checking.

Let me know if you need any more specific details on any of these steps, or if you have any questions (either here, or on Discord or Zulip).

TimDiekmann commented 4 years ago

@Avi-D-coder Were you already able to make progress?

Avi-D-coder commented 4 years ago

@TimDiekmann There is an error that I found, that I still have to get around to fixing, but it is mostly done.

Edit: the bug is determined not to get squashed.

Ericson2314 commented 4 years ago

If anything proves harder to debug, remember you can always make a WIP PR; then debugging and review can happen in parallel.

TimDiekmann commented 4 years ago

you can always make a WIP PR; then debugging and review can happen in parallel.

A WIP PR has another advantage: When queued for merging the earlier the PR was submitted, the higher the priority of the PR :slightly_smiling_face:

Avi-D-coder commented 4 years ago

I gave up and sent a WIP PR.

Avi-D-coder commented 4 years ago

It's already possible to provide annotations for generic parameters, so you just need to integrate the stability checking.

@varkor Are you sure? The attributes did not show up (test result).

#![feature(staged_api)]
#![stable(feature = "stable_generic_feature", since = "1.40.0")]

#[stable(feature = "stable_generic_feature", since = "1.40.0")]
pub struct Foo<#[unstable(feature = "unstable_generic_feature", issue = "0")] T = usize> {
    _f: T,
}

I would think the first line of auxiliary/stability_attribute_generic.rs would enable the staged API for auxiliary/stability_attribute_generic.rs, but in the test the staged_api is false.

// aux-build:stability_attribute_generic.rs

extern crate stability_attribute_generic;
use stability_attribute_generic::*;

fn new_foo(f: Foo) {}

fn new_foo_t<T>(f: Foo<T>) {}

fn new_foo_str(f: Foo<&str>) {}

fn main() {}
varkor commented 4 years ago

They're supposed to work: https://github.com/rust-lang/rust/issues/48848 Try seeing whether #[may_dangle] shows up in your logs. It's possible the code paths with attributes on generic parameters are not well-tested, because there's only one valid attribute so far, so there may be a bug there somewhere.

Avi-D-coder commented 4 years ago

@varkor #[may_dangle] does not show up either.

HirId { owner: DefIndex(6), local_id: 5 }
annotate_generic
NOT staged_api annotate(id = HirId { owner: DefIndex(6), local_id: 5 }, attrs = [])
UNMARKED: DefId(15:4 ~ stability_attribute_generic[8787]::Foo[0]::T[0])
UNMARKED: DefId(15:4 ~ stability_attribute_generic[8787]::Foo[0]::T[0])
UNMARKED: DefId(15:4 ~ stability_attribute_generic[8787]::Foo[0]::T[0])
UNMARKED: DefId(15:4 ~ stability_attribute_generic[8787]::Foo[0]::T[0])
SimonSapin commented 4 years ago

Another aspect to consider is how adding a defaulted type parameter affects inference: https://github.com/rust-lang/rust/issues/50822#issuecomment-389758598

TimDiekmann commented 4 years ago

This issue is now exactly one year old.

Extend the stability mechanism in the language to support unstable type parameters.

This pretty much failed. I don't think we should focus on this anymore.

I'd say we do two things in parallel to push this further:

  1. a crater-run on an MVP with an allocator aware Box to solve #1
  2. implement another Box (maybe in alloc::alloc::boxed) gated behind a feature flag and test around with this box. The new box should be compatible with the old box, so at a later point it could replace it.

I'm pretty sure new issues will occure as soon as we push the box upstream.

Avi-D-coder commented 4 years ago

I'm still interested in https://github.com/rust-lang/rust/pull/65083, but I would need guidance on blocking type inference.

TimDiekmann commented 4 years ago

That's nice! However the crater run is needed anyway and using a new type could be a temporary solution to get things done.

On May 4, 2020, 04:37, at 04:37, Avi Dessauer notifications@github.com wrote:

I'm still interested in https://github.com/rust-lang/rust/pull/65083, but I would need guidance on blocking type inference.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/rust-lang/wg-allocators/issues/2#issuecomment-623232350

Amanieu commented 4 years ago

If we can't get unstable type parameters then we will just have to introduce the allocator parameter as insta-stable, like a trait impl.

TimDiekmann commented 4 years ago

https://github.com/rust-lang/rust/pull/58457 would have removed AllocRef for System. This is also an option to prevent users from specifying an allocator. However this does not prevent writing

let boxed: Box<T, _> = Box::new(...)

But this is fine to me.

varkor commented 4 years ago

I think https://github.com/rust-lang/rust/pull/72314 is ready to merge now, which adds support for unstable type parameters. That is, type parameters with defaults can be marked unstable, which means an explicit type argument will only be permitted if the corresponding feature flag is enabled. However, as a note of warning to API implementers: a non-default type can still be inferred for an unstable type parameter. (This appears to be necessary to prevent code breaking when a type parameter is stabilised.) That means that you must be careful not to allow the type parameter to be inferred outside of library code, e.g. by having custom constructors for the non-default type parameter, at least until the type parameter is stabilised. I think that this is the most reasonable design, but let me know if you have any concerns. It may be helpful for those interested to take a look at the tests on https://github.com/rust-lang/rust/pull/72314 to check it matches their intuition. If there are no concerns, I'll approve it in a few days.

Wodann commented 4 years ago

That seems like a reasonable trade-off between reviewer responsibility and compiler detectability to me. Nice work @Avi-D-coder and @varkor!

SimonSapin commented 4 years ago

you must be careful not to allow the type parameter to be inferred outside of library code, e.g. by having custom constructors for the non-default type parameter

I don’t understand this part, could you say more?

TimDiekmann commented 4 years ago

I don’t understand this part, could you say more?

If I understand correctly, this means, that a stable API must not make any assumption about the unstable type parameter. For example we must not stabilize Vec::new_in before stabilizing the type parameter. If we'd do this, we may pass an allocator without a feature gate and alter the defaulted type parameter.

varkor commented 4 years ago

Now that I think about it, it seems harder to accidentally allow this than I thought. The sort of example I had in mind was the following.

// Library code.
pub struct SomeType<#[unstable(feature = "test_feature")] T = ()>(T);

// User code.
SomeType(0u8) // Works, even though a non-default is not specified, because the default is inferred.

There could be a similar problem if existing methods are extended to support different type parameters, but that's not going to happen, in which case @TimDiekmann's comment is the main point to keep in mind: if you stabilise any method that allows the default to be changed (even implicitly, e.g. taking a type that can be inferred as non-default), you've effectively stabilised the type parameter itself (even if the user can't mention it explicitly). I think due to the way the APIs are designed, this won't be a problem in practice, though.

TimDiekmann commented 3 years ago

It's now possible to add stability annotations on generic parameters as seen in https://github.com/rust-lang/rust/pull/77187. Many thanks for @Avi-D-coder to make this possible. Also thank you for @varkor for reviewing the changes and @exrook to catch this up, so it's merged now!