Skip to content
New issue

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

#

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

Already on GitHub? # to your account

typeck: simplify the handling of diverges #68422

Merged
merged 3 commits into from
Jan 22, 2020
Merged

typeck: simplify the handling of diverges #68422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d47673d
typeck: use diverges.replace(..)
Centril Jan 21, 2020
1240a31
typeck: remove redundant diverges code
Centril Jan 21, 2020
7dceff9
typeck: remove redundant diverges check
Centril Jan 21, 2020
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
55 changes: 18 additions & 37 deletions src/librustc_typeck/check/_match.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,30 +50,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

self.warn_arms_when_scrutinee_diverges(arms, match_src);

// Otherwise, we have to union together the types that the
// arms produce and so forth.
let scrut_diverges = self.diverges.get();
self.diverges.set(Diverges::Maybe);
// Otherwise, we have to union together the types that the arms produce and so forth.
let scrut_diverges = self.diverges.replace(Diverges::Maybe);
Copy link
Member

Choose a reason for hiding this comment

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

Ohh, we might have more similar places where we use get and set, predating Cell::replace.
I found some in these two files, if someone wants to take them:

  • src/librustc/ty/print/pretty.rs
  • src/librustc/infer/mod.rs

(there's others, but they use the result of get() to compute the value to set, and I don't think we have a nice abstraction for that)


// rust-lang/rust#55810: Typecheck patterns first (via eager
// collection into `Vec`), so we get types for all bindings.
let all_arm_pats_diverge: Vec<_> = arms
.iter()
.map(|arm| {
let mut all_pats_diverge = Diverges::WarnedAlways;
self.diverges.set(Diverges::Maybe);
self.check_pat_top(&arm.pat, scrut_ty, Some(scrut.span), true);
all_pats_diverge &= self.diverges.get();

// As discussed with @eddyb, this is for disabling unreachable_code
// warnings on patterns (they're now subsumed by unreachable_patterns
// warnings).
match all_pats_diverge {
Diverges::Maybe => Diverges::Maybe,
Diverges::Always { .. } | Diverges::WarnedAlways => Diverges::WarnedAlways,
}
})
.collect();
// #55810: Type check patterns first so we get types for all bindings.
for arm in arms {
self.check_pat_top(&arm.pat, scrut_ty, Some(scrut.span), true);
}
Copy link
Member

Choose a reason for hiding this comment

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

I assume "they're now subsumed by unreachable_patterns warnings" in the deleted comment is why you can do this change now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #68429.

(To recap what I said in private: check/pat.rs does not mention diverges, except indirectly by type checking expressions, which only occur in literals and range patterns, but those cannot be typed at a diverging type, so removing this code has no effect.)


// Now typecheck the blocks.
//
Expand Down Expand Up @@ -104,19 +87,19 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
CoerceMany::with_coercion_sites(coerce_first, arms)
};

let mut other_arms = vec![]; // used only for diagnostics
let mut other_arms = vec![]; // Used only for diagnostics.
let mut prior_arm_ty = None;
for (i, (arm, pats_diverge)) in arms.iter().zip(all_arm_pats_diverge).enumerate() {
for (i, arm) in arms.iter().enumerate() {
if let Some(g) = &arm.guard {
self.diverges.set(pats_diverge);
self.diverges.set(Diverges::Maybe);
match g {
hir::Guard::If(e) => {
self.check_expr_has_type_or_error(e, tcx.types.bool, |_| {})
}
};
}

self.diverges.set(pats_diverge);
self.diverges.set(Diverges::Maybe);
let arm_ty = if source_if
&& if_no_else
&& i != 0
Expand Down Expand Up @@ -200,16 +183,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
arms: &'tcx [hir::Arm<'tcx>],
source: hir::MatchSource,
) {
if self.diverges.get().is_always() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this was a performance microopt?

use hir::MatchSource::*;
let msg = match source {
IfDesugar { .. } | IfLetDesugar { .. } => "block in `if` expression",
WhileDesugar { .. } | WhileLetDesugar { .. } => "block in `while` expression",
_ => "arm",
};
for arm in arms {
self.warn_if_unreachable(arm.body.hir_id, arm.body.span, msg);
}
use hir::MatchSource::*;
let msg = match source {
IfDesugar { .. } | IfLetDesugar { .. } => "block in `if` expression",
WhileDesugar { .. } | WhileLetDesugar { .. } => "block in `while` expression",
_ => "arm",
};
for arm in arms {
self.warn_if_unreachable(arm.body.hir_id, arm.body.span, msg);
}
}

Expand Down
6 changes: 2 additions & 4 deletions src/librustc_typeck/check/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

// Hide the outer diverging and has_errors flags.
let old_diverges = self.diverges.get();
let old_has_errors = self.has_errors.get();
self.diverges.set(Diverges::Maybe);
self.has_errors.set(false);
let old_diverges = self.diverges.replace(Diverges::Maybe);
let old_has_errors = self.has_errors.replace(false);

let ty = self.check_expr_kind(expr, expected, needs);

Expand Down
7 changes: 2 additions & 5 deletions src/librustc_typeck/check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4418,10 +4418,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.warn_if_unreachable(stmt.hir_id, stmt.span, "statement");

// Hide the outer diverging and `has_errors` flags.
let old_diverges = self.diverges.get();
let old_has_errors = self.has_errors.get();
self.diverges.set(Diverges::Maybe);
self.has_errors.set(false);
let old_diverges = self.diverges.replace(Diverges::Maybe);
let old_has_errors = self.has_errors.replace(false);

match stmt.kind {
hir::StmtKind::Local(ref l) => {
Expand All @@ -4431,7 +4429,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
hir::StmtKind::Item(_) => {}
hir::StmtKind::Expr(ref expr) => {
// Check with expected type of `()`.

self.check_expr_has_type_or_error(&expr, self.tcx.mk_unit(), |err| {
self.suggest_semicolon_at_end(expr.span, err);
});
Expand Down