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

Type inference breakage on beta #41677

Closed
Diggsey opened this issue May 1, 2017 · 3 comments
Closed

Type inference breakage on beta #41677

Diggsey opened this issue May 1, 2017 · 3 comments
Assignees
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Diggsey
Copy link
Contributor

Diggsey commented May 1, 2017

This code compiles on stable, but fails on beta (playground link):

#![allow(unused_variables, dead_code)]

use std::marker::PhantomData;

trait Handle {
    type Inner;
}

struct ResizingHandle<H>(PhantomData<H>);
impl<H> Handle for ResizingHandle<H> {
    type Inner = H;
}

struct Sender<T, H: Handle<Inner=T>>(PhantomData<H>);
struct Receiver<T, H: Handle<Inner=T>>(PhantomData<H>);

type ResizingSender<T> = Sender<T, ResizingHandle<T>>;
type ResizingReceiver<T> = Receiver<T, ResizingHandle<T>>;

impl<T, H: Handle<Inner=T>> Receiver<T, H> {
    fn new() -> Self {
        Receiver(PhantomData)
    }
}

impl<T, H: Handle<Inner=T>> Sender<T, H> {
    fn new(receiver: &Receiver<T, H>) -> Self {
        Sender(PhantomData)
    }
}

fn channel<T>(size: usize) -> (ResizingSender<T>, ResizingReceiver<T>) {
    let rx = ResizingReceiver::new();
    let tx = ResizingSender::new(&rx);
    (tx, rx)
}


fn main() {
}

Error message:

error[E0282]: type annotations needed
  --> <anon>:33:9
   |
33 |     let rx = ResizingReceiver::new();
   |         ^^
   |         |
   |         consider giving `rx` a type
   |         cannot infer type for `Receiver<_, ResizingHandle<T>>`
@sfackler sfackler added the regression-from-stable-to-beta Performance or correctness regression from stable to beta. label May 1, 2017
@arielb1
Copy link
Contributor

arielb1 commented May 1, 2017

minified:

#![allow(unused_variables, dead_code)]

use std::marker::PhantomData;

trait Handle {
    type Inner;
}

struct ResizingHandle<H>(PhantomData<H>);
impl<H> Handle for ResizingHandle<H> {
    type Inner = H;
}

struct Receiver<T, H: Handle<Inner=T>>(PhantomData<H>);

fn channel<T>(size: usize) -> Receiver<T, ResizingHandle<T>> {
    let rx = Receiver(PhantomData);
    rx
}

fn main() {
}

@nikomatsakis nikomatsakis added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 1, 2017
@nikomatsakis
Copy link
Contributor

triage: P-high

@rust-highfive rust-highfive added the P-high High priority label May 1, 2017
@nikomatsakis nikomatsakis self-assigned this May 1, 2017
@nikomatsakis
Copy link
Contributor

So, I have a branch which fixes this by adding WF(T) constraints to the generalized type T whenever it includes an unconstrained type variable in a bivariant context. Currently testing now. Here is my comment from the code explaining why I think this makes sense (Generalization<'tcx> is the new result from the generalization code):

/// Result from a generalization operation. This includes
/// not only the generalized type, but also a bool flag
/// indicating whether further WF checks are needed.q
struct Generalization<'tcx> {
    ty: Ty<'tcx>,

    /// If true, then the generalized type may not be well-formed,
    /// even if the source type is well-formed, so we should add an
    /// additional check to enforce that it is. This arises in
    /// particular around 'bivariant' type parameters that are only
    /// constrained by a where-clause. As an example, imagine a type:
    ///
    ///     struct Foo<A, B> where A: Iterator<Item=B> {
    ///         data: A
    ///     }
    ///
    /// here, `A` will be covariant, but `B` is
    /// unconstrained. However, whatever it is, for `Foo` to be WF, it
    /// must be equal to `A::Item`. If we have an input `Foo<?A, ?B>`,
    /// then after generalization we will wind up with a type like
    /// `Foo<?C, ?D>`. When we enforce that `Foo<?A, ?B> <: Foo<?C,
    /// ?D>` (or `>:`), we will wind up with the requirement that `?A
    /// <: ?C`, but no particular relationship between `?B` and `?D`
    /// (after all, we do not know the variance of the normalized form
    /// of `A::Item` with respect to `A`). If we do nothing else, this
    /// may mean that `?D` goes unconstrained (as in #41677).  So, in
    /// this scenario where we create a new type variable in a
    /// bivariant context, we set the `needs_wf` flag to true. This
    /// will force the calling code to check that `WF(Foo<?C, ?D>)`
    /// holds, which in turn implies that `?C::Item == ?D`. So once
    /// `?C` is constrained, that should suffice to restrict `?D`.
    needs_wf: bool,
}

@arielb1 I opted for this strategy instead of adding explicit WF requirements to local variables because I was concerned about "intermediate" types that might wind up unconstrained. It seems like the code should be able to expect that, whenever we create a type variable, that type variable will be inferred to something WF as long as it is only related to WF types.

However, as I write this comment, I suppose that I can't think of a place where some intermediate type might appear that would be unconstrained. e.g., I could imagine types that get "autoderef'd" away before they hit a local variable, but those would have to be custom deref impls, and I suppose that we would place WF requirements at the point where we call deref anyway. So I could also try adding WF constraints to locals.

Still, this approach seems maybe a mite more targeted? (I'm not really sure how often we generalize types that have type variables in bivariant positions, but it seems like it would likely be less often than we declare local variables.)

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue May 12, 2017
enforce WF conditions after generalizing

Add a `WF(T')` obligation after generalizing `T` to `T'`, if `T'` contains an unconstrained type variable in a bivariant context.

Fixes rust-lang#41677.

Beta nominating -- regression.

r? @arielb1
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue May 12, 2017
enforce WF conditions after generalizing

Add a `WF(T')` obligation after generalizing `T` to `T'`, if `T'` contains an unconstrained type variable in a bivariant context.

Fixes rust-lang#41677.

Beta nominating -- regression.

r? @arielb1
bors added a commit that referenced this issue May 12, 2017
enforce WF conditions after generalizing

Add a `WF(T')` obligation after generalizing `T` to `T'`, if `T'` contains an unconstrained type variable in a bivariant context.

Fixes #41677.

Beta nominating -- regression.

r? @arielb1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants