Use equality when relating formal and expected type in arg checking

[compiler-errors]

#129059 uncovered an interesting issue in argument checking. When we check arguments, we create three sets of types:

• Formals
• Expected
• Actuals

The actuals are the types of the argument expressions themselves. The formals are the types from the signature that we're checking. The expected types are the formal types, but passed through expected_inputs_for_expected_outputs:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

Lines 691 to 725 in a971212

	/// Unifies the output type with the expected type early, for more coercions
	/// and forward type information on the input expressions.
	#[instrument(skip(self, call_span), level = "debug")]
	pub(crate) fn expected_inputs_for_expected_output(
	&self,
	call_span: Span,
	expected_ret: Expectation<'tcx>,
	formal_ret: Ty<'tcx>,
	formal_args: &[Ty<'tcx>],
	) -> Option<Vec<Ty<'tcx>>> {
	let formal_ret = self.resolve_vars_with_obligations(formal_ret);
	let ret_ty = expected_ret.only_has_type(self)?;
	let expect_args = self
	.fudge_inference_if_ok(|| {
	let ocx = ObligationCtxt::new(self);
	// Attempt to apply a subtyping relationship between the formal
	// return type (likely containing type variables if the function
	// is polymorphic) and the expected return type.
	// No argument expectations are produced if unification fails.
	let origin = self.misc(call_span);
	ocx.sup(&origin, self.param_env, ret_ty, formal_ret)?;
	if !ocx.select_where_possible().is_empty() {
	return Err(TypeError::Mismatch);
	}
	// Record all the argument types, with the args
	// produced from the above subtyping unification.
	Ok(Some(formal_args.iter().map(|&ty| self.resolve_vars_if_possible(ty)).collect()))
	})
	.unwrap_or_default();
	debug!(?formal_args, ?formal_ret, ?expect_args, ?expected_ret);
	expect_args
	}

This method attempts to constrain the formal inputs by relating the expectation of the call expression and the formal output.

When we check an argument, we get the expression's actual type, and then we first attempt to coerce it to the expected type:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

Lines 280 to 293 in a971212

	let checked_ty = self.check_expr_with_expectation(provided_arg, expectation);
	// 2. Coerce to the most detailed type that could be coerced
	// to, which is `expected_ty` if `rvalue_hint` returns an
	// `ExpectHasType(expected_ty)`, or the `formal_ty` otherwise.
	let coerced_ty = expectation.only_has_type(self).unwrap_or(formal_input_ty);
	// Cause selection errors caused by resolving a single argument to point at the
	// argument and not the call. This lets us customize the span pointed to in the
	// fulfillment error to be more accurate.
	let coerced_ty = self.resolve_vars_with_obligations(coerced_ty);
	let coerce_error =
	self.coerce(provided_arg, checked_ty, coerced_ty, AllowTwoPhase::Yes, None).err();

Then we subtype the expected type and the formal type:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs

Lines 299 to 305 in a971212

	// 3. Check if the formal type is a supertype of the checked one
	// and register any such obligations for future type checks
	let supertype_error = self.at(&self.misc(provided_arg.span), self.param_env).sup(
	DefineOpaqueTypes::Yes,
	formal_input_ty,
	coerced_ty,
	);

However, since we are now recording the right coercion target (since #129059), we now end up recording the expected type to the typeck results, rather than the actual.

Since that expected type was fudged, it has fresh variables. And since the expected type is only subtyped against the formal type, if that expected type has a bivariant parameter, it will likely remain unconstrained since Covariant * Bivariant = Bivariant according to xform. This leads to an unconstrained type variable in writeback.

AFAICT, there's no reason for us to be using subtyping here, though. The expected output is already related to the expectation by subtyping:

rust/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs

Line 713 in a971212

ocx.sup(&origin, self.param_env, ret_ty, formal_ret)?;

So the formals don't need "another" indirection of subtyping in the argument checking... So I've changed it to use equality here. We could alternatively fix this by requiring WF for all the expected types to constrain their bivariant parameters, but this seems a bit overkill.

Fixes #129286

[rustbot]

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[compiler-errors]

I expect the only person who's able to review this to be lcnr, who is gone, but I guess I'll just assign it to them.

r? lcnr

[lcnr]

can you move the computation of expected_input_tys into check_argument_types? I feel like treating them as potentially detached from the formal_input_tys is confusing and what caused this bug

r=me on the core change, if you feel like your refactoring is too invasive, I am going to review it this friday

[compiler-errors]

@bors try @rust-timer queue

[bors]

⌛ Trying commit c286326 with merge fc83350...

[bors]

💔 Test failed - checks-actions

[compiler-errors]

@bors try @rust-timer queue

[bors]

⌛ Trying commit f6f5d72 with merge b4cbff9...

[bors]

☀️ Try build successful - checks-actions
Build commit: b4cbff9 (b4cbff954774fa306ee6a05617024c2e1484732d)

[bors]

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

[compiler-errors]

@bors r=lcnr

[bors]

📌 Commit 95b9ecd has been approved by lcnr

It is now in the queue for this repository.

[bors]

⌛ Testing commit 95b9ecd with merge 3cb42da...

[rust-log-analyzer]

The job x86_64-msvc-ext failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[RUSTC-TIMING] miri test:false 4.518
error: failed to remove file `C:\a\rust\rust\build\x86_64-pc-windows-msvc\stage1-tools\x86_64-pc-windows-msvc\release\miri.exe`

Caused by:
  Access is denied. (os error 5)
Command has failed. Rerun with -v to see more details.
  local time: Mon, Sep  2, 2024  1:39:36 PM
  network time: Mon, 02 Sep 2024 13:39:36 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

[bors]

💔 Test failed - checks-actions

[compiler-errors]

@bors retry

[bors]

⌛ Testing commit 95b9ecd with merge bd53aa3...

[bors]

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing bd53aa3 to master...

[rust-timer]

Finished benchmarking commit (bd53aa3): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results (secondary -2.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Cycles

Results (secondary -2.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

Binary size

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

Bootstrap: 750.422s -> 750.044s (-0.05%)
Artifact size: 338.30 MiB -> 338.29 MiB (-0.00%)

[compiler-errors]

@rustbot label +beta-nominated

Fixes a beta regression #130576 which was introduced by #129059, but which i only caught in the crater run for #129021 (comment)