Skip to content

Commit

Permalink
Auto merge of #98360 - estebank:uninit-binding, r=oli-obk
Browse files Browse the repository at this point in the history 
 On partial uninit error point at where we need init

When a binding is declared without a value, borrowck verifies that all
codepaths have *one* assignment to them to initialize them fully. If
there are any cases where a condition can be met that leaves the binding
uninitialized or we attempt to initialize a field of an uninitialized
binding, we emit E0381.

We now look at all the statements that initialize the binding, and use
them to explore branching code paths that *don't* and point at them. If
we find *no* potential places where an assignment to the binding might
be missing, we display the spans of all the existing initializers to
provide some context.

Fix #97956.
  • Loading branch information
@bors
bors committed Jul 7, 2022
2 parents 1517f5d + 2a2df9d commit 9b21131
Show file tree
Hide file tree
Showing 111 changed files with 1,060 additions and 445 deletions.
26 changes: 4 additions & 22 deletions compiler/rustc_borrowck/src/borrowck_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,6 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
err
}

pub(crate) fn cannot_act_on_uninitialized_variable(
&self,
span: Span,
verb: &str,
desc: &str,
) -> DiagnosticBuilder<'cx, ErrorGuaranteed> {
struct_span_err!(
self,
span,
E0381,
"{} of possibly-uninitialized variable: `{}`",
verb,
desc,
)
}

pub(crate) fn cannot_mutably_borrow_multiply(
&self,
new_loan_span: Span,
Expand Down Expand Up @@ -175,8 +159,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
self,
new_loan_span,
E0501,
"cannot borrow {}{} as {} because previous closure \
requires unique access",
"cannot borrow {}{} as {} because previous closure requires unique access",
desc_new,
opt_via,
kind_new,
Expand Down Expand Up @@ -453,9 +436,8 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
self,
closure_span,
E0373,
"{} may outlive the current function, \
but it borrows {}, \
which is owned by the current function",
"{} may outlive the current function, but it borrows {}, which is owned by the current \
function",
closure_kind,
borrowed_path,
);
Expand All @@ -479,7 +461,7 @@ impl<'cx, 'tcx> crate::MirBorrowckCtxt<'cx, 'tcx> {
}

#[rustc_lint_diagnostics]
fn struct_span_err_with_code<S: Into<MultiSpan>>(
pub(crate) fn struct_span_err_with_code<S: Into<MultiSpan>>(
&self,
sp: S,
msg: impl Into<DiagnosticMessage>,
Expand Down
295 changes: 275 additions & 20 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ use either::Either;
use rustc_const_eval::util::CallKind;
use rustc_data_structures::captures::Captures;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan};
use rustc_errors::{
struct_span_err, Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed, MultiSpan,
};
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::{walk_expr, Visitor};
use rustc_hir::{AsyncGeneratorKind, GeneratorKind};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_infer::traits::ObligationCause;
Expand All @@ -18,6 +21,7 @@ use rustc_middle::ty::{
self, subst::Subst, suggest_constraining_type_params, EarlyBinder, PredicateKind, Ty,
};
use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex};
use rustc_span::hygiene::DesugaringKind;
use rustc_span::symbol::sym;
use rustc_span::{BytePos, Span};
use rustc_trait_selection::infer::InferCtxtExt;
Expand Down Expand Up @@ -94,32 +98,20 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
return;
}

let item_msg =
match self.describe_place_with_options(used_place, IncludingDowncast(true)) {
Some(name) => format!("`{}`", name),
None => "value".to_owned(),
};
let mut err = self.cannot_act_on_uninitialized_variable(
let err = self.report_use_of_uninitialized(
mpi,
used_place,
moved_place,
desired_action,
span,
desired_action.as_noun(),
&self
.describe_place_with_options(moved_place, IncludingDowncast(true))
.unwrap_or_else(|| "_".to_owned()),
use_spans,
);
err.span_label(span, format!("use of possibly-uninitialized {}", item_msg));

use_spans.var_span_label_path_only(
&mut err,
format!("{} occurs due to use{}", desired_action.as_noun(), use_spans.describe()),
);

self.buffer_error(err);
} else {
if let Some((reported_place, _)) = self.has_move_error(&move_out_indices) {
if self.prefixes(*reported_place, PrefixSet::All).any(|p| p == used_place) {
debug!(
"report_use_of_moved_or_uninitialized place: error suppressed \
mois={:?}",
"report_use_of_moved_or_uninitialized place: error suppressed mois={:?}",
move_out_indices
);
return;
Expand Down Expand Up @@ -326,6 +318,130 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

fn report_use_of_uninitialized(
&self,
mpi: MovePathIndex,
used_place: PlaceRef<'tcx>,
moved_place: PlaceRef<'tcx>,
desired_action: InitializationRequiringAction,
span: Span,
use_spans: UseSpans<'tcx>,
) -> DiagnosticBuilder<'cx, ErrorGuaranteed> {
// We need all statements in the body where the binding was assigned to to later find all
// the branching code paths where the binding *wasn't* assigned to.
let inits = &self.move_data.init_path_map[mpi];
let move_path = &self.move_data.move_paths[mpi];
let decl_span = self.body.local_decls[move_path.place.local].source_info.span;
let mut spans = vec![];
for init_idx in inits {
let init = &self.move_data.inits[*init_idx];
let span = init.span(&self.body);
if !span.is_dummy() {
spans.push(span);
}
}

let (name, desc) =
match self.describe_place_with_options(moved_place, IncludingDowncast(true)) {
Some(name) => (format!("`{name}`"), format!("`{name}` ")),
None => ("the variable".to_string(), String::new()),
};
let path = match self.describe_place_with_options(used_place, IncludingDowncast(true)) {
Some(name) => format!("`{name}`"),
None => "value".to_string(),
};

// We use the statements were the binding was initialized, and inspect the HIR to look
// for the branching codepaths that aren't covered, to point at them.
let hir_id = self.mir_hir_id();
let map = self.infcx.tcx.hir();
let body_id = map.body_owned_by(hir_id);
let body = map.body(body_id);

let mut visitor = ConditionVisitor { spans: &spans, name: &name, errors: vec![] };
visitor.visit_body(&body);

let isnt_initialized = if let InitializationRequiringAction::PartialAssignment
| InitializationRequiringAction::Assignment = desired_action
{
// The same error is emitted for bindings that are *sometimes* initialized and the ones
// that are *partially* initialized by assigning to a field of an uninitialized
// binding. We differentiate between them for more accurate wording here.
"isn't fully initialized"
} else if spans
.iter()
.filter(|i| {
// We filter these to avoid misleading wording in cases like the following,
// where `x` has an `init`, but it is in the same place we're looking at:
// ```
// let x;
// x += 1;
// ```
!i.contains(span)
// We filter these to avoid incorrect main message on `match-cfg-fake-edges.rs`
&& !visitor
.errors
.iter()
.map(|(sp, _)| *sp)
.any(|sp| span < sp && !sp.contains(span))
})
.count()
== 0
{
"isn't initialized"
} else {
"is possibly-uninitialized"
};

let used = desired_action.as_general_verb_in_past_tense();
let mut err =
struct_span_err!(self, span, E0381, "{used} binding {desc}{isnt_initialized}");
use_spans.var_span_label_path_only(
&mut err,
format!("{} occurs due to use{}", desired_action.as_noun(), use_spans.describe()),
);

if let InitializationRequiringAction::PartialAssignment
| InitializationRequiringAction::Assignment = desired_action
{
err.help(
"partial initialization isn't supported, fully initialize the binding with a \
default value and mutate it, or use `std::mem::MaybeUninit`",
);
}
err.span_label(span, format!("{path} {used} here but it {isnt_initialized}"));

let mut shown = false;
for (sp, label) in visitor.errors {
if sp < span && !sp.overlaps(span) {
// When we have a case like `match-cfg-fake-edges.rs`, we don't want to mention
// match arms coming after the primary span because they aren't relevant:
// ```
// let x;
// match y {
// _ if { x = 2; true } => {}
// _ if {
// x; //~ ERROR
// false
// } => {}
// _ => {} // We don't want to point to this.
// };
// ```
err.span_label(sp, &label);
shown = true;
}
}
if !shown {
for sp in &spans {
if *sp < span && !sp.overlaps(span) {
err.span_label(*sp, "binding initialized here in some conditions");
}
}
}
err.span_label(decl_span, "binding declared here but left uninitialized");
err
}

fn suggest_borrow_fn_like(
&self,
err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>,
Expand Down Expand Up @@ -2448,3 +2564,142 @@ impl<'tcx> AnnotatedBorrowFnSignature<'tcx> {
}
}
}

/// Detect whether one of the provided spans is a statement nested within the top-most visited expr
struct ReferencedStatementsVisitor<'a>(&'a [Span], bool);

impl<'a, 'v> Visitor<'v> for ReferencedStatementsVisitor<'a> {
fn visit_stmt(&mut self, s: &'v hir::Stmt<'v>) {
match s.kind {
hir::StmtKind::Semi(expr) if self.0.contains(&expr.span) => {
self.1 = true;
}
_ => {}
}
}
}

/// Given a set of spans representing statements initializing the relevant binding, visit all the
/// function expressions looking for branching code paths that *do not* initialize the binding.
struct ConditionVisitor<'b> {
spans: &'b [Span],
name: &'b str,
errors: Vec<(Span, String)>,
}

impl<'b, 'v> Visitor<'v> for ConditionVisitor<'b> {
fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
match ex.kind {
hir::ExprKind::If(cond, body, None) => {
// `if` expressions with no `else` that initialize the binding might be missing an
// `else` arm.
let mut v = ReferencedStatementsVisitor(self.spans, false);
v.visit_expr(body);
if v.1 {
self.errors.push((
cond.span,
format!(
"if this `if` condition is `false`, {} is not initialized",
self.name,
),
));
self.errors.push((
ex.span.shrink_to_hi(),
format!("an `else` arm might be missing here, initializing {}", self.name),
));
}
}
hir::ExprKind::If(cond, body, Some(other)) => {
// `if` expressions where the binding is only initialized in one of the two arms
// might be missing a binding initialization.
let mut a = ReferencedStatementsVisitor(self.spans, false);
a.visit_expr(body);
let mut b = ReferencedStatementsVisitor(self.spans, false);
b.visit_expr(other);
match (a.1, b.1) {
(true, true) | (false, false) => {}
(true, false) => {
if other.span.is_desugaring(DesugaringKind::WhileLoop) {
self.errors.push((
cond.span,
format!(
"if this condition isn't met and the `while` loop runs 0 \
times, {} is not initialized",
self.name
),
));
} else {
self.errors.push((
body.span.shrink_to_hi().until(other.span),
format!(
"if the `if` condition is `false` and this `else` arm is \
executed, {} is not initialized",
self.name
),
));
}
}
(false, true) => {
self.errors.push((
cond.span,
format!(
"if this condition is `true`, {} is not initialized",
self.name
),
));
}
}
}
hir::ExprKind::Match(e, arms, loop_desugar) => {
// If the binding is initialized in one of the match arms, then the other match
// arms might be missing an initialization.
let results: Vec<bool> = arms
.iter()
.map(|arm| {
let mut v = ReferencedStatementsVisitor(self.spans, false);
v.visit_arm(arm);
v.1
})
.collect();
if results.iter().any(|x| *x) && !results.iter().all(|x| *x) {
for (arm, seen) in arms.iter().zip(results) {
if !seen {
if loop_desugar == hir::MatchSource::ForLoopDesugar {
self.errors.push((
e.span,
format!(
"if the `for` loop runs 0 times, {} is not initialized ",
self.name
),
));
} else if let Some(guard) = &arm.guard {
self.errors.push((
arm.pat.span.to(guard.body().span),
format!(
"if this pattern and condition are matched, {} is not \
initialized",
self.name
),
));
} else {
self.errors.push((
arm.pat.span,
format!(
"if this pattern is matched, {} is not initialized",
self.name
),
));
}
}
}
}
}
// FIXME: should we also account for binops, particularly `&&` and `||`? `try` should
// also be accounted for. For now it is fine, as if we don't find *any* relevant
// branching code paths, we point at the places where the binding *is* initialized for
// *some* context.
_ => {}
}
walk_expr(self, ex);
}
}
Loading

0 comments on commit 9b21131

Please sign in to comment.