Self as default type isnt typechecked

[DutchGhost]

The following code compiles:

struct B<P: Sized = [Self]>(P);

but shouldn't, because [Self] is NOT Sized.

If we change [Self] with a [u8], like this:

struct B<P: Sized = [u8]>(P);

it fails to compile, because [u8] isnt sized.

Also note that if we would write impl B {} in the case of P being of default type [Self], we get an ICE:

Related to #59956 (comment)

[DutchGhost]

Using godbolt, this slipped in with 1.32, where Self is allowed in type defs: #56366

[Centril]

cc @alexreg

[alexreg]

Thanks for the report.

@Centril I think the straightforward solution is to ban mentions of Self as type parameter defaults. After all, if you replace Self with B in the above example, you get:

error[E0391]: cycle detected when processing `B::P`

Sound fair?

[Centril]

That's a breaking change; consider e.g.:

enum L<P = Self> {
    N,
    C(u8, Box<P>),
}

fn main() {
    L::C(0, Box::new(L::<()>::N));
}

I think the right thing to do is to account for Self when checking the bounds of a type definition.

[alexreg]

Oh, I didn't realise Self was previously permitted...

[Centril]

This appears to be an instance of a wider problem:

trait Foo<Bar: Sized = [Self]> {}

Has compiled, but shouldn't have (?), since 1.0.

cc @nikomatsakis

[alexreg]

Thoughts after discussing this with @Centril: maybe we should be allowing this after all (or have no other choice due to the way the type system works), and should instead put in checks on things like imp B { ... } (with no binding for P) and possibly elsewhere.

[pnkfelix]

triage: P-high. Leaving nominated for discussion at rustc meeting. Leaving unassigned for now.

[DutchGhost]

I did some more playing around, and it's not just about the Self type:

The following code compiles, but shouldn't:

struct Bug<F: Fn(&u8) = fn() -> &'static u8> {
    F: P,
}

We only get a type error when we write:

impl Bug {}

or

let _: Bug;

[pnkfelix]

ah, so is part of the issue that the type expression used as the default for the type parameter is not checked to actually meet its given trait bounds at its definition site. We wait until a usage site to do the check.

[jonas-schievink]

That only seems to happen in some cases, eg. this example from the OP fails to compile despite being unused:

struct B<P: Sized = [u8]>(P);

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time

(the : Sized is implicit and can be removed, same effect)

The same happens here, ruling out a special-cased sizedness check:

struct B<P: Copy = String>(&'static P);

error[E0277]: the trait bound `std::string::String: std::marker::Copy` is not satisfied

But for some reason @DutchGhost's example above is accepted, despite being clearly wrong:

struct Bug<F: Fn(&u8) = fn() -> &'static u8> {
    f: F,
}

[DutchGhost]

@jonas-schievink , I think it only works for:

• Self
• References (including wrapped references)

Here I build an example playground to experiment:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=bac29fbf5106d03904218cc074da1fa5

use core::marker::{
    PhantomData,
    PhantomPinned,
};

// !Send + !Sync + !Unpin.
// lacks Copy + Clone
struct NoAutoTrait<T> {
    _mark: PhantomData<T>,
    nosend: PhantomData<*mut ()>,
    pinned: PhantomPinned,
}

struct Bug<'a, F: Copy + Send + Sync + Unpin = NoAutoTrait<&'a String>> {
    mark: PhantomData<&'a F>
}

However, if I changed NoAutoTrait<&'a String> to NoAutoTrait<&'static String>, I get compile errors.
Same goes for &'a mut to &'static mut.

[alexreg]

We wait until a usage site to do the checkk.

Yes, exactly. And I think the way that type-checking is implemented, you'll always end up in cycles if you try to do it at definition-site, since you don't have a concrete type yet... which is why @Centril and I thought that maybe we should allow this and solve the use-site issues (e.g., the situation with impls).

[pnkfelix]

Discussed at T-compiler meeting. Sounds pretty thorny.

@eddyb put forward the idea that we might at least try to emit warnings for these cases even if we cannot reliably do hard errors for them.

In any case, needs further investigation before we'd commit to saying that we do not need to do it at the definition site.

Assigning to @eddyb and myself. But also Cc'ing @nikomatsakis in the hopes that they will also chime in.

[eddyb]

even if we cannot reliably do hard errors for them

To be clear, I was thinking of backwards-compatibility concerns, that would imply we might never be able to make something into a proper hard error, even if we have a reliable way to detect it.

---

But I missed the fact that this is specifically about Self in a default of a type parameter.

So here's what I think of Self in type parameter default:
We have logic in the compiler to forbid a default using a later parameter, or the one it's a default of:

rust/src/librustc_resolve/lib.rs

Lines 895 to 944 in a6b5d22

	fn visit_generics(&mut self, generics: &'tcx Generics) {
	// For type parameter defaults, we have to ban access
	// to following type parameters, as the InternalSubsts can only
	// provide previous type parameters as they're built. We
	// put all the parameters on the ban list and then remove
	// them one by one as they are processed and become available.
	let mut default_ban_rib = Rib::new(ForwardTyParamBanRibKind);
	let mut found_default = false;
	default_ban_rib.bindings.extend(generics.params.iter()
	.filter_map(|param| match param.kind {
	GenericParamKind::Const { .. } |
	GenericParamKind::Lifetime { .. } => None,
	GenericParamKind::Type { ref default, .. } => {
	found_default |= default.is_some();
	if found_default {
	Some((Ident::with_empty_ctxt(param.ident.name), Res::Err))
	} else {
	None
	}
	}
	}));
	// We also ban access to type parameters for use as the types of const parameters.
	let mut const_ty_param_ban_rib = Rib::new(TyParamAsConstParamTy);
	const_ty_param_ban_rib.bindings.extend(generics.params.iter()
	.filter(|param| {
	if let GenericParamKind::Type { .. } = param.kind {
	true
	} else {
	false
	}
	})
	.map(|param| (Ident::with_empty_ctxt(param.ident.name), Res::Err)));
	for param in &generics.params {
	match param.kind {
	GenericParamKind::Lifetime { .. } => self.visit_generic_param(param),
	GenericParamKind::Type { ref default, .. } => {
	for bound in &param.bounds {
	self.visit_param_bound(bound);
	}
	if let Some(ref ty) = default {
	self.ribs[TypeNS].push(default_ban_rib);
	self.visit_ty(ty);
	default_ban_rib = self.ribs[TypeNS].pop().unwrap();
	}
	// Allow all following defaults to refer to this type parameter.
	default_ban_rib.bindings.remove(&Ident::with_empty_ctxt(param.ident.name));

Previous comments have mentioned this behavior, but I think it implies something stronger:
Self, by definition, is an alias for TypeName<A, B, C, ...>, so we should be denying Self in places where we deny any of the parameters.

---

But note that this applies to struct, enum, union and impl. Self in trait is a bit different, as it's effectively "the first parameter", so other parameters referring to it is fine - there appears to be a different bug @Centril spotted in #61631 (comment).

[alexreg]

But note that this applies to struct, enum, union and impl. Self in trait is a bit different, as it's effectively "the first parameter", so other parameters referring to it is fine - there appears to be a different bug @Centril spotted in #61631 (comment).

Well that's the whole point, surely? Self is the implicit first parameter, so its usage should be banned in this example per the rule "forbid a default using an earlier parameter".

[eddyb]

@alexreg Oops, that was meant to say "using a later parameter" (that is, we only allow earlier ones).

[alexreg]

@eddyb Ah, that makes sense then... in any case, maybe it makes sense to consider the Self type as an implicit final type parameter for this purpose?

[nikomatsakis]

Clarification question:

When we last discussed this in the @rust-lang/lang meeting, the conclusion was that this was roughly the expected behavior -- except for the bug around Self. This is roughly what @eddyb outlined in this comment. This is still the case, right?

In which case, what we need to do is fix the bug around Self specifically?

[alexreg]

@nikomatsakis What exactly are you referring to as "expected behaviour" here? That using later type params in defaults for earlier ones is disallowed? [EDIT: Ah, are you referring to @DutchGhost's case of (wrapped) references being okay?]

For sure, there's a bug with Self though, yep...

[alexreg]

@eddyb left some general guidance for me on Discord, which may be enough for me to tackle this along with his above comment:

I think the problem is this code doesn't know about the Self in scope https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/lib.rs#L907-L926
hmmm
so the problem is entirely localized within https://github.com/rust-lang/rust/blob/master/src/librustc_resolve/lib.rs#L2492-L2502
walk_item ends up calling visit_generics

[DutchGhost]

Discussed on discord with @alexreg I would make this comment to raise some awareness.

As I showed in my comment here: #61631 (comment) , this issue isn't solely about the Self type being able to compile when it shouldn't, but also about reference types and wrapped references showing the exact same wrong behaviour.

I'd argue the latter should get a fix as well.
Imagine I make a library containing the code I wrote in that comment. For me it compiles, but anyone using it could get compile errors whenever they use my type with the defaults, since any usage of my type with the defaults still results in a compile error.

So, being able to declare types with default parameters that don't satisfy the trait bounds seems wrong to me, because it would allow crates that compile on the crate author's side, but fail to compile on any user's side.

cc @nikomatsakis @alexreg

[nikomatsakis]

Here is a summary comment from the last discussion we had on this topic. This comment then covers the lang-team reasoning about the rules and some specific examples. The TL;DR is that, while it is true that you can get errors at use-sites that could've been detected at the caller site, it's also possible to create constructs that are useful (an example is in the comment). Moreover, we are reluctant to break existing code in general, so becoming more strict is always a trade-off.

In fact, the surprising behavior that you are pointing out, where changing from 'a to 'static starts to generate errors, is a result of the compromise we struck on our last foray here. In particular, we only check if the default has no generic parameters appearing in it. In this case, &'static str does not, so basically any use that leaves the default unspecified is known to be in error. In contrast, &'a str does have a parameter that is not yet known ('a), so we don't check it for consistency.

[nikomatsakis]

I opened rust-lang/reference#636 to document the intended behavior here -- this does not include the bugs around Self, though.

[alexreg]

So... is someone tackling this issue?

[pnkfelix]

triage: changing assignee list to @alexreg and @pnkfelix rather than @eddyb and @pnkfelix

[pnkfelix]

I think I have a patch for this. PR forthcoming.

[pnkfelix]

@eddyb wrote:

But note that this applies to struct, enum, union and impl.

Hey @eddyb, what were you referring to when you said impl here?

An impl<generics ...> Trait for Type (or impl Type) isn't an ADT definition, so I don't know where it would fall in your characterization of Self being an implicit type parameter (either the first one, in the case of trait, or the last one, in the case of ADT items).

And also, that generics ... list isn't allowed to have type parameter defaults, according to #36887

I would imagine that the generics ... in the impl definition could have defaults that refer to Self, no? (nevermind; previous sentence was written before I actually tried to make an example and discovered #36887.)

[eddyb]

An impl<generics ...> Trait for Type (or impl Type) isn't an ADT definition, so I don't know where it would fall in your characterization of Self being an implicit type parameter (either the first one, in the case of trait, or the last one, in the case of ADT items).

I was talking about Self as an "alias" for Adt<generics...> or Type (in an impl), not an implicit parameter.

The reason we should disallow it in ADTs' type parameter defaults is because it contains all the parameters by necessity and expanding it by hand would always produce an error anyway.

[pnkfelix]

I was talking about Self as an "alias" for Adt<generics...> or Type (in an impl), not an implicit parameter.

Ah okay. I was adopting a model based on @alexreg's comments on this issue, and I overlooked the point you made about what Self is "by definition."