Skip to content

Defer repeat expr Copy checks to end of type checking #137045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 1, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Defer repeat expr Copy check
  • Loading branch information
BoxyUwU committed Feb 27, 2025
commit dc6db192c4bc8906b42c68f5ed586cf1b41477a2
11 changes: 7 additions & 4 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1853,12 +1853,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
return Ty::new_error(tcx, guar);
}

// We defer checking whether the element type is `Copy` as it is possible to have
// an inference variable as a repeat count and it seems unlikely that `Copy` would
// have inference side effects required for type checking to succeed.
if tcx.features().generic_arg_infer() {
self.deferred_repeat_expr_checks.borrow_mut().push((element, element_ty, count));
// If the length is 0, we don't create any elements, so we don't copy any.
// If the length is 1, we don't copy that one element, we move it. Only check
// for `Copy` if the length is larger, or unevaluated.
// FIXME(min_const_generic_exprs): We could perhaps defer this check so that
// we don't require `<?0t as Tr>::CONST` doesn't unnecessarily require `Copy`.
if count.try_to_target_usize(tcx).is_none_or(|x| x > 1) {
} else if count.try_to_target_usize(self.tcx).is_none_or(|x| x > 1) {
self.enforce_repeat_element_needs_copy_bound(element, element_ty);
}

Expand All @@ -1868,7 +1871,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

/// Requires that `element_ty` is `Copy` (unless it's a const expression itself).
fn enforce_repeat_element_needs_copy_bound(
pub(super) fn enforce_repeat_element_needs_copy_bound(
&self,
element: &hir::Expr<'_>,
element_ty: Ty<'tcx>,
Expand Down
51 changes: 40 additions & 11 deletions compiler/rustc_hir_typeck/src/fn_ctxt/_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,33 +85,36 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
})
}

/// Resolves type and const variables in `ty` if possible. Unlike the infcx
/// Resolves type and const variables in `t` if possible. Unlike the infcx
/// version (resolve_vars_if_possible), this version will
/// also select obligations if it seems useful, in an effort
/// to get more type information.
// FIXME(-Znext-solver): A lot of the calls to this method should
// probably be `try_structurally_resolve_type` or `structurally_resolve_type` instead.
#[instrument(skip(self), level = "debug", ret)]
pub(crate) fn resolve_vars_with_obligations(&self, mut ty: Ty<'tcx>) -> Ty<'tcx> {
pub(crate) fn resolve_vars_with_obligations<T: TypeFoldable<TyCtxt<'tcx>>>(
&self,
mut t: T,
) -> T {
// No Infer()? Nothing needs doing.
if !ty.has_non_region_infer() {
if !t.has_non_region_infer() {
debug!("no inference var, nothing needs doing");
return ty;
return t;
}

// If `ty` is a type variable, see whether we already know what it is.
ty = self.resolve_vars_if_possible(ty);
if !ty.has_non_region_infer() {
debug!(?ty);
return ty;
// If `t` is a type variable, see whether we already know what it is.
t = self.resolve_vars_if_possible(t);
if !t.has_non_region_infer() {
debug!(?t);
return t;
}

// If not, try resolving pending obligations as much as
// possible. This can help substantially when there are
// indirect dependencies that don't seem worth tracking
// precisely.
self.select_obligations_where_possible(|_| {});
self.resolve_vars_if_possible(ty)
self.resolve_vars_if_possible(t)
}

pub(crate) fn record_deferred_call_resolution(
Expand Down Expand Up @@ -1454,7 +1457,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
sp: Span,
ct: ty::Const<'tcx>,
) -> ty::Const<'tcx> {
// FIXME(min_const_generic_exprs): We could process obligations here if `ct` is a var.
let ct = self.resolve_vars_with_obligations(ct);

if self.next_trait_solver()
&& let ty::ConstKind::Unevaluated(..) = ct.kind()
Expand Down Expand Up @@ -1510,6 +1513,32 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

pub(crate) fn structurally_resolve_const(
&self,
sp: Span,
ct: ty::Const<'tcx>,
) -> ty::Const<'tcx> {
let ct = self.try_structurally_resolve_const(sp, ct);

if !ct.is_ct_infer() {
ct
} else {
let e = self.tainted_by_errors().unwrap_or_else(|| {
self.err_ctxt()
.emit_inference_failure_err(
self.body_id,
sp,
ct.into(),
TypeAnnotationNeeded::E0282,
true,
)
.emit()
});
// FIXME: Infer `?ct = {const error}`?
ty::Const::new_error(self.tcx, e)
Comment on lines +1537 to +1538
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no fcx.demand_subtype for consts yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could just self.at(..).eq(ct, err)?

}
}

pub(crate) fn with_breakable_ctxt<F: FnOnce() -> R, R>(
&self,
id: HirId,
Expand Down
25 changes: 25 additions & 0 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,31 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

pub(in super::super) fn check_repeat_exprs(&self) {
let mut deferred_repeat_expr_checks = self.deferred_repeat_expr_checks.borrow_mut();
debug!("FnCtxt::check_repeat_exprs: {} deferred checks", deferred_repeat_expr_checks.len());
for (element, element_ty, count) in deferred_repeat_expr_checks.drain(..) {
// We want to emit an error if the const is not structurally resolveable as otherwise
// we can find up conservatively proving `Copy` which may infer the repeat expr count
// to something that never required `Copy` in the first place.
let count =
self.structurally_resolve_const(element.span, self.normalize(element.span, count));

// Avoid run on "`NotCopy: Copy` is not implemented" errors when the repeat expr count
// is erroneous/unknown. The user might wind up specifying a repeat count of 0/1.
if count.references_error() {
continue;
}

// If the length is 0, we don't create any elements, so we don't copy any.
// If the length is 1, we don't copy that one element, we move it. Only check
// for `Copy` if the length is larger.
if count.try_to_target_usize(self.tcx).is_none_or(|x| x > 1) {
self.enforce_repeat_element_needs_copy_bound(element, element_ty);
}
}
}

pub(in super::super) fn check_method_argument_types(
&self,
sp: Span,
Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_hir_typeck/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,15 @@ fn typeck_with_inspect<'tcx>(
fcx.write_ty(id, expected_type);
};

// Whether to check repeat exprs before/after inference fallback is somewhat arbitrary of a decision
// as neither option is strictly more permissive than the other. However, we opt to check repeat exprs
// first as errors from not having inferred array lengths yet seem less confusing than errors from inference
// fallback arbitrarily inferring something incompatible with `Copy` inference side effects.
//
// This should also be forwards compatible with moving repeat expr checks to a custom goal kind or using
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel somewhat hesitant to add a custom goal kind for repeat expr checks but I do think it would be "optimal" behaviour-wise as the goals could stall on the repeat count infer var and be deferred ~as long as necessary while also not delaying inference constraints from them any later than is needed.

// marker traits in the future.
fcx.check_repeat_exprs();

fcx.type_inference_fallback();

// Even though coercion casts provide type hints, we check casts after fallback for
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_hir_typeck/src/typeck_root_ctxt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ pub(crate) struct TypeckRootCtxt<'tcx> {

pub(super) deferred_coroutine_interiors: RefCell<Vec<(LocalDefId, hir::BodyId, Ty<'tcx>)>>,

pub(super) deferred_repeat_expr_checks:
RefCell<Vec<(&'tcx hir::Expr<'tcx>, Ty<'tcx>, ty::Const<'tcx>)>>,

/// Whenever we introduce an adjustment from `!` into a type variable,
/// we record that type variable here. This is later used to inform
/// fallback. See the `fallback` module for details.
Expand Down Expand Up @@ -96,6 +99,7 @@ impl<'tcx> TypeckRootCtxt<'tcx> {
deferred_transmute_checks: RefCell::new(Vec::new()),
deferred_asm_checks: RefCell::new(Vec::new()),
deferred_coroutine_interiors: RefCell::new(Vec::new()),
deferred_repeat_expr_checks: RefCell::new(Vec::new()),
diverging_type_vars: RefCell::new(Default::default()),
infer_var_info: RefCell::new(Default::default()),
}
Expand Down