Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid ICES after reporting errors on erroneous patterns #126320

Merged
merged 5 commits into from
Jun 14, 2024
Merged

Avoid ICES after reporting errors on erroneous patterns #126320

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2733b8a
Avoid follow-up errors on erroneous patterns
oli-obk Jun 11, 2024
a621701
Replace some `Option<Diag>` with `Result<(), Diag>`
oli-obk Jun 12, 2024
ece3e3e
Replace some `Option<Diag>` with `Result<(), Diag>`
oli-obk Jun 12, 2024
e8d6170
Replace some `Option<Diag>` with `Result<(), Diag>`
oli-obk Jun 12, 2024
7566307
Replace a `bool` with a `Result<(), ErrorGuaranteed>`
oli-obk Jun 12, 2024
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
10 changes: 5 additions & 5 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2888,7 +2888,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
..
} = explanation
{
if let Some(diag) = self.try_report_cannot_return_reference_to_local(
if let Err(diag) = self.try_report_cannot_return_reference_to_local(
borrow,
borrow_span,
span,
Expand Down Expand Up @@ -3075,7 +3075,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
if let BorrowExplanation::MustBeValidFor { category, span, from_closure: false, .. } =
explanation
{
if let Some(diag) = self.try_report_cannot_return_reference_to_local(
if let Err(diag) = self.try_report_cannot_return_reference_to_local(
borrow,
proper_span,
span,
Expand Down Expand Up @@ -3237,11 +3237,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
return_span: Span,
category: ConstraintCategory<'tcx>,
opt_place_desc: Option<&String>,
) -> Option<Diag<'tcx>> {
) -> Result<(), Diag<'tcx>> {
let return_kind = match category {
ConstraintCategory::Return(_) => "return",
ConstraintCategory::Yield => "yield",
_ => return None,
_ => return Ok(()),
};

// FIXME use a better heuristic than Spans
Expand Down Expand Up @@ -3317,7 +3317,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

Some(err)
Err(err)
}

#[instrument(level = "debug", skip(self))]
Expand Down
57 changes: 28 additions & 29 deletions compiler/rustc_hir_typeck/src/demand.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use rustc_errors::{Applicability, Diag};
use rustc_hir as hir;
use rustc_hir::def::Res;
use rustc_hir::intravisit::Visitor;
use rustc_infer::infer::{DefineOpaqueTypes, InferOk};
use rustc_infer::infer::DefineOpaqueTypes;
use rustc_middle::bug;
use rustc_middle::ty::adjustment::AllowTwoPhase;
use rustc_middle::ty::error::{ExpectedFound, TypeError};
Expand Down Expand Up @@ -166,7 +166,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Requires that the two types unify, and prints an error message if
/// they don't.
pub fn demand_suptype(&self, sp: Span, expected: Ty<'tcx>, actual: Ty<'tcx>) {
if let Some(e) = self.demand_suptype_diag(sp, expected, actual) {
if let Err(e) = self.demand_suptype_diag(sp, expected, actual) {
e.emit();
}
}
Expand All @@ -176,7 +176,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
sp: Span,
expected: Ty<'tcx>,
actual: Ty<'tcx>,
) -> Option<Diag<'tcx>> {
) -> Result<(), Diag<'tcx>> {
self.demand_suptype_with_origin(&self.misc(sp), expected, actual)
}

Expand All @@ -186,18 +186,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
cause: &ObligationCause<'tcx>,
expected: Ty<'tcx>,
actual: Ty<'tcx>,
) -> Option<Diag<'tcx>> {
match self.at(cause, self.param_env).sup(DefineOpaqueTypes::Yes, expected, actual) {
Ok(InferOk { obligations, value: () }) => {
self.register_predicates(obligations);
None
}
Err(e) => Some(self.err_ctxt().report_mismatched_types(cause, expected, actual, e)),
}
) -> Result<(), Diag<'tcx>> {
self.at(cause, self.param_env)
.sup(DefineOpaqueTypes::Yes, expected, actual)
.map(|infer_ok| self.register_infer_ok_obligations(infer_ok))
.map_err(|e| self.err_ctxt().report_mismatched_types(cause, expected, actual, e))
}

pub fn demand_eqtype(&self, sp: Span, expected: Ty<'tcx>, actual: Ty<'tcx>) {
if let Some(err) = self.demand_eqtype_diag(sp, expected, actual) {
if let Err(err) = self.demand_eqtype_diag(sp, expected, actual) {
err.emit();
}
}
Expand All @@ -207,7 +204,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
sp: Span,
expected: Ty<'tcx>,
actual: Ty<'tcx>,
) -> Option<Diag<'tcx>> {
) -> Result<(), Diag<'tcx>> {
self.demand_eqtype_with_origin(&self.misc(sp), expected, actual)
}

Expand All @@ -216,14 +213,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
cause: &ObligationCause<'tcx>,
expected: Ty<'tcx>,
actual: Ty<'tcx>,
) -> Option<Diag<'tcx>> {
match self.at(cause, self.param_env).eq(DefineOpaqueTypes::Yes, expected, actual) {
Ok(InferOk { obligations, value: () }) => {
self.register_predicates(obligations);
None
}
Err(e) => Some(self.err_ctxt().report_mismatched_types(cause, expected, actual, e)),
}
) -> Result<(), Diag<'tcx>> {
self.at(cause, self.param_env)
.eq(DefineOpaqueTypes::Yes, expected, actual)
.map(|infer_ok| self.register_infer_ok_obligations(infer_ok))
.map_err(|e| self.err_ctxt().report_mismatched_types(cause, expected, actual, e))
}

pub fn demand_coerce(
Expand All @@ -234,12 +228,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
allow_two_phase: AllowTwoPhase,
) -> Ty<'tcx> {
let (ty, err) =
self.demand_coerce_diag(expr, checked_ty, expected, expected_ty_expr, allow_two_phase);
if let Some(err) = err {
err.emit();
match self.demand_coerce_diag(expr, checked_ty, expected, expected_ty_expr, allow_two_phase)
{
Ok(ty) => ty,
Err(err) => {
err.emit();
// Return the original type instead of an error type here, otherwise the type of `x` in
// `let x: u32 = ();` will be a type error, causing all subsequent usages of `x` to not
// report errors, even though `x` is definitely `u32`.
expected
Copy link
Contributor

Choose a reason for hiding this comment

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

interesting that we return expected instead of Ty::new_error here 🤔 if someone wants to experiment with this, try changing it and run our UI test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did have that, the result is not great. I should document it here

}
}
ty
}

/// Checks that the type of `expr` can be coerced to `expected`.
Expand All @@ -254,11 +253,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expected: Ty<'tcx>,
mut expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
allow_two_phase: AllowTwoPhase,
) -> (Ty<'tcx>, Option<Diag<'tcx>>) {
) -> Result<Ty<'tcx>, Diag<'tcx>> {
let expected = self.resolve_vars_with_obligations(expected);

let e = match self.coerce(expr, checked_ty, expected, allow_two_phase, None) {
Ok(ty) => return (ty, None),
Ok(ty) => return Ok(ty),
Copy link
Contributor

Choose a reason for hiding this comment

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

we set_tainted_by_errors in line 263 using a span_delayed_bug and then emit the error further down in line 270. Feel like we should just flip the order and use the ErrorGuaranteed from actually emitting something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, we didn't emit the error initially, so I didn't change it, good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah nevermind, we're not actually emitting the error here, we're just creating the diagnostic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those functions don't have a great name, but that's a very common pattern throughout rustc. Maybe we should add an internal lint for methods with report in their name that also return a Diag.

Err(e) => e,
};

Expand All @@ -275,7 +274,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

self.emit_coerce_suggestions(&mut err, expr, expr_ty, expected, expected_ty_expr, Some(e));

(expected, Some(err))
Err(err)
}

/// Notes the point at which a variable is constrained to some type incompatible
Expand Down
11 changes: 5 additions & 6 deletions compiler/rustc_hir_typeck/src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty = adj_ty;
}

if let Some(mut err) = self.demand_suptype_diag(expr.span, expected_ty, ty) {
if let Err(mut err) = self.demand_suptype_diag(expr.span, expected_ty, ty) {
let _ = self.emit_type_mismatch_suggestions(
&mut err,
expr.peel_drop_temps(),
Expand Down Expand Up @@ -1132,7 +1132,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// say that the user intended to write `lhs == rhs` instead of `lhs = rhs`.
// The likely cause of this is `if foo = bar { .. }`.
let actual_ty = self.tcx.types.unit;
let mut err = self.demand_suptype_diag(expr.span, expected_ty, actual_ty).unwrap();
let mut err = self.demand_suptype_diag(expr.span, expected_ty, actual_ty).unwrap_err();
let lhs_ty = self.check_expr(lhs);
let rhs_ty = self.check_expr(rhs);
let refs_can_coerce = |lhs: Ty<'tcx>, rhs: Ty<'tcx>| {
Expand Down Expand Up @@ -1236,7 +1236,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// This is (basically) inlined `check_expr_coercible_to_type`, but we want
// to suggest an additional fixup here in `suggest_deref_binop`.
let rhs_ty = self.check_expr_with_hint(rhs, lhs_ty);
if let (_, Some(mut diag)) =
if let Err(mut diag) =
self.demand_coerce_diag(rhs, rhs_ty, lhs_ty, Some(lhs), AllowTwoPhase::No)
{
suggest_deref_binop(&mut diag, rhs_ty);
Expand Down Expand Up @@ -1741,10 +1741,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// Make sure to give a type to the field even if there's
// an error, so we can continue type-checking.
let ty = self.check_expr_with_hint(field.expr, field_type);
let (_, diag) =
self.demand_coerce_diag(field.expr, ty, field_type, None, AllowTwoPhase::No);
let diag = self.demand_coerce_diag(field.expr, ty, field_type, None, AllowTwoPhase::No);

if let Some(diag) = diag {
if let Err(diag) = diag {
if idx == hir_fields.len() - 1 {
if remaining_fields.is_empty() {
self.suggest_fru_from_range_and_emit(field, variant, args, diag);
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1578,7 +1578,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// type of the place it is referencing, and not some
// supertype thereof.
let init_ty = self.check_expr_with_needs(init, Needs::maybe_mut_place(m));
if let Some(mut diag) = self.demand_eqtype_diag(init.span, local_ty, init_ty) {
if let Err(mut diag) = self.demand_eqtype_diag(init.span, local_ty, init_ty) {
self.emit_type_mismatch_suggestions(
&mut diag,
init.peel_drop_temps(),
Expand Down Expand Up @@ -1624,7 +1624,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let previous_diverges = self.diverges.get();
let else_ty = self.check_block_with_expected(blk, NoExpectation);
let cause = self.cause(blk.span, ObligationCauseCode::LetElse);
if let Some(err) = self.demand_eqtype_with_origin(&cause, self.tcx.types.never, else_ty)
if let Err(err) = self.demand_eqtype_with_origin(&cause, self.tcx.types.never, else_ty)
{
err.emit();
}
Expand Down
Loading
Loading