RFC 213: Implement Default Type Parameter Fallback

[jroesch]

This PR completes RFC 213 by allowing default type parameters to influence inference. This is almost certainly a breaking change due to interactions between default type parameters and the old fallback algorithm used for integral and floating point literals.

The error messages still require polish but I wanted to get early review and feedback from others on the the changes, error messages, and test cases. I also imagine we will want to run anywhere from 1-3 versions of this on crater and evaluate the impact, and it would be best to get that ball rolling.

The only outstanding issue I'm aware of is that type alias defaults don't work. It seems this may require significant restructuring, since during inference type aliases have already been expanded. @nikomatsakis might be able to provide some clarity here.

r? @nikomatsakis

cc @eddyb @gankro @aturon @brson

[aturon]

\o/

[arielb1]

Error message?

[jroesch]

I was working on improving it this afternoon/evening but got side tracked by trying to get type aliases to work correctly. Going to try to finish (and then add it here) it tomorrow morning PST.

[bluss]

Is it a breaking change for code involving default type parameters in struct declarations? I guess not. Defaults in impl blocks? Defaults in methods and functions' type parameters? (Yes, I guess it must be).

The rationale is that type parameter defaults never did anything really for methods and functions, so it's not breaking a previously working feature(?)

[bluss]

Default type parameters allow a form of static function parameters with defaults. Imagine fn graphemes<Kind: GraphemeKind = Extended>(&self) and then you can call it as .graphemes::<Legacy>() or just .graphemes() to get the default.

[Gankra]

Definitely would like a crater build of this change just to see what the impact would be. I don't expect much breakage in practice.

[nikomatsakis]

So, I'm concerned about this map.

For one thing, I'd want to "encapsulate it more":

a. make this private
b. key it by ty::TyVid
c. insert new data by adding an (optional) parameter to next_ty_var, rather than mutating the table after the fact

But a bigger issue is that it should be versioned for transactions. As it is, we are going to create fresh type variables with defaults then (potentially) roll them back, which might clear the default. I think my preference would be to remove the map, and instead store the Option<Ty<'tcx>> as part of the Bounded variant in type_variable::TypeVariableValue (after all, who cares what the default was once the type is Known?)

I'd probably change TypeVariableValue to something like:

enum TypeVariableValue<'tcx> {
    Known(Ty<'tcx>),
    Bounded { relations: Vec<Relation>, default: Option<Ty<'tcx>> }
}

(Also, if you do it this way, the transactional problem should just take care of itself.)

[jroesch]

Did this refactor, testing now.

[nikomatsakis]

More tests that would be nice:

• Test that i32 and () fallback takes precedence.
• Tests where the defaults reference once another.
• Try to pin @gankro down and have him give you one of the collections patterns he would like to see working.

[Gankra]

CC @apasel422 and @reem for crazy uses of defaults

[bors]

☔ The latest upstream changes (presumably #26895) made this pull request unmergeable. Please resolve the merge conflicts.

[nikomatsakis]

we still are not substituting here, right? I am guessing you'll have problems related to references to other variables, as well as maybe Self...?

[brson]

Crater says: https://gist.github.com/brson/ce88d850e673ea2c2d05

Two stack overflows.

[nikomatsakis]

@bors r+

[bors]

📌 Commit 5ad36cb has been approved by nikomatsakis

[bors]

⌛ Testing commit 5ad36cb with merge a5c12f4...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

[petrochenkov]

Hm, after this change I still can't implement heterogeneous comparisons for Option without breakage.

Reduced case:

// Almost PartialEq
trait PartialQe<Rhs = Self> {
    fn qe(&self, other: &Rhs) {}
}

impl<A, B> PartialQe<Option<B>> for Option<A> {}

fn main() {
    // `None` is not inferred to be `None::<&str>` based on the default `Rhs = Self`
    PartialQe::qe(&Some("str"), &None); // error: unable to infer enough type information about `_`; type annotations or generic parameter binding required [E0282]
}

[eddyb]

@petrochenkov Maybe the impl also needs the default?

[petrochenkov]

@eddyb
impl<A, B = A> PartialQe<Option<B>> for Option<A> {} or impl<B, A = B> PartialQe<Option<B>> for Option<A> {} don't seem to work either.

[Gankra]

💓 😻 😍 💖 ✨

[jroesch]

This program works on the compiler built from my branch:

// Almost PartialEq
trait PartialQe<Rhs = Self> {
        fn qe(&self, other: &Rhs) {}
}

impl<A, B = A> PartialQe<Option<B>> for Option<A> {}

fn main() {
        PartialQe::qe(&Some("str"), &None); 
        Some('a').qe(&None);
}

[petrochenkov]

Now I see, it works only when the feature gate is enabled.

#![feature(default_type_parameter_fallback)]

So, it can't yet be used to generalize standard library facilities, like Option, because users on stable won't be able to unbreak their code by enabling the feature gate.

[jroesch]

@petrochenkov we are going to let it bake for a cycle in order to give us time to tweak the algorithm, then it will be stabilized.