Unstable type parameters on stable types

[SimonSapin]

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:

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

• Accept that the existence of the new type parameter is instantly stable as soon as it lands, and hope that the trait that constraints this parameter being unstable is enough to avoid locking is into some API detail that we might want to change later. In this case Compat of adding parameters to stable types #1 needs to be resolved before adding the parameter.

• Avoid adding type parameters at all to stable types. Define new types instead. This would mean that e.g. &mut NewVec<T, Global> could not be passed where &mut Vec<T> is expected.

[glandium]

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]

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]

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]

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]

`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 ***@***.***> 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: #2 (comment)

[Ericson2314]

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]

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

With glandium/rust@4395ce1 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]

@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]

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]

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]

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]

So what's the prefered way to go?

• extend the stability mechanism to support unstable type parameters (OP)
  → long term goal, but it's not suitable right now, as it would take too long.
• remove the Alloc impl for System (Unstable type parameters on stable types #2 (comment))
  → requires some @rust-lang/libs buy-in before landing changes (Unstable type parameters on stable types #2 (comment)).
• define new types instead until the first point is resolved (OP)
  → API not compatible with other old types
• Use WG-repo until the first point is resolved (Reimplement the most important structs and traits for testing #19)
  → Not all features available as in rust-lang/rust

[gnzlbg]

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]

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]

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]

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

[gnzlbg]

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

[Avi-D-coder]

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]

@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.

• You'll need to visit_generic_param in the Annotator impl and annotate type and const parameters if they have defaults:
  https://github.com/rust-lang/rust/blob/60895fd1f9ad24ee41bb3dac67fc2c5f127ecdb4/src/librustc/middle/stability.rs#L238
  Lifetimes and types/consts without defaults, that have been annotated with #[unstable(...)] should throw errors.
• You ought not need to touch MissingStabilityAnnotations, because we're going to assume that generic parameters without stability attributes have always been stable. However, if you get encountered unmarked API errors, you'll need to explicitly mark them as stable in visit_generic_param in the previous bullet.
• Because every unstable type parameter must have a default, we shouldn't need to modify the checks (https://github.com/rust-lang/rust/blob/60895fd1f9ad24ee41bb3dac67fc2c5f127ecdb4/src/librustc_typeck/astconv.rs#L278-L280) that enough arguments have been provided: we just have to make sure that a user cannot provide a default unless they've opted into the feature gate.
• To do the stability checking, we'll need to visit generic arguments and, for each generic type/const argument, check if has a stability attribute and if it does, error if the user hasn't opted in. We'll do this by calling check_stability (from stability.rs) appropriately. I think the best place for this probably would be in check_generic_arg_count in astconv.rs, which is where generic arguments get checked against the parameters.
• I think this should be sufficient to get things working, so then you'll need to add some tests (there are plenty of stability-* tests in src/tests/ui already to compare to) to make sure this is working as intended.
• After that, we just need to make sure rustdoc understands the stability markers, though we can probably leave that for a follow up.

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]

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

[Avi-D-coder]

@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]

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

[TimDiekmann]

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 🙂

[Avi-D-coder]

I gave up and sent a WIP PR.

[Avi-D-coder]

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]

They're supposed to work: rust-lang/rust#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]

@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]

Another aspect to consider is how adding a defaulted type parameter affects inference: rust-lang/rust#50822 (comment)

[TimDiekmann]

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 Compat of adding parameters to stable types #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]

I'm still interested in rust-lang/rust#65083, but I would need guidance on blocking type inference.

[TimDiekmann]

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 ***@***.***> wrote: I'm still interested in rust-lang/rust#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: #2 (comment)

[Amanieu]

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]

rust-lang/rust#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]

I think rust-lang/rust#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 rust-lang/rust#72314 to check it matches their intuition. If there are no concerns, I'll approve it in a few days.

[Wodann]

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

[SimonSapin]

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]

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]

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]

It's now possible to add stability annotations on generic parameters as seen in rust-lang/rust#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!