Skip to content

Commit

Permalink
Auto merge of #129317 - compiler-errors:expectation-subtyping, r=<try>
Browse files Browse the repository at this point in the history
Use equality when relating formal and expected type in arg checking

#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`:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L691-L725

This method attempts to constrain the formal inputs by relating the [expectation](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_hir_typeck/expectation/enum.Expectation.html) 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:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L280-L293

Then we subtype the expected type and the formal type:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs#L299-L305

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](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_infer/infer/struct.InferCtxt.html#method.fudge_inference_if_ok), 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](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/enum.Variance.html#method.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:

https://github.com/rust-lang/rust/blob/a971212545766fdfe0dd68e5d968133f79944a19/compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs#L713

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
  • Loading branch information
bors committed Aug 25, 2024
2 parents 8910346 + f6f5d72 commit b4cbff9
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 80 deletions.
21 changes: 4 additions & 17 deletions compiler/rustc_hir_typeck/src/callee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,18 +502,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let fn_sig = self.instantiate_binder_with_fresh_vars(call_expr.span, infer::FnCall, fn_sig);
let fn_sig = self.normalize(call_expr.span, fn_sig);

// Call the generic checker.
let expected_arg_tys = self.expected_inputs_for_expected_output(
call_expr.span,
expected,
fn_sig.output(),
fn_sig.inputs(),
);
self.check_argument_types(
call_expr.span,
call_expr,
fn_sig.inputs(),
expected_arg_tys,
fn_sig.output(),
expected,
arg_exprs,
fn_sig.c_variadic,
TupleArgumentsFlag::DontTupleArguments,
Expand Down Expand Up @@ -865,19 +859,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// don't know the full details yet (`Fn` vs `FnMut` etc), but we
// do know the types expected for each argument and the return
// type.

let expected_arg_tys = self.expected_inputs_for_expected_output(
call_expr.span,
expected,
fn_sig.output(),
fn_sig.inputs(),
);

self.check_argument_types(
call_expr.span,
call_expr,
fn_sig.inputs(),
expected_arg_tys,
fn_sig.output(),
expected,
arg_exprs,
fn_sig.c_variadic,
TupleArgumentsFlag::TupleArguments,
Expand Down
25 changes: 18 additions & 7 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1668,15 +1668,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
) {
let tcx = self.tcx;

let expected_inputs =
self.expected_inputs_for_expected_output(span, expected, adt_ty, &[adt_ty]);
let adt_ty_hint = if let Some(expected_inputs) = expected_inputs {
expected_inputs.get(0).cloned().unwrap_or(adt_ty)
let adt_ty = self.resolve_vars_with_obligations(adt_ty);
let adt_ty_hint = if adt_ty.has_non_region_infer() {
expected.only_has_type(self).and_then(|expected| {
self.fudge_inference_if_ok(|| {
let ocx = ObligationCtxt::new(self);
ocx.sup(&self.misc(span), self.param_env, expected, adt_ty)?;
if !ocx.select_where_possible().is_empty() {
return Err(TypeError::Mismatch);
}
Ok(self.resolve_vars_if_possible(adt_ty))
})
.ok()
})
} else {
adt_ty
None
};
// re-link the regions that EIfEO can erase.
self.demand_eqtype(span, adt_ty_hint, adt_ty);
if let Some(adt_ty_hint) = adt_ty_hint {
// re-link the variables that the fudging above can create.
self.demand_eqtype(span, adt_ty_hint, adt_ty);
}

let ty::Adt(adt, args) = adt_ty.kind() else {
span_bug!(span, "non-ADT passed to check_expr_struct_fields");
Expand Down
39 changes: 1 addition & 38 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ use rustc_infer::infer::canonical::{Canonical, OriginalQueryValues, QueryRespons
use rustc_infer::infer::{DefineOpaqueTypes, InferResult};
use rustc_lint::builtin::SELF_CONSTRUCTOR_FROM_OUTER_ITEM;
use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
use rustc_middle::ty::error::TypeError;
use rustc_middle::ty::fold::TypeFoldable;
use rustc_middle::ty::visit::{TypeVisitable, TypeVisitableExt};
use rustc_middle::ty::{
Expand All @@ -36,7 +35,7 @@ use rustc_span::Span;
use rustc_target::abi::FieldIdx;
use rustc_trait_selection::error_reporting::infer::need_type_info::TypeAnnotationNeeded;
use rustc_trait_selection::traits::{
self, NormalizeExt, ObligationCauseCode, ObligationCtxt, StructurallyNormalizeExt,
self, NormalizeExt, ObligationCauseCode, StructurallyNormalizeExt,
};

use crate::callee::{self, DeferredCallResolution};
Expand Down Expand Up @@ -688,42 +687,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
vec![ty_error; len]
}

/// 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
}

pub(crate) fn resolve_lang_item_path(
&self,
lang_item: hir::LangItem,
Expand Down
74 changes: 56 additions & 18 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
use rustc_index::IndexVec;
use rustc_infer::infer::{DefineOpaqueTypes, InferOk, TypeTrace};
use rustc_middle::ty::adjustment::AllowTwoPhase;
use rustc_middle::ty::error::TypeError;
use rustc_middle::ty::visit::TypeVisitableExt;
use rustc_middle::ty::{self, IsSuggestable, Ty, TyCtxt};
use rustc_middle::{bug, span_bug};
Expand All @@ -25,7 +26,7 @@ use rustc_span::symbol::{kw, Ident};
use rustc_span::{sym, Span, DUMMY_SP};
use rustc_trait_selection::error_reporting::infer::{FailureCode, ObligationCauseExt};
use rustc_trait_selection::infer::InferCtxtExt;
use rustc_trait_selection::traits::{self, ObligationCauseCode, SelectionContext};
use rustc_trait_selection::traits::{self, ObligationCauseCode, ObligationCtxt, SelectionContext};
use {rustc_ast as ast, rustc_hir as hir};

use crate::coercion::CoerceMany;
Expand Down Expand Up @@ -123,6 +124,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
};
if let Err(guar) = has_error {
let err_inputs = self.err_args(args_no_rcvr.len(), guar);
let err_output = Ty::new_error(self.tcx, guar);

let err_inputs = match tuple_arguments {
DontTupleArguments => err_inputs,
Expand All @@ -133,28 +135,23 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
sp,
expr,
&err_inputs,
None,
err_output,
NoExpectation,
args_no_rcvr,
false,
tuple_arguments,
method.ok().map(|method| method.def_id),
);
return Ty::new_error(self.tcx, guar);
return err_output;
}

let method = method.unwrap();
// HACK(eddyb) ignore self in the definition (see above).
let expected_input_tys = self.expected_inputs_for_expected_output(
sp,
expected,
method.sig.output(),
&method.sig.inputs()[1..],
);
self.check_argument_types(
sp,
expr,
&method.sig.inputs()[1..],
expected_input_tys,
method.sig.output(),
expected,
args_no_rcvr,
method.sig.c_variadic,
tuple_arguments,
Expand All @@ -174,8 +171,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
call_expr: &'tcx hir::Expr<'tcx>,
// Types (as defined in the *signature* of the target function)
formal_input_tys: &[Ty<'tcx>],
// More specific expected types, after unifying with caller output types
expected_input_tys: Option<Vec<Ty<'tcx>>>,
formal_output: Ty<'tcx>,
// Expected output from the parent expression or statement
expectation: Expectation<'tcx>,
// The expressions for each provided argument
provided_args: &'tcx [hir::Expr<'tcx>],
// Whether the function is variadic, for example when imported from C
Expand Down Expand Up @@ -209,6 +207,47 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
}

// First, let's unify the formal method signature with the expectation eagerly.
// We use this to guide coercion inference; it's output is "fudged" which means
// any remaining type variables are assigned to new, unrelated variables. This
// is because the inference guidance here is only speculative.
//
// We only do this if the formals have non-region infer vars, since this is only
// meant to guide inference.
let formal_output = self.resolve_vars_with_obligations(formal_output);
let expected_input_tys: Option<Vec<_>> = if formal_input_tys.has_non_region_infer() {
expectation
.only_has_type(self)
.and_then(|expected_output| {
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, expected_output, formal_output)?;
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_input_tys
.iter()
.map(|&ty| self.resolve_vars_if_possible(ty))
.collect(),
))
})
.ok()
})
.unwrap_or_default()
} else {
None
};

let mut err_code = E0061;

// If the arguments should be wrapped in a tuple (ex: closures), unwrap them here
Expand Down Expand Up @@ -291,21 +330,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let coerce_error =
self.coerce(provided_arg, checked_ty, coerced_ty, AllowTwoPhase::Yes, None).err();

if coerce_error.is_some() {
return Compatibility::Incompatible(coerce_error);
}

// 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(
// 3. Check if the formal type is actually equal to the checked one
// and register any such obligations for future type checks.
let formal_ty_error = self.at(&self.misc(provided_arg.span), self.param_env).eq(
DefineOpaqueTypes::Yes,
formal_input_ty,
coerced_ty,
);

// If neither check failed, the types are compatible
match supertype_error {
match formal_ty_error {
Ok(InferOk { obligations, value: () }) => {
self.register_predicates(obligations);
Compatibility::Compatible
Expand Down
19 changes: 19 additions & 0 deletions tests/ui/coercion/constrain-expectation-in-arg.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//@ check-pass

trait Trait {
type Item;
}

struct Struct<A: Trait<Item = B>, B> {
pub field: A,
}

fn identity<T>(x: T) -> T {
x
}

fn test<A: Trait<Item = B>, B>(x: &Struct<A, B>) {
let x: &Struct<_, _> = identity(x);
}

fn main() {}

0 comments on commit b4cbff9

Please sign in to comment.