Skip to content

Commit d5d5eb4

Browse files
matthiaskrgrmatthiaskrgr
authored
Rollup merge of #139594 - compiler-errors:if-cause, r=oli-obk
Simplify `ObligationCauseCode::IfExpression` This originally started out as an experiment to do less incremental invalidation by deferring the span operations that happen on the good path in `check_expr_if`, but it ended up not helping much (or at least not showing up in our incremental tests). As a side-effect though, I think the code is a lot cleaner and there are modest diagnostics improvements with overlapping spans, so I think it's still worth landing.
2 parents aa8ba54 + 59e1a3c commit d5d5eb4

File tree

15 files changed

+202
-265
lines changed

15 files changed

+202
-265
lines changed

‎compiler/rustc_hir_typeck/src/_match.rs‎

Lines changed: 5 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
use rustc_errors::{Applicability, Diag};
22
use rustc_hir::def::{CtorOf, DefKind, Res};
33
use rustc_hir::def_id::LocalDefId;
4-
use rustc_hir::{self as hir, ExprKind, PatKind};
4+
use rustc_hir::{self as hir, ExprKind, HirId, PatKind};
55
use rustc_hir_pretty::ty_to_string;
66
use rustc_middle::ty::{self, Ty};
77
use rustc_span::Span;
88
use rustc_trait_selection::traits::{
9-
IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
9+
MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
1010
};
1111
use tracing::{debug, instrument};
1212

@@ -414,105 +414,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
414414

415415
pub(crate) fn if_cause(
416416
&self,
417-
span: Span,
418-
cond_span: Span,
419-
then_expr: &'tcx hir::Expr<'tcx>,
417+
expr_id: HirId,
420418
else_expr: &'tcx hir::Expr<'tcx>,
421-
then_ty: Ty<'tcx>,
422-
else_ty: Ty<'tcx>,
423419
tail_defines_return_position_impl_trait: Option<LocalDefId>,
424420
) -> ObligationCause<'tcx> {
425-
let mut outer_span = if self.tcx.sess.source_map().is_multiline(span) {
426-
// The `if`/`else` isn't in one line in the output, include some context to make it
427-
// clear it is an if/else expression:
428-
// ```
429-
// LL | let x = if true {
430-
// | _____________-
431-
// LL || 10i32
432-
// || ----- expected because of this
433-
// LL || } else {
434-
// LL || 10u32
435-
// || ^^^^^ expected `i32`, found `u32`
436-
// LL || };
437-
// ||_____- `if` and `else` have incompatible types
438-
// ```
439-
Some(span)
440-
} else {
441-
// The entire expression is in one line, only point at the arms
442-
// ```
443-
// LL | let x = if true { 10i32 } else { 10u32 };
444-
// | ----- ^^^^^ expected `i32`, found `u32`
445-
// | |
446-
// | expected because of this
447-
// ```
448-
None
449-
};
450-
451-
let (error_sp, else_id) = if let ExprKind::Block(block, _) = &else_expr.kind {
452-
let block = block.innermost_block();
453-
454-
// Avoid overlapping spans that aren't as readable:
455-
// ```
456-
// 2 | let x = if true {
457-
// | _____________-
458-
// 3 | | 3
459-
// | | - expected because of this
460-
// 4 | | } else {
461-
// | |____________^
462-
// 5 | ||
463-
// 6 | || };
464-
// | || ^
465-
// | ||_____|
466-
// | |______if and else have incompatible types
467-
// | expected integer, found `()`
468-
// ```
469-
// by not pointing at the entire expression:
470-
// ```
471-
// 2 | let x = if true {
472-
// | ------- `if` and `else` have incompatible types
473-
// 3 | 3
474-
// | - expected because of this
475-
// 4 | } else {
476-
// | ____________^
477-
// 5 | |
478-
// 6 | | };
479-
// | |_____^ expected integer, found `()`
480-
// ```
481-
if block.expr.is_none()
482-
&& block.stmts.is_empty()
483-
&& let Some(outer_span) = &mut outer_span
484-
&& let Some(cond_span) = cond_span.find_ancestor_inside(*outer_span)
485-
{
486-
*outer_span = outer_span.with_hi(cond_span.hi())
487-
}
488-
489-
(self.find_block_span(block), block.hir_id)
490-
} else {
491-
(else_expr.span, else_expr.hir_id)
492-
};
493-
494-
let then_id = if let ExprKind::Block(block, _) = &then_expr.kind {
495-
let block = block.innermost_block();
496-
// Exclude overlapping spans
497-
if block.expr.is_none() && block.stmts.is_empty() {
498-
outer_span = None;
499-
}
500-
block.hir_id
501-
} else {
502-
then_expr.hir_id
503-
};
421+
let error_sp = self.find_block_span_from_hir_id(else_expr.hir_id);
504422

505423
// Finally construct the cause:
506424
self.cause(
507425
error_sp,
508-
ObligationCauseCode::IfExpression(Box::new(IfExpressionCause {
509-
else_id,
510-
then_id,
511-
then_ty,
512-
else_ty,
513-
outer_span,
514-
tail_defines_return_position_impl_trait,
515-
})),
426+
ObligationCauseCode::IfExpression { expr_id, tail_defines_return_position_impl_trait },
516427
)
517428
}
518429

‎compiler/rustc_hir_typeck/src/coercion.rs‎

Lines changed: 18 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
4646
use rustc_infer::infer::relate::RelateResult;
4747
use rustc_infer::infer::{Coercion, DefineOpaqueTypes, InferOk, InferResult};
4848
use rustc_infer::traits::{
49-
IfExpressionCause, ImplSource, MatchExpressionArmCause, Obligation, PredicateObligation,
50-
PredicateObligations, SelectionError,
49+
MatchExpressionArmCause, Obligation, PredicateObligation, PredicateObligations, SelectionError,
5150
};
5251
use rustc_middle::span_bug;
5352
use rustc_middle::ty::adjustment::{
@@ -59,7 +58,7 @@ use rustc_span::{BytePos, DUMMY_SP, DesugaringKind, Span};
5958
use rustc_trait_selection::infer::InferCtxtExt as _;
6059
use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
6160
use rustc_trait_selection::traits::{
62-
self, NormalizeExt, ObligationCause, ObligationCauseCode, ObligationCtxt,
61+
self, ImplSource, NormalizeExt, ObligationCause, ObligationCauseCode, ObligationCtxt,
6362
};
6463
use smallvec::{SmallVec, smallvec};
6564
use tracing::{debug, instrument};
@@ -1719,39 +1718,30 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
17191718
);
17201719
}
17211720
}
1722-
ObligationCauseCode::IfExpression(box IfExpressionCause {
1723-
then_id,
1724-
else_id,
1725-
then_ty,
1726-
else_ty,
1721+
ObligationCauseCode::IfExpression {
1722+
expr_id,
17271723
tail_defines_return_position_impl_trait: Some(rpit_def_id),
1728-
..
1729-
}) => {
1724+
} => {
1725+
let hir::Node::Expr(hir::Expr {
1726+
kind: hir::ExprKind::If(_, then_expr, Some(else_expr)),
1727+
..
1728+
}) = fcx.tcx.hir_node(expr_id)
1729+
else {
1730+
unreachable!();
1731+
};
17301732
err = fcx.err_ctxt().report_mismatched_types(
17311733
cause,
17321734
fcx.param_env,
17331735
expected,
17341736
found,
17351737
coercion_error,
17361738
);
1737-
let then_span = fcx.find_block_span_from_hir_id(then_id);
1738-
let else_span = fcx.find_block_span_from_hir_id(else_id);
1739-
// don't suggest wrapping either blocks in `if .. {} else {}`
1740-
let is_empty_arm = |id| {
1741-
let hir::Node::Block(blk) = fcx.tcx.hir_node(id) else {
1742-
return false;
1743-
};
1744-
if blk.expr.is_some() || !blk.stmts.is_empty() {
1745-
return false;
1746-
}
1747-
let Some((_, hir::Node::Expr(expr))) =
1748-
fcx.tcx.hir_parent_iter(id).nth(1)
1749-
else {
1750-
return false;
1751-
};
1752-
matches!(expr.kind, hir::ExprKind::If(..))
1753-
};
1754-
if !is_empty_arm(then_id) && !is_empty_arm(else_id) {
1739+
let then_span = fcx.find_block_span_from_hir_id(then_expr.hir_id);
1740+
let else_span = fcx.find_block_span_from_hir_id(else_expr.hir_id);
1741+
// Don't suggest wrapping whole block in `Box::new`.
1742+
if then_span != then_expr.span && else_span != else_expr.span {
1743+
let then_ty = fcx.typeck_results.borrow().expr_ty(then_expr);
1744+
let else_ty = fcx.typeck_results.borrow().expr_ty(else_expr);
17551745
self.suggest_boxing_tail_for_return_position_impl_trait(
17561746
fcx,
17571747
&mut err,

‎compiler/rustc_hir_typeck/src/expr.rs‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -583,7 +583,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
583583
ascribed_ty
584584
}
585585
ExprKind::If(cond, then_expr, opt_else_expr) => {
586-
self.check_expr_if(cond, then_expr, opt_else_expr, expr.span, expected)
586+
self.check_expr_if(expr.hir_id, cond, then_expr, opt_else_expr, expr.span, expected)
587587
}
588588
ExprKind::DropTemps(e) => self.check_expr_with_expectation(e, expected),
589589
ExprKind::Array(args) => self.check_expr_array(args, expected, expr),
@@ -1343,6 +1343,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
13431343
// or 'if-else' expression.
13441344
fn check_expr_if(
13451345
&self,
1346+
expr_id: HirId,
13461347
cond_expr: &'tcx hir::Expr<'tcx>,
13471348
then_expr: &'tcx hir::Expr<'tcx>,
13481349
opt_else_expr: Option<&'tcx hir::Expr<'tcx>>,
@@ -1382,15 +1383,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
13821383

13831384
let tail_defines_return_position_impl_trait =
13841385
self.return_position_impl_trait_from_match_expectation(orig_expected);
1385-
let if_cause = self.if_cause(
1386-
sp,
1387-
cond_expr.span,
1388-
then_expr,
1389-
else_expr,
1390-
then_ty,
1391-
else_ty,
1392-
tail_defines_return_position_impl_trait,
1393-
);
1386+
let if_cause =
1387+
self.if_cause(expr_id, else_expr, tail_defines_return_position_impl_trait);
13941388

13951389
coerce.coerce(self, &if_cause, else_expr, else_ty);
13961390

‎compiler/rustc_infer/src/infer/mod.rs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ use rustc_middle::ty::{
3535
PseudoCanonicalInput, Term, TermKind, Ty, TyCtxt, TyVid, TypeFoldable, TypeFolder,
3636
TypeSuperFoldable, TypeVisitable, TypeVisitableExt, TypingEnv, TypingMode, fold_regions,
3737
};
38-
use rustc_span::{Span, Symbol};
38+
use rustc_span::{DUMMY_SP, Span, Symbol};
3939
use snapshot::undo_log::InferCtxtUndoLogs;
4040
use tracing::{debug, instrument};
4141
use type_variable::TypeVariableOrigin;
@@ -1557,15 +1557,16 @@ impl<'tcx> InferCtxt<'tcx> {
15571557
}
15581558
}
15591559

1560-
/// Given a [`hir::HirId`] for a block, get the span of its last expression
1561-
/// or statement, peeling off any inner blocks.
1560+
/// Given a [`hir::HirId`] for a block (or an expr of a block), get the span
1561+
/// of its last expression or statement, peeling off any inner blocks.
15621562
pub fn find_block_span_from_hir_id(&self, hir_id: hir::HirId) -> Span {
15631563
match self.tcx.hir_node(hir_id) {
1564-
hir::Node::Block(blk) => self.find_block_span(blk),
1565-
// The parser was in a weird state if either of these happen, but
1566-
// it's better not to panic.
1564+
hir::Node::Block(blk)
1565+
| hir::Node::Expr(&hir::Expr { kind: hir::ExprKind::Block(blk, _), .. }) => {
1566+
self.find_block_span(blk)
1567+
}
15671568
hir::Node::Expr(e) => e.span,
1568-
_ => rustc_span::DUMMY_SP,
1569+
_ => DUMMY_SP,
15691570
}
15701571
}
15711572
}

‎compiler/rustc_middle/src/traits/mod.rs‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,11 @@ pub enum ObligationCauseCode<'tcx> {
332332
},
333333

334334
/// Computing common supertype in an if expression
335-
IfExpression(Box<IfExpressionCause<'tcx>>),
335+
IfExpression {
336+
expr_id: HirId,
337+
// Is the expectation of this match expression an RPIT?
338+
tail_defines_return_position_impl_trait: Option<LocalDefId>,
339+
},
336340

337341
/// Computing common supertype of an if expression with no else counter-part
338342
IfExpressionWithNoElse,
@@ -550,18 +554,6 @@ pub struct PatternOriginExpr {
550554
pub peeled_prefix_suggestion_parentheses: bool,
551555
}
552556

553-
#[derive(Copy, Clone, Debug, PartialEq, Eq)]
554-
#[derive(TypeFoldable, TypeVisitable, HashStable, TyEncodable, TyDecodable)]
555-
pub struct IfExpressionCause<'tcx> {
556-
pub then_id: HirId,
557-
pub else_id: HirId,
558-
pub then_ty: Ty<'tcx>,
559-
pub else_ty: Ty<'tcx>,
560-
pub outer_span: Option<Span>,
561-
// Is the expectation of this match expression an RPIT?
562-
pub tail_defines_return_position_impl_trait: Option<LocalDefId>,
563-
}
564-
565557
#[derive(Clone, Debug, PartialEq, Eq, HashStable, TyEncodable, TyDecodable)]
566558
#[derive(TypeVisitable, TypeFoldable)]
567559
pub struct DerivedCause<'tcx> {

‎compiler/rustc_trait_selection/src/error_reporting/infer/mod.rs‎

Lines changed: 46 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -82,9 +82,7 @@ use crate::infer;
8282
use crate::infer::relate::{self, RelateResult, TypeRelation};
8383
use crate::infer::{InferCtxt, InferCtxtExt as _, TypeTrace, ValuePairs};
8484
use crate::solve::deeply_normalize_for_diagnostics;
85-
use crate::traits::{
86-
IfExpressionCause, MatchExpressionArmCause, ObligationCause, ObligationCauseCode,
87-
};
85+
use crate::traits::{MatchExpressionArmCause, ObligationCause, ObligationCauseCode};
8886

8987
mod note_and_explain;
9088
mod suggest;
@@ -613,28 +611,61 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
613611
}
614612
}
615613
},
616-
ObligationCauseCode::IfExpression(box IfExpressionCause {
617-
then_id,
618-
else_id,
619-
then_ty,
620-
else_ty,
621-
outer_span,
622-
..
623-
}) => {
624-
let then_span = self.find_block_span_from_hir_id(then_id);
625-
let else_span = self.find_block_span_from_hir_id(else_id);
626-
if let hir::Node::Expr(e) = self.tcx.hir_node(else_id)
627-
&& let hir::ExprKind::If(_cond, _then, None) = e.kind
614+
ObligationCauseCode::IfExpression { expr_id, .. } => {
615+
let hir::Node::Expr(&hir::Expr {
616+
kind: hir::ExprKind::If(cond_expr, then_expr, Some(else_expr)),
617+
span: expr_span,
618+
..
619+
}) = self.tcx.hir_node(expr_id)
620+
else {
621+
return;
622+
};
623+
let then_span = self.find_block_span_from_hir_id(then_expr.hir_id);
624+
let then_ty = self
625+
.typeck_results
626+
.as_ref()
627+
.expect("if expression only expected inside FnCtxt")
628+
.expr_ty(then_expr);
629+
let else_span = self.find_block_span_from_hir_id(else_expr.hir_id);
630+
let else_ty = self
631+
.typeck_results
632+
.as_ref()
633+
.expect("if expression only expected inside FnCtxt")
634+
.expr_ty(else_expr);
635+
if let hir::ExprKind::If(_cond, _then, None) = else_expr.kind
628636
&& else_ty.is_unit()
629637
{
630638
// Account for `let x = if a { 1 } else if b { 2 };`
631639
err.note("`if` expressions without `else` evaluate to `()`");
632640
err.note("consider adding an `else` block that evaluates to the expected type");
633641
}
634642
err.span_label(then_span, "expected because of this");
643+
644+
let outer_span = if self.tcx.sess.source_map().is_multiline(expr_span) {
645+
if then_span.hi() == expr_span.hi() || else_span.hi() == expr_span.hi() {
646+
// Point at condition only if either block has the same end point as
647+
// the whole expression, since that'll cause awkward overlapping spans.
648+
Some(expr_span.shrink_to_lo().to(cond_expr.peel_drop_temps().span))
649+
} else {
650+
Some(expr_span)
651+
}
652+
} else {
653+
None
654+
};
635655
if let Some(sp) = outer_span {
636656
err.span_label(sp, "`if` and `else` have incompatible types");
637657
}
658+
659+
let then_id = if let hir::ExprKind::Block(then_blk, _) = then_expr.kind {
660+
then_blk.hir_id
661+
} else {
662+
then_expr.hir_id
663+
};
664+
let else_id = if let hir::ExprKind::Block(else_blk, _) = else_expr.kind {
665+
else_blk.hir_id
666+
} else {
667+
else_expr.hir_id
668+
};
638669
if let Some(subdiag) = self.suggest_remove_semi_or_return_binding(
639670
Some(then_id),
640671
then_ty,

0 commit comments

Comments
 (0)