Skip to content

Commit 6e50c94

Browse files
Dylan-DPCDylan-DPC
authored
Rollup merge of rust-lang#81062 - sexxi-goose:precise_capture_diagnostics, 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
2 parents e5cf216 + cf71d83 commit 6e50c94

18 files changed

+773
-31
lines changed

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

+19-2
Original file line numberDiff line numberDiff line change
@@ -780,8 +780,20 @@ pub fn place_to_string_for_capture(tcx: TyCtxt<'tcx>, place: &HirPlace<'tcx>) ->
780780
pub struct CaptureInfo<'tcx> {
781781
/// Expr Id pointing to use that resulted in selecting the current capture kind
782782
///
783+
/// Eg:
784+
/// ```rust,no_run
785+
/// let mut t = (0,1);
786+
///
787+
/// let c = || {
788+
/// println!("{}",t); // L1
789+
/// t.1 = 4; // L2
790+
/// };
791+
/// ```
792+
/// `capture_kind_expr_id` will point to the use on L2 and `path_expr_id` will point to the
793+
/// use on L1.
794+
///
783795
/// If the user doesn't enable feature `capture_disjoint_fields` (RFC 2229) then, it is
784-
/// possible that we don't see the use of a particular place resulting in expr_id being
796+
/// possible that we don't see the use of a particular place resulting in capture_kind_expr_id being
785797
/// None. In such case we fallback on uvpars_mentioned for span.
786798
///
787799
/// Eg:
@@ -795,7 +807,12 @@ pub struct CaptureInfo<'tcx> {
795807
///
796808
/// In this example, if `capture_disjoint_fields` is **not** set, then x will be captured,
797809
/// but we won't see it being used during capture analysis, since it's essentially a discard.
798-
pub expr_id: Option<hir::HirId>,
810+
pub capture_kind_expr_id: Option<hir::HirId>,
811+
/// Expr Id pointing to use that resulted the corresponding place being captured
812+
///
813+
/// See `capture_kind_expr_id` for example.
814+
///
815+
pub path_expr_id: Option<hir::HirId>,
799816

800817
/// Capture mode that was selected
801818
pub capture_kind: UpvarCapture<'tcx>,

‎compiler/rustc_typeck/src/check/upvar.rs

+124-25
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ use rustc_infer::infer::UpvarRegion;
4242
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
4343
use rustc_middle::ty::{self, Ty, TyCtxt, UpvarSubsts};
4444
use rustc_span::sym;
45-
use rustc_span::{Span, Symbol};
45+
use rustc_span::{MultiSpan, Span, Symbol};
4646

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

136136
let upvar_id = ty::UpvarId::new(var_hir_id, local_def_id);
137137
let capture_kind = self.init_capture_kind(capture_clause, upvar_id, span);
138-
let info = ty::CaptureInfo { expr_id: None, capture_kind };
138+
let info = ty::CaptureInfo {
139+
capture_kind_expr_id: None,
140+
path_expr_id: None,
141+
capture_kind,
142+
};
139143

140144
capture_information.insert(place, info);
141145
}
@@ -308,8 +312,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
308312
if let Some(capture_kind) = upvar_capture_map.get(&upvar_id) {
309313
// upvar_capture_map only stores the UpvarCapture (CaptureKind),
310314
// so we create a fake capture info with no expression.
311-
let fake_capture_info =
312-
ty::CaptureInfo { expr_id: None, capture_kind: *capture_kind };
315+
let fake_capture_info = ty::CaptureInfo {
316+
capture_kind_expr_id: None,
317+
path_expr_id: None,
318+
capture_kind: *capture_kind,
319+
};
313320
determine_capture_info(fake_capture_info, capture_info).capture_kind
314321
} else {
315322
capture_info.capture_kind
@@ -359,20 +366,44 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
359366
///
360367
/// ```
361368
/// {
362-
/// Place(base: hir_id_s, projections: [], ....) -> (hir_id_L5, ByValue),
363-
/// Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> (hir_id_L2, ByRef(MutBorrow))
364-
/// Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> (hir_id_L3, ByRef(ImmutBorrow))
365-
/// Place(base: hir_id_p, projections: [], ...) -> (hir_id_L4, ByRef(ImmutBorrow))
369+
/// Place(base: hir_id_s, projections: [], ....) -> {
370+
/// capture_kind_expr: hir_id_L5,
371+
/// path_expr_id: hir_id_L5,
372+
/// capture_kind: ByValue
373+
/// },
374+
/// Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> {
375+
/// capture_kind_expr: hir_id_L2,
376+
/// path_expr_id: hir_id_L2,
377+
/// capture_kind: ByValue
378+
/// },
379+
/// Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> {
380+
/// capture_kind_expr: hir_id_L3,
381+
/// path_expr_id: hir_id_L3,
382+
/// capture_kind: ByValue
383+
/// },
384+
/// Place(base: hir_id_p, projections: [], ...) -> {
385+
/// capture_kind_expr: hir_id_L4,
386+
/// path_expr_id: hir_id_L4,
387+
/// capture_kind: ByValue
388+
/// },
366389
/// ```
367390
///
368391
/// After the min capture analysis, we get:
369392
/// ```
370393
/// {
371394
/// hir_id_s -> [
372-
/// Place(base: hir_id_s, projections: [], ....) -> (hir_id_L4, ByValue)
395+
/// Place(base: hir_id_s, projections: [], ....) -> {
396+
/// capture_kind_expr: hir_id_L5,
397+
/// path_expr_id: hir_id_L5,
398+
/// capture_kind: ByValue
399+
/// },
373400
/// ],
374401
/// hir_id_p -> [
375-
/// Place(base: hir_id_p, projections: [], ...) -> (hir_id_L2, ByRef(MutBorrow)),
402+
/// Place(base: hir_id_p, projections: [], ...) -> {
403+
/// capture_kind_expr: hir_id_L2,
404+
/// path_expr_id: hir_id_L4,
405+
/// capture_kind: ByValue
406+
/// },
376407
/// ],
377408
/// ```
378409
fn compute_min_captures(
@@ -425,8 +456,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
425456
// current place is ancestor of possible_descendant
426457
PlaceAncestryRelation::Ancestor => {
427458
descendant_found = true;
459+
let backup_path_expr_id = updated_capture_info.path_expr_id;
460+
428461
updated_capture_info =
429462
determine_capture_info(updated_capture_info, possible_descendant.info);
463+
464+
// we need to keep the ancestor's `path_expr_id`
465+
updated_capture_info.path_expr_id = backup_path_expr_id;
430466
false
431467
}
432468

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

484+
// we need to keep the ancestor's `path_expr_id`
485+
possible_ancestor.info.path_expr_id = backup_path_expr_id;
486+
447487
// Only one ancestor of the current place will be in the list.
448488
break;
449489
}
@@ -518,7 +558,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
518558
let capture_str = construct_capture_info_string(self.tcx, place, capture_info);
519559
let output_str = format!("Capturing {}", capture_str);
520560

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

545-
let span =
546-
capture_info.expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
547-
diag.span_note(span, &output_str);
586+
if capture.info.path_expr_id != capture.info.capture_kind_expr_id {
587+
let path_span = capture_info
588+
.path_expr_id
589+
.map_or(closure_span, |e| self.tcx.hir().span(e));
590+
let capture_kind_span = capture_info
591+
.capture_kind_expr_id
592+
.map_or(closure_span, |e| self.tcx.hir().span(e));
593+
594+
let mut multi_span: MultiSpan =
595+
MultiSpan::from_spans(vec![path_span, capture_kind_span]);
596+
597+
let capture_kind_label =
598+
construct_capture_kind_reason_string(self.tcx, place, capture_info);
599+
let path_label = construct_path_string(self.tcx, place);
600+
601+
multi_span.push_span_label(path_span, path_label);
602+
multi_span.push_span_label(capture_kind_span, capture_kind_label);
603+
604+
diag.span_note(multi_span, &output_str);
605+
} else {
606+
let span = capture_info
607+
.path_expr_id
608+
.map_or(closure_span, |e| self.tcx.hir().span(e));
609+
610+
diag.span_note(span, &output_str);
611+
};
548612
}
549613
}
550614
diag.emit();
@@ -642,7 +706,8 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
642706
);
643707

644708
let capture_info = ty::CaptureInfo {
645-
expr_id: Some(diag_expr_id),
709+
capture_kind_expr_id: Some(diag_expr_id),
710+
path_expr_id: Some(diag_expr_id),
646711
capture_kind: ty::UpvarCapture::ByValue(Some(usage_span)),
647712
};
648713

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

764829
let capture_info = ty::CaptureInfo {
765-
expr_id: Some(diag_expr_id),
830+
capture_kind_expr_id: Some(diag_expr_id),
831+
path_expr_id: Some(diag_expr_id),
766832
capture_kind: ty::UpvarCapture::ByRef(new_upvar_borrow),
767833
};
768834
let updated_info = determine_capture_info(curr_capture_info, capture_info);
@@ -824,7 +890,11 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
824890
self.fcx.init_capture_kind(self.capture_clause, upvar_id, self.closure_span);
825891

826892
let expr_id = Some(diag_expr_id);
827-
let capture_info = ty::CaptureInfo { expr_id, capture_kind };
893+
let capture_info = ty::CaptureInfo {
894+
capture_kind_expr_id: expr_id,
895+
path_expr_id: expr_id,
896+
capture_kind,
897+
};
828898

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

@@ -890,11 +960,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
890960
}
891961
}
892962

893-
fn construct_capture_info_string(
894-
tcx: TyCtxt<'_>,
895-
place: &Place<'tcx>,
896-
capture_info: &ty::CaptureInfo<'tcx>,
897-
) -> String {
963+
fn construct_place_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
898964
let variable_name = match place.base {
899965
PlaceBase::Upvar(upvar_id) => var_name(tcx, upvar_id.var_path.hir_id).to_string(),
900966
_ => bug!("Capture_information should only contain upvars"),
@@ -914,11 +980,42 @@ fn construct_capture_info_string(
914980
projections_str.push_str(proj.as_str());
915981
}
916982

983+
format!("{}[{}]", variable_name, projections_str)
984+
}
985+
986+
fn construct_capture_kind_reason_string(
987+
tcx: TyCtxt<'_>,
988+
place: &Place<'tcx>,
989+
capture_info: &ty::CaptureInfo<'tcx>,
990+
) -> String {
991+
let place_str = construct_place_string(tcx, &place);
992+
917993
let capture_kind_str = match capture_info.capture_kind {
918994
ty::UpvarCapture::ByValue(_) => "ByValue".into(),
919995
ty::UpvarCapture::ByRef(borrow) => format!("{:?}", borrow.kind),
920996
};
921-
format!("{}[{}] -> {}", variable_name, projections_str, capture_kind_str)
997+
998+
format!("{} captured as {} here", place_str, capture_kind_str)
999+
}
1000+
1001+
fn construct_path_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
1002+
let place_str = construct_place_string(tcx, &place);
1003+
1004+
format!("{} used here", place_str)
1005+
}
1006+
1007+
fn construct_capture_info_string(
1008+
tcx: TyCtxt<'_>,
1009+
place: &Place<'tcx>,
1010+
capture_info: &ty::CaptureInfo<'tcx>,
1011+
) -> String {
1012+
let place_str = construct_place_string(tcx, &place);
1013+
1014+
let capture_kind_str = match capture_info.capture_kind {
1015+
ty::UpvarCapture::ByValue(_) => "ByValue".into(),
1016+
ty::UpvarCapture::ByRef(borrow) => format!("{:?}", borrow.kind),
1017+
};
1018+
format!("{} -> {}", place_str, capture_kind_str)
9221019
}
9231020

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

9831082
if eq_capture_kind {
984-
match (capture_info_a.expr_id, capture_info_b.expr_id) {
1083+
match (capture_info_a.capture_kind_expr_id, capture_info_b.capture_kind_expr_id) {
9851084
(Some(_), _) | (None, None) => capture_info_a,
9861085
(None, Some(_)) => capture_info_b,
9871086
}

‎compiler/rustc_typeck/src/check/writeback.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -348,7 +348,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
348348
let min_list_wb = min_list
349349
.iter()
350350
.map(|captured_place| {
351-
let locatable = captured_place.info.expr_id.unwrap_or(
351+
let locatable = captured_place.info.path_expr_id.unwrap_or(
352352
self.tcx().hir().local_def_id_to_hir_id(closure_def_id.expect_local()),
353353
);
354354

‎compiler/rustc_typeck/src/expr_use_visitor.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -630,7 +630,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
630630
PlaceBase::Local(*var_hir_id)
631631
};
632632
let place_with_id = PlaceWithHirId::new(
633-
capture_info.expr_id.unwrap_or(closure_expr.hir_id),
633+
capture_info.path_expr_id.unwrap_or(closure_expr.hir_id),
634634
place.base_ty,
635635
place_base,
636636
place.projections.clone(),

‎src/test/ui/closures/2229_closure_analysis/capture-analysis-1.rs

+35
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
#![feature(capture_disjoint_fields)]
2+
//~^ WARNING: the feature `capture_disjoint_fields` is incomplete
3+
//~| NOTE: `#[warn(incomplete_features)]` on by default
4+
//~| NOTE: see issue #53488 <https://github.com/rust-lang/rust/issues/53488>
5+
#![feature(rustc_attrs)]
6+
7+
#[derive(Debug)]
8+
struct Point {
9+
x: i32,
10+
y: i32,
11+
}
12+
13+
fn main() {
14+
let p = Point { x: 10, y: 10 };
15+
let q = Point { x: 10, y: 10 };
16+
17+
let c = #[rustc_capture_analysis]
18+
//~^ ERROR: attributes on expressions are experimental
19+
//~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
20+
|| {
21+
//~^ First Pass analysis includes:
22+
//~| Min Capture analysis includes:
23+
println!("{:?}", p);
24+
//~^ NOTE: Capturing p[] -> ImmBorrow
25+
//~| NOTE: Min Capture p[] -> ImmBorrow
26+
println!("{:?}", p.x);
27+
//~^ NOTE: Capturing p[(0, 0)] -> ImmBorrow
28+
29+
println!("{:?}", q.x);
30+
//~^ NOTE: Capturing q[(0, 0)] -> ImmBorrow
31+
println!("{:?}", q);
32+
//~^ NOTE: Capturing q[] -> ImmBorrow
33+
//~| NOTE: Min Capture q[] -> ImmBorrow
34+
};
35+
}

0 commit comments

Comments
 (0)