No error reported when a generic parameter doesn't meet the requirement of an associated type

[yshui]

Code

I tried this code:

trait A {
    type T: Iterator<Item = i32>;
}

trait B {
    fn x(_: impl A<T = impl Send>);
}

I expected to see this happen:

error[E0277]: `impl Send` is not an iterator

Instead, this happened:

Code compiles

Version it worked on

It most recently worked on: 1.48.0-beta.8

Version with regression

rust nightly 2020-11-08 (likely not the earliest)

[jonas-schievink]

cc @matthewjasper (might be caused by the projection bounds changes)

[camelid]

Note: not just impl Trait; also occurs with ordinary trait bounds.

[camelid]

searched nightlies: from nightly-2020-10-01 to nightly-2020-11-09
regressed nightly: nightly-2020-10-07
searched commits: from a1dfd24 to 98edd1f
regressed commit: 08e2d46

bisected with cargo-bisect-rustc v0.6.0

Host triple: x86_64-apple-darwin
Reproduce with:

cargo bisect-rustc --preserve --regress=success --start=2020-10-01 

---

Indeed caused by #73905.

[camelid]

Assigning P-critical and removing I-prioritize as discussed in the prioritization working group.

[pnkfelix]

We discussed this in today's T-compiler triage meeting.

This may or may not represent a significant change to the static semantics. It may or may not represent a footgun.

My original instinct was to revert PR #73905 before the beta cut, to buy us time. But team members convinced me that would be rash, given the amount of work that builds upon PR #73905.

We decided it would be best to let PR #73905 remain landed for now, but we definitely want T-lang to double-check that we are all okay with the change to the static semantics here.

[oli-obk]

Here's the zulip thread if you want to join the discussion: https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/.2378893.20no.20err.20when.20genrc.20param.20doesnt.20meet.20req.20of.20assoc.20type

[yshui]

@pnkfelix Interesting. Keeping this regression means the user wouldn't get an error until they actually use the type. To me this doesn't feel very desirable. But I can also see the argument that this is not a very big problem, because the user is likely going to use it, otherwise they wouldn't have written it out.

Perhaps this would break the pattern where the user defines a trait, uses the trait, but only implement the trait much later (which is kind of how I found this problem)?

[oli-obk]

You can in fact implement both A and B in this situation (as the examples linked in the zulip show). All you did with B is to add additional constraints to callers of B::x. Though it is true that you don't have access (yet) to the bounds on A::T within B::x

[nikomatsakis]

Related test:

trait A {
    type T: Iterator<Item = i32>;
}

fn foo<X, Y: Send>() 
where
    X: A<T = Y>
{ 
}

this is an error on stable, but not nightly.

[nikomatsakis]

I did a little investigation here. I think that, for the second test I gave at least, the fix I would expect to do would be around here:

rust/compiler/rustc_trait_selection/src/traits/wf.rs

Lines 117 to 120 in fe98231

	ty::PredicateAtom::Projection(t) => {
	wf.compute_projection(t.projection_ty);
	wf.compute(t.ty.into());
	}

In particular, given a where clause like X: A<T = Y>, the first line checks that <X as A>::T is valid, the second checks that Y is valid, but nothing checks that Y meets the bounds declared for T. I believe that what we want is some code that fetches the item_bounds for t.projection_ty.item_def_id, applies the substitutions, and then replaces the "self" types with Y. I started tinkering with writing that code but it's mildly annoying, in part because the self types on item_bounds are full projection types -- I feel like it'd be nice if they were "bound" instead for easy substitution. (I'm not clear on whether item_bounds only applies to associated types?)

In any case, this won't fix the original example though. That would require a change to the opaque type rules, let me investigate that a bit and post a follow up.

[nikomatsakis]

Ok so the impl Trait WF code is here:

rust/compiler/rustc_trait_selection/src/traits/wf.rs

Lines 609 to 613 in fe98231

	// for named opaque `impl Trait` types we still need to check them
	if ty::is_impl_trait_defn(self.infcx.tcx, did).is_none() {
	let obligations = self.nominal_obligations(did, substs);
	self.out.extend(obligations);
	}

Those rules currently fall through to the nominal_obligations here:

rust/compiler/rustc_trait_selection/src/traits/wf.rs

Lines 677 to 681 in fe98231

	fn nominal_obligations(
	&mut self,
	def_id: DefId,
	substs: SubstsRef<'tcx>,
	) -> Vec<traits::PredicateObligation<'tcx>> {

which invokes predicates_of on the opaque type.

Or, well, hmm, maybe the right fix there is to modify how wfcheck applies to the opaque type definition that we synthesize. So one possible fix might be to make the previous change I mentioned and then extend

rust/compiler/rustc_typeck/src/check/wfcheck.rs

Line 78 in fe98231

pub fn check_item_well_formed(tcx: TyCtxt<'_>, def_id: LocalDefId) {

with code to handle HirKind::OpaqueTy to check that the where clauses are well-formed (in the same way as we check for other items).

[Mark-Simulacrum]

This is likely going to be a stable/stable regression with the release of 1.49, but as it expands the set of compiling code I do not see it as a release blocker at this time (and it seems that a fix is at best unlikely in that time frame).

[yshui]

@Mark-Simulacrum if this gets into stable, the eventual fix of this bug is going to be a breaking change.

[pnkfelix]

This is a stable-to-stable regression now with the 1.49 release.

[chrysn]

Closing #80821 as a duplicate of this; the code provided there is something that can easily come up in development on 1.49, and all the types involved are used in the example code (showing that this is not just about errors in traits that can't be implemented anyway).

[jackh726]

This compiles now, marking as needs-test

[chrysn]

This has compiled all the way since 1.49, which is the problem: it should not have, the regression is that builds started to work. (AIU there is work going on to make requirements propagate implicitly, but these should still be behind a feature gate and were not part of release announcements that'd make people with MSRV requirements aware of them).

[jackh726]

Ah, I missed that. Relabeled accordingly.

[lcnr]

discussed during t-types backlog bonanza.

Going to discuss this in a later meeting to make a final discussion here

[jackh726]

Discussed today during t-types meeting.

We agreed that this should compile, given this also compiles:

trait B {
    fn x(_: impl A);
}

In other words, the bounds specified in the function arg are a convenient way to add new bounds, but don't change the fact that you still must prove the bounds on the trait.

We think this can be closed with the following tests (or test with revisions):

1. A test added showing this compiles
2. A test added that shows you can't call x without proving that T: Iterator<Item = i32>

[yshui]

@jackh726 Then shouldn't this also compile with the same logic?

trait A<T: Iterator<Item = i32>> {
}
trait B {
    fn x<T: Send>(_: impl A<T>);
}

Why is associated type treated differently from type parameters?

[lcnr]

@yshui we've ended up quickly talking about your question in the t-types meeting as well

my intuition for this is that bounds on associated types have to be proven by the impl while bounds on generic arguments have to be proven by the users of that impl.

A bound like T: Trait<Assoc = U> means that T: Trait, and <T as Trait>::Assoc == U must hold.

The equality implies that all bounds on <T as Trait>::Assoc must also hold for U (as these two are equal), but it never requires it. Both T: Trait and <T as Trait>::Assoc == U do not require U: Iterator<Item = i32>.

If you have T: Trait<U>, you cannot destruct this into smaller goals, so you have to prove all the requirements for this, including U: Iterator<Item = i32>.

As mentioned in the meeting itself, this isn't too great of an explanation if you aren't already deeply involved in the trait system, but I hope it was at least somewhat understandable 😅

[yshui]

@lcnr This seems to be tied to the specific implementation of the trait system in rustc. And, at least to me, doesn't feel like an intuitive distinction between the 2 cases.

i.e. another implementation of the Rust language might come along and implement this differently. (mrustc doesn't report error for either).

But I guess in the end this isn't that impactful of a change, and I can go with the decision. 👍