Do not shallow resolve to root var while fudging

[ShoyuVanilla]

Fixes #153816 and fixes #153849

In #151380, I thought that whether shallow resolve to root var or not wouldn't affect the actual type inferencing, but it isn't true for the fudge, in which we discard all newly created relationships between unresolved inference variables 😅

r? lcnr

[ShoyuVanilla]

@bors try @rust-timer queue

[ShoyuVanilla]

@bors try cancel

[rust-bors]

Try build cancelled. Cancelled workflows:

• https://github.com/rust-lang/rust/actions/runs/23089332098

[ShoyuVanilla]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 1edb863 (1edb863e6d15a3bd38cd1bfc3f124a3de6878c25, parent: fd2649988fdc91e056ddd316865d33e812f48a0e)

[rust-timer]

Finished benchmarking commit (1edb863): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Benchmarking this pull request means it may be perf-sensitive – we'll automatically label it not fit for rolling up. You can override this, but we strongly advise not to, due to possible changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please do so in sufficient writing along with @rustbot label: +perf-regression-triaged. If not, please fix the regressions and do another perf run. If its results are neutral or positive, the label will be automatically removed.

@bors rollup=never
@rustbot label: -S-waiting-on-perf +perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.1%	[0.0%, 0.2%]	8
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	1
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (secondary 1.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	1.9%	[1.0%, 2.8%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.9%	[-0.9%, -0.9%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (secondary 3.9%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.9%	[3.9%, 3.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 481.843s -> 481.051s (-0.16%)
Artifact size: 396.85 MiB -> 394.81 MiB (-0.51%)

[lcnr]

I dislike these changes to shallow_resolve as worry that it's easy for this to have unintended sideeffects/weirdness, e.g. it also affects canonicalization in the fudging scope.

My understanding of the two regressions is as follows:

tests/ui/coercion/fudge-inference/input-ty-higher-ranked-fn-trait.rs: We have the expectation Server<?n> and the ret type of the function Server<?m> with n < m. We have an ?m: Fn obligation, so if we don't relate ?n with ?m, but have fudging return ?n, we lose that knowledge

tests/ui/coercion/fudge-inference/input-ty-closure-param-fn-trait-bounds.rs: not actually minimal, this is enough

struct Inv<T, U>(*mut (T, U));
fn pass_through<F>(_: F) -> Inv<F, F> { todo!() }
fn map(_: Inv<impl FnOnce(), impl Fn()>) {}
pub fn traverse() {
    map(pass_through(|| ()))
}

We have the same issue on stable already, if we partially constrain F

struct Inv<T, U>(*mut (T, U));
fn pass_through<F>(_: F) -> Inv<F, F> { todo!() }
fn map(_: Inv<(impl FnOnce(),), (impl Fn(),)>) {}
pub fn traverse() {
    map(pass_through((|| (),)))
}

I don't get why we'd actually limit the closure to only impl FnOnce instead of properly inferring the closure kind as we do normally. That feels like a separate issue here, would be happy for you to look into what happens if we never eagerly infer the closure kind in deduce_closure_signature

Could you instead add a fn resolve_vars_for_fudging which has this special behavior by just ignoring the output of shallow_resolve if its an infer var?

I really dislike this problem :x I feel like there should be a better way to handle this sort of thing, but can't think of anything right away

View changes since this review

[ShoyuVanilla]

Yeah, this doesn't feel to me the right way to do the things, either. I think I should look into the problem more deeply and make some correct fix in a long term, but since the problemtaic issues are stable to beta regression, I ended up in somewhat makeshift 😅

Could you instead add a fn resolve_vars_for_fudging which has this special behavior by just ignoring the output of shallow_resolve if its an infer var?

Yeah, this feels better

I don't get why we'd actually limit the closure to only impl FnOnce instead of properly inferring the closure kind as we do normally. That feels like a separate issue here, would be happy for you to look into what happens if we never eagerly infer the closure kind in deduce_closure_signature

I'm already a bit familiar with this closure kind deduction. Gonna be a bit verbose, so I'll continue in a new comment

[ShoyuVanilla]

In the beta,

struct Inv<T, U>(*mut (T, U));
fn pass_through<F>(_: F) -> Inv<F, F> { todo!() }
fn map(_: Inv<impl FnOnce(), impl Fn()>) {}
pub fn traverse() {
    map(pass_through(|| ()))
}

When we fudge the input expectations in map(pass_through(|| ())), we make Inv<?impl FnOnce(), ?impl FnOnce> :> Inv<?F, ?F>, which results into eq relations ?impl FnOnce() = ?F and ?impl Fn() = ?F.

Since the ty vars aren't fully resolved yet, their root variable becomes something feels quite random, the one with the minimal vid index, ?impl FnOnce(). This is somewhat implementation dependent thing here:

rust/compiler/rustc_infer/src/infer/type_variable.rs

Lines 347 to 349 in e0a8361

	fn order_roots(a: Self, _: &Self::Value, b: Self, _: &Self::Value) -> Option<(Self, Self)> {
	if a.vid.as_u32() < b.vid.as_u32() { Some((a, b)) } else { Some((b, a)) }
	}

With my resolve to root var PR, the pass_through's input type expectation ?F ends up to its root var ?impl FnOnce() from the fudge and therefore, the only predicate we have for the pass_through's input ty is ?impl FnOnce(): impl FnOnce()

This makes us to deduce the closure kind as FnOnce, becase we deduce closure kind from predicates, which leads to {closure}: Fn() -> NoSolution.

rust/compiler/rustc_hir_typeck/src/closure.rs

Line 347 in e0a8361

fn deduce_closure_signature_from_predicates(

But in stable, we do not shallow resolve to root var, so the expectation becomes ?F, which has no Fn trait related predicate.
Thus we infer the closure's kind without any predicates, but modulo upvars, which leads to Fn as || () doesn't have any move or borrow.

For the second code which has a outer concrete wraper type(unary tuple) for the expected input type, the input ty var is resolved into it ((?impl Fn(), )) instead of unresolved ty var both in the beta and the stable, so the closure has {closure}: FnOnce() predicate again, which leads to compilation error for both cases.

I've been thinking this deduce closure kind from predicates behavior somewhat weird since my first PR to rust-lang org - rust-lang/rust-analyzer#16472 😅 (it has been closed, but superseded by my another PR)

rust-analyzer was deducing the kind of a closure purely depending on its upvars, and I thought that makes more sense, but anyway, the source of truth for rust-analyzer is rustc 😄

what happens if we never eagerly infer the closure kind in deduce_closure_signature

So, I can say that the problematic regression(or breaking change) that I meant to fix with that PR will happen if we do so, at least.

struct Foo<F: std::ops::FnOnce()>(F);

fn main() {
    let mut x = String::new();
    let y = Foo(|| {
        x = String::from("foo");
    });
    (y.0)();
}

The above is compiled fine with the current rustc, but if we deduce the kind for the closure with its upvars, it would result in FnMut and the last line inside the main function would emit a mutability error.

[rust-bors]

☀️ Try build successful (CI)
Build commit: bd1c579 (bd1c579a9f06e65c30a9927c80221375e9804aab, parent: 79d2026ae87386ccbe8fc729d130e5e298959a48)

[ShoyuVanilla]

We don't need resolve_var_if_possible here as we already do it in fudge_inference_if_ok

[ShoyuVanilla]

Same here