WIP: Incorporate variance into typeck

[edwardw]

Currently, rustc only does so for regions substitution. Applies the same
rule to type and self parameter substitution.

With all three substitutions on, the following becomes legitimate:

fn foo<'a, 'b>(x: &'b Option<&'a int>) -> Option<&'b int> {
    *x
}

x here is contravariant with respect to the region parameter b, so
it is eligible to the return position, also contravariant.

It is also found that rustc handled one special case incorrectly. In
variance inference pass, constraints are added and then solved. But rustc
didn't add any constraint for nullary generic enum and struct so they
were classified as bivariant, the default. Then the following two
implementations of Foo:

pub struct S<T>;

impl Foo for S<int> {
    fn bar(&self) {}
}

impl Foo for S<uint> {
    fn bar(&self) {}
}

would be identical therefor conflicting under type substitution, which
is incorrect.

Closes #3598.
Closes #5781.

[alexcrichton]

The travis errors look a little suspicious, but they may be spurious, just making sure that you managed to get past stage1 locally.

[edwardw]

It did. All the way to the run doc-crate-std and it failed there.

[alexcrichton]

Cool, thanks!

[nikomatsakis]

r? me -- @edwardw sorry for being incommunicado, I've been heads down on unrelated things and was planning to come back to your questions this week, but I see you've made great progress on your own.

[nikomatsakis]

I'm not sure about the changes to inference with the nullary type. I agree it's surprising, but completely consistent, that struct Foo<T> is a subtype regardless of T. That is, the current change feels a bit ad-hoc -- I feel like we should treat unused type or lifetime parameters the same regardless of whether a struct is nullary or not. The current behavior of bivariance is theoretically the most general but not the most practical. Perhaps for cases where the parameter is unreferenced we should simply default to what people likely: covariant for types and contravariant for lifetimes (i.e., as if you had a data member with the type T or a reference with the lifetime 'a). We might even be able to remove the marker types then, if we made Unsafe<T> hardcoded to be invariant with respect to T.

[edwardw]

The current patch makes no effort to discriminate nullary type from others; it simply add covariant constraint to type parameters for all types, used or unused. As for lifetime parameters, I'll try to add constraints to them, too.

[edwardw]

By adding contravariant constraint to all lifetime parameters, we make rust more rigorous. For example, json.rs#L318 is now rejected:

pub fn buffer_encode<T:Encodable<Encoder<'a>>>(to_encode_object: &T) -> ~[u8] {
   //Serialize the object in a string using a writer
    let mut m = MemWriter::new();
    {
        let mut encoder = Encoder::new(&mut m as &mut io::Writer); // `m` does not live long enough
        to_encode_object.encode(&mut encoder);
    }
    m.unwrap()
}

[nikomatsakis]

@edwardw Sorry, I'm confused. Why would we add a covariant constraint to all type parameters? And why would we add contravariant to all lifetimes? That seems clearly wrong. For example, a type like:

struct Foo<T> {
    x: fn(T)
}

Should be contravariant in T.

[edwardw]

As discussed in IRC, this is just another bug. The lifetime parameter and mutability in a trait reference are never used in variance inference. So the patch ends up always adding constraints for them which leads to noticeable behavior change.

[alexcrichton]

I was driving by this (just taking a look), and noticed that transmutes are popping up in what I thought was completely safe code. Is the current code unsound?

[edwardw]

I assume you asked about the last commit about librustdoc. Strangely enough, without the additional transmute, there is LLVM assertion failure.

[alexcrichton]

Not just that, but also the "Fix fallout for stage1" commit. Fixing an LLVM assertion failure with a transmute sounds... dubious

[edwardw]

Oh, that's because a bug in the variance inference that didn't take lifetime parameter and mutability of a trait reference into consideration, which is now fixed. If taking a close look, Encoder::new(&mut m as &mut io::Writer) requires a lifetime 'a, but m is just a local variable.

Yes, this is a behavior change and is open for discussion.

[alexcrichton]

I'm not entirely certain how much this relates to rust-lang/rfcs#12, but if this has semantic changes I think it should go through an RFC before merging.

I still don't understand why transmutes are popping up as fallout of this change, nor why the tests had to change. I'm not entirely certain what's going on here though, so if these are soundness changes than that's fine, but unfortunate.

[edwardw]

The commit that fixes ty_trait variance inference bug causes most of the noticeable behavior change. With that bug, the variance inference pass actually treats:

pub struct Encoder<'a> {
    priv wr: &'a mut io::Writer,
}

as if it were

pub struct Encoder<'a> {
    priv wr: & io::Writer,
}

In my opinion this is a soundness issue. I couldn't find a way to fix json.rs without using transmute though.

As for the variance RFC, it asks to add constraints for unused type parameters only. The ty_trait inference bug prevents us from distinguishing unused from used reliably. The commit #2 also implements actually adding variance constraints as the RFC asks. So yes, this PR is related to it.

The transmute introduced in librustdoc is a different story. After talking to @eddyb and @huon on IRC, it seems that somehow extern "C" fn gets expanded to extern "C" extern fn after this patch. It should be able to be addressed with a proper fix without transmute.

[nikomatsakis]

On Wed, Mar 19, 2014 at 01:20:13PM -0700, Edward Wang wrote:

Im my opinion this is a soundness issue.

Yes.

I couldn't find a way to fix json.rs without using transmute though.

Probably we will have to refactor json.rs. This happens sometimes.

Alex is definitely right though -- there should be no need for transmutes and so forth.

[edwardw]

I'd like to close this PR. It has changed scope several times. The ty_trait bug fixing part is blocked by a possible json.rs refactoring if not using transmute, while the incorporating variance into typeck part is tied to adding additional variance constraints.