fix unification of const variables

[lcnr]

r? @nikomatsakis @varkor @eddyb let's just ping everyone here 😅

[lcnr]

I am not too familiar with what has to be done with UniverseIndex here. When instantiating type inference variables, we sometimes adjust them, which we might also have to do for consts

rust/src/librustc_infer/infer/combine.rs

Lines 660 to 668 in 0cd7ff7

	if self.for_universe.can_name(universe) {
	Ok(c)
	} else {
	let new_var_id = variable_table.new_key(ConstVarValue {
	origin: var_value.origin,
	val: ConstVariableValue::Unknown { universe: self.for_universe },
	});
	Ok(self.tcx().mk_const_var(new_var_id, c.ty))
	}

and

rust/src/librustc_infer/infer/combine.rs

Lines 558 to 587 in 0cd7ff7

	match self.ambient_variance {
	// Invariant: no need to make a fresh type variable.
	ty::Invariant => {
	if self.for_universe.can_name(universe) {
	return Ok(t);
	}
	}
	// Bivariant: make a fresh var, but we
	// may need a WF predicate. See
	// comment on `needs_wf` field for
	// more info.
	ty::Bivariant => self.needs_wf = true,
	// Co/contravariant: this will be
	// sufficiently constrained later on.
	ty::Covariant | ty::Contravariant => (),
	}
	let origin =
	*self.infcx.inner.borrow_mut().type_variables().var_origin(vid);
	let new_var_id = self
	.infcx
	.inner
	.borrow_mut()
	.type_variables()
	.new_var(self.for_universe, false, origin);
	let u = self.tcx().mk_ty_var(new_var_id);
	debug!("generalize: replacing original vid={:?} with new={:?}", vid, u);
	Ok(u)

[nikomatsakis]

OK, so, the idea is that... if we are providing a value V for some variable ?A, then we have to ensure that the value V can be named from within the universe of ?A. e.g., if ?A is in the root universe, then V can only reference names in the root universe.

Therefore, if V contains some other variable ?B, then the universe Universe(?B) of ?B must be some ancestor of Universe(?A).

We have to decide how "eagerly" we want to report these errors.

In chalk at least, we treat type variables and lifetime variables differently. For example, if we have exists<T> { forall<U> { T = U } } in chalk, we consider that to be a unification failure. But if we have exists<'a> { forall<'b> { 'a = 'b } } in chalk, we produce a region constraint that will never be satisfied.

However, Rust doesn't presently have binders over type variables like this, so the question doesn't arise in stable rust. I guess I have to go read what the unification code does now.

[lcnr]

Hm, so I guess the goal is to use a TypeFolder instead of a visitor here and update the type and const variables if they are in a universe which can't be named.

I am still not completely sure where this is needed and how to test this, so it might be fine to merge this with a FIXME here

[nikomatsakis]

Given the limited range of "forall" currently possible in Rust, that may well be true...not sure. I guess there might be things like exists<'a> { forall<'b> { <T as Foo<'a>>::BAR = <T as Foo<'b>>::BAR } } or something that might indirectly relate lifetimes with universes and be led astray.

[nikomatsakis]

I guess I should review the type unification code.

[lcnr]

@nikomatsakis @varkor I added a comment which explains what we are trying to prevent here,
please tell me if there is anything else you want me to elaborate on.

[nikomatsakis]

Sorry for slow review. Will try to get this done ASAP!

[nikomatsakis]

OK, I read the comments here, but @lcnr I'm feeling a bit uncertain as to what I think is best. Maybe we should find some time to chat a bit interactively?

[nikomatsakis]

OK, so, the idea is that... if we are providing a value V for some variable ?A, then we have to ensure that the value V can be named from within the universe of ?A. e.g., if ?A is in the root universe, then V can only reference names in the root universe.

Therefore, if V contains some other variable ?B, then the universe Universe(?B) of ?B must be some ancestor of Universe(?A).

We have to decide how "eagerly" we want to report these errors.

In chalk at least, we treat type variables and lifetime variables differently. For example, if we have exists<T> { forall<U> { T = U } } in chalk, we consider that to be a unification failure. But if we have exists<'a> { forall<'b> { 'a = 'b } } in chalk, we produce a region constraint that will never be satisfied.

However, Rust doesn't presently have binders over type variables like this, so the question doesn't arise in stable rust. I guess I have to go read what the unification code does now.

[bors]

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

[bors]

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

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

[nikomatsakis]

Sorry for being super slow. I read over this quickly and everything looks right. Given how little time I have for reviewing I'm going to r+ despite not having done a deep read, I certainly trust @lcnr and we can patch it up later if there's something wrong =)

r=me but needs rebase.

[lcnr]

let's not use a rollup here in case this breaks something

@bors r=nikomatsakis rollup=never

[bors]

📌 Commit 0f17d355693c5c0fbc3ac5399273066725ddb476 has been approved by nikomatsakis

[lcnr]

@bors r- formatting

[lcnr]

@bors r=nikomatsakis rollup=never

[bors]

📌 Commit 2855b92 has been approved by nikomatsakis

[bors]

⌛ Testing commit 2855b92 with merge e0bf356...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing e0bf356 to master...

[ecstatic-morse]

Hi! This PR showed up in the weekly perf triage report. It seems to have caused a
moderate regression in instruction counts
(up to 3.3% on incr-full builds of coercions-debug).

However, no other benchmarks regressed, which is pretty unusual. I doubt that there's much to be done here, but just in case cc @lcnr

[lcnr]

So we now do slightly more work when dealing with unevaluated constants, which rarely happens without const_generics so I did not expect a huge impact here.

The regression for coercions-debug looks like llvm has more work here, which really should not happen beause of that PR as I do not think we change anything outside of typeck, is it worth it to try and revert this to see if the regression is deterministic?

[ecstatic-morse]

Looking at recent perf results, something seems to have introduced some bimodal noise for coercions-debug. I'll try to revert a few PRs in that range to see what the cause is.