Skip to content

Commit

Permalink
Rollup merge of rust-lang#81062 - sexxi-goose:precise_capture_diagnos…
Browse files Browse the repository at this point in the history
…tics, r=nikomatsakis

Improve diagnostics for Precise Capture

This is just the capture analysis part and borrow checker logging will updated as part of rust-lang/project-rfc-2229#8

Closes rust-lang/project-rfc-2229#22
  • Loading branch information
Dylan-DPC authored Jan 27, 2021
2 parents f37d9d0 + cf71d83 commit 8879a90
Show file tree
Hide file tree
Showing 18 changed files with 773 additions and 31 deletions.
21 changes: 19 additions & 2 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -780,8 +780,20 @@ pub fn place_to_string_for_capture(tcx: TyCtxt<'tcx>, place: &HirPlace<'tcx>) ->
pub struct CaptureInfo<'tcx> {
/// Expr Id pointing to use that resulted in selecting the current capture kind
///
/// Eg:
/// ```rust,no_run
/// let mut t = (0,1);
///
/// let c = || {
/// println!("{}",t); // L1
/// t.1 = 4; // L2
/// };
/// ```
/// `capture_kind_expr_id` will point to the use on L2 and `path_expr_id` will point to the
/// use on L1.
///
/// If the user doesn't enable feature `capture_disjoint_fields` (RFC 2229) then, it is
/// possible that we don't see the use of a particular place resulting in expr_id being
/// possible that we don't see the use of a particular place resulting in capture_kind_expr_id being
/// None. In such case we fallback on uvpars_mentioned for span.
///
/// Eg:
Expand All @@ -795,7 +807,12 @@ pub struct CaptureInfo<'tcx> {
///
/// In this example, if `capture_disjoint_fields` is **not** set, then x will be captured,
/// but we won't see it being used during capture analysis, since it's essentially a discard.
pub expr_id: Option<hir::HirId>,
pub capture_kind_expr_id: Option<hir::HirId>,
/// Expr Id pointing to use that resulted the corresponding place being captured
///
/// See `capture_kind_expr_id` for example.
///
pub path_expr_id: Option<hir::HirId>,

/// Capture mode that was selected
pub capture_kind: UpvarCapture<'tcx>,
Expand Down
149 changes: 124 additions & 25 deletions compiler/rustc_typeck/src/check/upvar.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ use rustc_infer::infer::UpvarRegion;
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
use rustc_middle::ty::{self, Ty, TyCtxt, UpvarSubsts};
use rustc_span::sym;
use rustc_span::{Span, Symbol};
use rustc_span::{MultiSpan, Span, Symbol};

/// Describe the relationship between the paths of two places
/// eg:
Expand Down Expand Up @@ -135,7 +135,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let upvar_id = ty::UpvarId::new(var_hir_id, local_def_id);
let capture_kind = self.init_capture_kind(capture_clause, upvar_id, span);
let info = ty::CaptureInfo { expr_id: None, capture_kind };
let info = ty::CaptureInfo {
capture_kind_expr_id: None,
path_expr_id: None,
capture_kind,
};

capture_information.insert(place, info);
}
Expand Down Expand Up @@ -308,8 +312,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if let Some(capture_kind) = upvar_capture_map.get(&upvar_id) {
// upvar_capture_map only stores the UpvarCapture (CaptureKind),
// so we create a fake capture info with no expression.
let fake_capture_info =
ty::CaptureInfo { expr_id: None, capture_kind: *capture_kind };
let fake_capture_info = ty::CaptureInfo {
capture_kind_expr_id: None,
path_expr_id: None,
capture_kind: *capture_kind,
};
determine_capture_info(fake_capture_info, capture_info).capture_kind
} else {
capture_info.capture_kind
Expand Down Expand Up @@ -359,20 +366,44 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
///
/// ```
/// {
/// Place(base: hir_id_s, projections: [], ....) -> (hir_id_L5, ByValue),
/// Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> (hir_id_L2, ByRef(MutBorrow))
/// Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> (hir_id_L3, ByRef(ImmutBorrow))
/// Place(base: hir_id_p, projections: [], ...) -> (hir_id_L4, ByRef(ImmutBorrow))
/// Place(base: hir_id_s, projections: [], ....) -> {
/// capture_kind_expr: hir_id_L5,
/// path_expr_id: hir_id_L5,
/// capture_kind: ByValue
/// },
/// Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> {
/// capture_kind_expr: hir_id_L2,
/// path_expr_id: hir_id_L2,
/// capture_kind: ByValue
/// },
/// Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> {
/// capture_kind_expr: hir_id_L3,
/// path_expr_id: hir_id_L3,
/// capture_kind: ByValue
/// },
/// Place(base: hir_id_p, projections: [], ...) -> {
/// capture_kind_expr: hir_id_L4,
/// path_expr_id: hir_id_L4,
/// capture_kind: ByValue
/// },
/// ```
///
/// After the min capture analysis, we get:
/// ```
/// {
/// hir_id_s -> [
/// Place(base: hir_id_s, projections: [], ....) -> (hir_id_L4, ByValue)
/// Place(base: hir_id_s, projections: [], ....) -> {
/// capture_kind_expr: hir_id_L5,
/// path_expr_id: hir_id_L5,
/// capture_kind: ByValue
/// },
/// ],
/// hir_id_p -> [
/// Place(base: hir_id_p, projections: [], ...) -> (hir_id_L2, ByRef(MutBorrow)),
/// Place(base: hir_id_p, projections: [], ...) -> {
/// capture_kind_expr: hir_id_L2,
/// path_expr_id: hir_id_L4,
/// capture_kind: ByValue
/// },
/// ],
/// ```
fn compute_min_captures(
Expand Down Expand Up @@ -425,8 +456,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// current place is ancestor of possible_descendant
PlaceAncestryRelation::Ancestor => {
descendant_found = true;
let backup_path_expr_id = updated_capture_info.path_expr_id;

updated_capture_info =
determine_capture_info(updated_capture_info, possible_descendant.info);

// we need to keep the ancestor's `path_expr_id`
updated_capture_info.path_expr_id = backup_path_expr_id;
false
}

Expand All @@ -441,9 +477,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// current place is descendant of possible_ancestor
PlaceAncestryRelation::Descendant => {
ancestor_found = true;
let backup_path_expr_id = possible_ancestor.info.path_expr_id;
possible_ancestor.info =
determine_capture_info(possible_ancestor.info, capture_info);

// we need to keep the ancestor's `path_expr_id`
possible_ancestor.info.path_expr_id = backup_path_expr_id;

// Only one ancestor of the current place will be in the list.
break;
}
Expand Down Expand Up @@ -518,7 +558,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let capture_str = construct_capture_info_string(self.tcx, place, capture_info);
let output_str = format!("Capturing {}", capture_str);

let span = capture_info.expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
let span =
capture_info.path_expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
diag.span_note(span, &output_str);
}
diag.emit();
Expand All @@ -542,9 +583,32 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
construct_capture_info_string(self.tcx, place, capture_info);
let output_str = format!("Min Capture {}", capture_str);

let span =
capture_info.expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
diag.span_note(span, &output_str);
if capture.info.path_expr_id != capture.info.capture_kind_expr_id {
let path_span = capture_info
.path_expr_id
.map_or(closure_span, |e| self.tcx.hir().span(e));
let capture_kind_span = capture_info
.capture_kind_expr_id
.map_or(closure_span, |e| self.tcx.hir().span(e));

let mut multi_span: MultiSpan =
MultiSpan::from_spans(vec![path_span, capture_kind_span]);

let capture_kind_label =
construct_capture_kind_reason_string(self.tcx, place, capture_info);
let path_label = construct_path_string(self.tcx, place);

multi_span.push_span_label(path_span, path_label);
multi_span.push_span_label(capture_kind_span, capture_kind_label);

diag.span_note(multi_span, &output_str);
} else {
let span = capture_info
.path_expr_id
.map_or(closure_span, |e| self.tcx.hir().span(e));

diag.span_note(span, &output_str);
};
}
}
diag.emit();
Expand Down Expand Up @@ -642,7 +706,8 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
);

let capture_info = ty::CaptureInfo {
expr_id: Some(diag_expr_id),
capture_kind_expr_id: Some(diag_expr_id),
path_expr_id: Some(diag_expr_id),
capture_kind: ty::UpvarCapture::ByValue(Some(usage_span)),
};

Expand Down Expand Up @@ -762,7 +827,8 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
let new_upvar_borrow = ty::UpvarBorrow { kind, region: curr_upvar_borrow.region };

let capture_info = ty::CaptureInfo {
expr_id: Some(diag_expr_id),
capture_kind_expr_id: Some(diag_expr_id),
path_expr_id: Some(diag_expr_id),
capture_kind: ty::UpvarCapture::ByRef(new_upvar_borrow),
};
let updated_info = determine_capture_info(curr_capture_info, capture_info);
Expand Down Expand Up @@ -824,7 +890,11 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
self.fcx.init_capture_kind(self.capture_clause, upvar_id, self.closure_span);

let expr_id = Some(diag_expr_id);
let capture_info = ty::CaptureInfo { expr_id, capture_kind };
let capture_info = ty::CaptureInfo {
capture_kind_expr_id: expr_id,
path_expr_id: expr_id,
capture_kind,
};

debug!("Capturing new place {:?}, capture_info={:?}", place_with_id, capture_info);

Expand Down Expand Up @@ -890,11 +960,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
}
}

fn construct_capture_info_string(
tcx: TyCtxt<'_>,
place: &Place<'tcx>,
capture_info: &ty::CaptureInfo<'tcx>,
) -> String {
fn construct_place_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
let variable_name = match place.base {
PlaceBase::Upvar(upvar_id) => var_name(tcx, upvar_id.var_path.hir_id).to_string(),
_ => bug!("Capture_information should only contain upvars"),
Expand All @@ -914,11 +980,42 @@ fn construct_capture_info_string(
projections_str.push_str(proj.as_str());
}

format!("{}[{}]", variable_name, projections_str)
}

fn construct_capture_kind_reason_string(
tcx: TyCtxt<'_>,
place: &Place<'tcx>,
capture_info: &ty::CaptureInfo<'tcx>,
) -> String {
let place_str = construct_place_string(tcx, &place);

let capture_kind_str = match capture_info.capture_kind {
ty::UpvarCapture::ByValue(_) => "ByValue".into(),
ty::UpvarCapture::ByRef(borrow) => format!("{:?}", borrow.kind),
};
format!("{}[{}] -> {}", variable_name, projections_str, capture_kind_str)

format!("{} captured as {} here", place_str, capture_kind_str)
}

fn construct_path_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
let place_str = construct_place_string(tcx, &place);

format!("{} used here", place_str)
}

fn construct_capture_info_string(
tcx: TyCtxt<'_>,
place: &Place<'tcx>,
capture_info: &ty::CaptureInfo<'tcx>,
) -> String {
let place_str = construct_place_string(tcx, &place);

let capture_kind_str = match capture_info.capture_kind {
ty::UpvarCapture::ByValue(_) => "ByValue".into(),
ty::UpvarCapture::ByRef(borrow) => format!("{:?}", borrow.kind),
};
format!("{} -> {}", place_str, capture_kind_str)
}

fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
Expand All @@ -930,7 +1027,9 @@ fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
/// (Note: CaptureInfo contains CaptureKind and an expression that led to capture it in that way)
///
/// If both `CaptureKind`s are considered equivalent, then the CaptureInfo is selected based
/// on the `CaptureInfo` containing an associated expression id.
/// on the `CaptureInfo` containing an associated `capture_kind_expr_id`.
///
/// It is the caller's duty to figure out which path_expr_id to use.
///
/// If both the CaptureKind and Expression are considered to be equivalent,
/// then `CaptureInfo` A is preferred. This can be useful in cases where we want to priortize
Expand Down Expand Up @@ -981,7 +1080,7 @@ fn determine_capture_info(
};

if eq_capture_kind {
match (capture_info_a.expr_id, capture_info_b.expr_id) {
match (capture_info_a.capture_kind_expr_id, capture_info_b.capture_kind_expr_id) {
(Some(_), _) | (None, None) => capture_info_a,
(None, Some(_)) => capture_info_b,
}
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/check/writeback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
let min_list_wb = min_list
.iter()
.map(|captured_place| {
let locatable = captured_place.info.expr_id.unwrap_or(
let locatable = captured_place.info.path_expr_id.unwrap_or(
self.tcx().hir().local_def_id_to_hir_id(closure_def_id.expect_local()),
);

Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_typeck/src/expr_use_visitor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,7 +630,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
PlaceBase::Local(*var_hir_id)
};
let place_with_id = PlaceWithHirId::new(
capture_info.expr_id.unwrap_or(closure_expr.hir_id),
capture_info.path_expr_id.unwrap_or(closure_expr.hir_id),
place.base_ty,
place_base,
place.projections.clone(),
Expand Down
35 changes: 35 additions & 0 deletions src/test/ui/closures/2229_closure_analysis/capture-analysis-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
#![feature(capture_disjoint_fields)]
//~^ WARNING: the feature `capture_disjoint_fields` is incomplete
//~| NOTE: `#[warn(incomplete_features)]` on by default
//~| NOTE: see issue #53488 <https://github.com/rust-lang/rust/issues/53488>
#![feature(rustc_attrs)]

#[derive(Debug)]
struct Point {
x: i32,
y: i32,
}

fn main() {
let p = Point { x: 10, y: 10 };
let q = Point { x: 10, y: 10 };

let c = #[rustc_capture_analysis]
//~^ ERROR: attributes on expressions are experimental
//~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
|| {
//~^ First Pass analysis includes:
//~| Min Capture analysis includes:
println!("{:?}", p);
//~^ NOTE: Capturing p[] -> ImmBorrow
//~| NOTE: Min Capture p[] -> ImmBorrow
println!("{:?}", p.x);
//~^ NOTE: Capturing p[(0, 0)] -> ImmBorrow

println!("{:?}", q.x);
//~^ NOTE: Capturing q[(0, 0)] -> ImmBorrow
println!("{:?}", q);
//~^ NOTE: Capturing q[] -> ImmBorrow
//~| NOTE: Min Capture q[] -> ImmBorrow
};
}
Loading

0 comments on commit 8879a90

Please sign in to comment.