bjoernager/rust
bjoernager/rust
1
Fork 0
Code Activity

Rollup merge of #80629 - sexxi-goose:migrations_1, r=nikomatsakis

Browse source 
Add lint for 2229 migrations

Implements the first for RFC 2229 where we make the decision to migrate a root variable based on if the type of the variable needs Drop and if the root variable would be moved into the closure when the feature isn't enabled.

r? `@nikomatsakis`
This commit is contained in:
Jonas Schievink 2021-02-02 12:14:44 +01:00 • committed by GitHub
commit a1887912e8
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 812 additions and 53 deletions
Download patch file Download diff file Expand all files Collapse all files

46
compiler/rustc_lint_defs/src/builtin.rs
View file

@ -2968,6 +2968,7 @@ declare_lint_pass! {
UNSUPPORTED_NAKED_FUNCTIONS,
MISSING_ABI,
SEMICOLON_IN_EXPRESSIONS_FROM_MACROS,
DISJOINT_CAPTURE_DROP_REORDER,
]
}
@ -2994,6 +2995,51 @@ declare_lint! {
"detects doc comments that aren't used by rustdoc"
}
declare_lint! {
/// The `disjoint_capture_drop_reorder` lint detects variables that aren't completely
/// captured when the feature `capture_disjoint_fields` is enabled and it affects the Drop
/// order of at least one path starting at this variable.
///
/// ### Example
///
/// ```rust,compile_fail
/// # #![deny(disjoint_capture_drop_reorder)]
/// # #![allow(unused)]
/// struct FancyInteger(i32);
///
/// impl Drop for FancyInteger {
/// fn drop(&mut self) {
/// println!("Just dropped {}", self.0);
/// }
/// }
///
/// struct Point { x: FancyInteger, y: FancyInteger }
///
/// fn main() {
/// let p = Point { x: FancyInteger(10), y: FancyInteger(20) };
///
/// let c = || {
/// let x = p.x;
/// };
///
/// c();
///
/// // ... More code ...
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// In the above example `p.y` will be dropped at the end of `f` instead of with `c` if
/// the feature `capture_disjoint_fields` is enabled.
pub DISJOINT_CAPTURE_DROP_REORDER,
Allow,
"Drop reorder because of `capture_disjoint_fields`"
}
declare_lint_pass!(UnusedDocComment => [UNUSED_DOC_COMMENTS]);
declare_lint! {

242
compiler/rustc_typeck/src/check/upvar.rs
View file

@ -30,6 +30,7 @@
//! then mean that all later passes would have to check for these figments
//! and report an error, and it just seems like more mess in the end.)
use super::writeback::Resolver;
use super::FnCtxt;
use crate::expr_use_visitor as euv;
@ -40,7 +41,9 @@ use rustc_hir::def_id::LocalDefId;
use rustc_hir::intravisit::{self, NestedVisitorMap, Visitor};
use rustc_infer::infer::UpvarRegion;
use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
use rustc_middle::ty::{self, Ty, TyCtxt, UpvarSubsts};
use rustc_middle::ty::fold::TypeFoldable;
use rustc_middle::ty::{self, Ty, TyCtxt, TypeckResults, UpvarSubsts};
use rustc_session::lint;
use rustc_span::sym;
use rustc_span::{MultiSpan, Span, Symbol};
@ -55,6 +58,11 @@ enum PlaceAncestryRelation {
Divergent,
}
/// Intermediate format to store a captured `Place` and associated `ty::CaptureInfo`
/// during capture analysis. Information in this map feeds into the minimum capture
/// analysis pass.
type InferredCaptureInformation<'tcx> = FxIndexMap<Place<'tcx>, ty::CaptureInfo<'tcx>>;
impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pub fn closure_analyze(&self, body: &'tcx hir::Body<'tcx>) {
InferBorrowKindVisitor { fcx: self }.visit_body(body);
@ -92,7 +100,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self,
closure_hir_id: hir::HirId,
span: Span,
body: &hir::Body<'_>,
body: &'tcx hir::Body<'tcx>,
capture_clause: hir::CaptureBy,
) {
debug!("analyze_closure(id={:?}, body.id={:?})", closure_hir_id, body.id());
@ -124,28 +132,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let local_def_id = closure_def_id.expect_local();
let mut capture_information: FxIndexMap<Place<'tcx>, ty::CaptureInfo<'tcx>> =
Default::default();
if !self.tcx.features().capture_disjoint_fields {
if let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) {
for (&var_hir_id, _) in upvars.iter() {
let place = self.place_for_root_variable(local_def_id, var_hir_id);
debug!("seed place {:?}", place);
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 {
capture_kind_expr_id: None,
path_expr_id: None,
capture_kind,
};
capture_information.insert(place, info);
}
}
}
let body_owner_def_id = self.tcx.hir().body_owner_def_id(body.id());
assert_eq!(body_owner_def_id.to_def_id(), closure_def_id);
let mut delegate = InferBorrowKind {
@ -155,7 +141,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
capture_clause,
current_closure_kind: ty::ClosureKind::LATTICE_BOTTOM,
current_origin: None,
capture_information,
capture_information: Default::default(),
};
euv::ExprUseVisitor::new(
&mut delegate,
@ -172,6 +158,40 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
self.log_capture_analysis_first_pass(closure_def_id, &delegate.capture_information, span);
self.compute_min_captures(closure_def_id, delegate.capture_information);
let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id);
if should_do_migration_analysis(self.tcx, closure_hir_id) {
self.perform_2229_migration_anaysis(closure_def_id, capture_clause, span, body);
}
// We now fake capture information for all variables that are mentioned within the closure
// We do this after handling migrations so that min_captures computes before
if !self.tcx.features().capture_disjoint_fields {
let mut capture_information: InferredCaptureInformation<'tcx> = Default::default();
if let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) {
for var_hir_id in upvars.keys() {
let place = self.place_for_root_variable(local_def_id, *var_hir_id);
debug!("seed place {:?}", place);
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 fake_info = ty::CaptureInfo {
capture_kind_expr_id: None,
path_expr_id: None,
capture_kind,
};
capture_information.insert(place, fake_info);
}
}
// This will update the min captures based on this new fake information.
self.compute_min_captures(closure_def_id, capture_information);
}
if let Some(closure_substs) = infer_kind {
// Unify the (as yet unbound) type variable in the closure
// substs with the kind we inferred.
@ -197,7 +217,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}
self.compute_min_captures(closure_def_id, delegate);
self.log_closure_min_capture_info(closure_def_id, span);
self.min_captures_to_closure_captures_bridge(closure_def_id);
@ -344,6 +363,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Places (and corresponding capture kind) that we need to keep track of to support all
/// the required captured paths.
///
///
/// Note: If this function is called multiple times for the same closure, it will update
/// the existing min_capture map that is stored in TypeckResults.
///
/// Eg:
/// ```rust,no_run
/// struct Point { x: i32, y: i32 }
@ -408,11 +431,18 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
fn compute_min_captures(
&self,
closure_def_id: DefId,
inferred_info: InferBorrowKind<'_, 'tcx>,
capture_information: InferredCaptureInformation<'tcx>,
) {
let mut root_var_min_capture_list: ty::RootVariableMinCaptureList<'_> = Default::default();
if capture_information.is_empty() {
return;
}
for (place, capture_info) in inferred_info.capture_information.into_iter() {
let mut typeck_results = self.typeck_results.borrow_mut();
let mut root_var_min_capture_list =
typeck_results.closure_min_captures.remove(&closure_def_id).unwrap_or_default();
for (place, capture_info) in capture_information.into_iter() {
let var_hir_id = match place.base {
PlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
base => bug!("Expected upvar, found={:?}", base),
@ -422,7 +452,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let min_cap_list = match root_var_min_capture_list.get_mut(&var_hir_id) {
None => {
let mutability = self.determine_capture_mutability(&place);
let mutability = self.determine_capture_mutability(&typeck_results, &place);
let min_cap_list =
vec![ty::CapturedPlace { place, info: capture_info, mutability }];
root_var_min_capture_list.insert(var_hir_id, min_cap_list);
@ -487,7 +517,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
// Only need to insert when we don't have an ancestor in the existing min capture list
if !ancestor_found {
let mutability = self.determine_capture_mutability(&place);
let mutability = self.determine_capture_mutability(&typeck_results, &place);
let captured_place =
ty::CapturedPlace { place, info: updated_capture_info, mutability };
min_cap_list.push(captured_place);
@ -495,15 +525,123 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
debug!("For closure={:?}, min_captures={:#?}", closure_def_id, root_var_min_capture_list);
typeck_results.closure_min_captures.insert(closure_def_id, root_var_min_capture_list);
}
if !root_var_min_capture_list.is_empty() {
self.typeck_results
.borrow_mut()
.closure_min_captures
.insert(closure_def_id, root_var_min_capture_list);
/// Perform the migration analysis for RFC 2229, and emit lint
/// `disjoint_capture_drop_reorder` if needed.
fn perform_2229_migration_anaysis(
&self,
closure_def_id: DefId,
capture_clause: hir::CaptureBy,
span: Span,
body: &'tcx hir::Body<'tcx>,
) {
let need_migrations = self.compute_2229_migrations_first_pass(
closure_def_id,
span,
capture_clause,
body,
self.typeck_results.borrow().closure_min_captures.get(&closure_def_id),
);
if !need_migrations.is_empty() {
let need_migrations_hir_id = need_migrations.iter().map(|m| m.0).collect::<Vec<_>>();
let migrations_text = migration_suggestion_for_2229(self.tcx, &need_migrations_hir_id);
let local_def_id = closure_def_id.expect_local();
let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id);
self.tcx.struct_span_lint_hir(
lint::builtin::DISJOINT_CAPTURE_DROP_REORDER,
closure_hir_id,
span,
|lint| {
let mut diagnostics_builder = lint.build(
"drop order affected for closure because of `capture_disjoint_fields`",
);
diagnostics_builder.note(&migrations_text);
diagnostics_builder.emit();
},
);
}
}
/// Figures out the list of root variables (and their types) that aren't completely
/// captured by the closure when `capture_disjoint_fields` is enabled and drop order of
/// some path starting at that root variable **might** be affected.
///
/// The output list would include a root variable if:
/// - It would have been moved into the closure when `capture_disjoint_fields` wasn't
/// enabled, **and**
/// - It wasn't completely captured by the closure, **and**
/// - The type of the root variable needs Drop.
fn compute_2229_migrations_first_pass(
&self,
closure_def_id: DefId,
closure_span: Span,
closure_clause: hir::CaptureBy,
body: &'tcx hir::Body<'tcx>,
min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
) -> Vec<(hir::HirId, Ty<'tcx>)> {
fn resolve_ty<T: TypeFoldable<'tcx>>(
fcx: &FnCtxt<'_, 'tcx>,
span: Span,
body: &'tcx hir::Body<'tcx>,
ty: T,
) -> T {
let mut resolver = Resolver::new(fcx, &span, body);
ty.fold_with(&mut resolver)
}
let upvars = if let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) {
upvars
} else {
return vec![];
};
let mut need_migrations = Vec::new();
for (&var_hir_id, _) in upvars.iter() {
let ty = resolve_ty(self, closure_span, body, self.node_ty(var_hir_id));
if !ty.needs_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local())) {
continue;
}
let root_var_min_capture_list = if let Some(root_var_min_capture_list) =
min_captures.and_then(|m| m.get(&var_hir_id))
{
root_var_min_capture_list
} else {
// The upvar is mentioned within the closure but no path starting from it is
// used.
match closure_clause {
// Only migrate if closure is a move closure
hir::CaptureBy::Value => need_migrations.push((var_hir_id, ty)),
hir::CaptureBy::Ref => {}
}
continue;
};
let is_moved = root_var_min_capture_list
.iter()
.any(|capture| matches!(capture.info.capture_kind, ty::UpvarCapture::ByValue(_)));
let is_not_completely_captured =
root_var_min_capture_list.iter().any(|capture| capture.place.projections.len() > 0);
if is_moved && is_not_completely_captured {
need_migrations.push((var_hir_id, ty));
}
}
need_migrations
}
fn init_capture_kind(
&self,
capture_clause: hir::CaptureBy,
@ -613,18 +751,17 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// A captured place is mutable if
/// 1. Projections don't include a Deref of an immut-borrow, **and**
/// 2. PlaceBase is mut or projections include a Deref of a mut-borrow.
fn determine_capture_mutability(&self, place: &Place<'tcx>) -> hir::Mutability {
fn determine_capture_mutability(
&self,
typeck_results: &'a TypeckResults<'tcx>,
place: &Place<'tcx>,
) -> hir::Mutability {
let var_hir_id = match place.base {
PlaceBase::Upvar(upvar_id) => upvar_id.var_path.hir_id,
_ => unreachable!(),
};
let bm = *self
.typeck_results
.borrow()
.pat_binding_modes()
.get(var_hir_id)
.expect("missing binding mode");
let bm = *typeck_results.pat_binding_modes().get(var_hir_id).expect("missing binding mode");
let mut is_mutbl = match bm {
ty::BindByValue(mutability) => mutability,
@ -698,9 +835,11 @@ struct InferBorrowKind<'a, 'tcx> {
///
/// For closure `fix_s`, (at a high level) the map contains
///
/// ```
/// Place { V1, [ProjectionKind::Field(Index=0, Variant=0)] } : CaptureKind { E1, ImmutableBorrow }
/// Place { V1, [ProjectionKind::Field(Index=1, Variant=0)] } : CaptureKind { E2, MutableBorrow }
capture_information: FxIndexMap<Place<'tcx>, ty::CaptureInfo<'tcx>>,
/// ```
capture_information: InferredCaptureInformation<'tcx>,
}
impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
@ -1119,6 +1258,21 @@ fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
tcx.hir().name(var_hir_id)
}
fn should_do_migration_analysis(tcx: TyCtxt<'_>, closure_id: hir::HirId) -> bool {
let (level, _) =
tcx.lint_level_at_node(lint::builtin::DISJOINT_CAPTURE_DROP_REORDER, closure_id);
!matches!(level, lint::Level::Allow)
}
fn migration_suggestion_for_2229(tcx: TyCtxt<'_>, need_migrations: &Vec<hir::HirId>) -> String {
let need_migrations_strings =
need_migrations.iter().map(|v| format!("{}", var_name(tcx, *v))).collect::<Vec<_>>();
let migrations_list_concat = need_migrations_strings.join(", ");
format!("drop(&({}));", migrations_list_concat)
}
/// Helper function to determine if we need to escalate CaptureKind from
/// CaptureInfo A to B and returns the escalated CaptureInfo.
/// (Note: CaptureInfo contains CaptureKind and an expression that led to capture it in that way)

6
compiler/rustc_typeck/src/check/writeback.rs
View file

@ -650,7 +650,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
}
}
trait Locatable {
crate trait Locatable {
fn to_span(&self, tcx: TyCtxt<'_>) -> Span;
}
@ -668,7 +668,7 @@ impl Locatable for hir::HirId {
/// The Resolver. This is the type folding engine that detects
/// unresolved types and so forth.
struct Resolver<'cx, 'tcx> {
crate struct Resolver<'cx, 'tcx> {
tcx: TyCtxt<'tcx>,
infcx: &'cx InferCtxt<'cx, 'tcx>,
span: &'cx dyn Locatable,
@ -679,7 +679,7 @@ struct Resolver<'cx, 'tcx> {
}
impl<'cx, 'tcx> Resolver<'cx, 'tcx> {
fn new(
crate fn new(
fcx: &'cx FnCtxt<'cx, 'tcx>,
span: &'cx dyn Locatable,
body: &'tcx hir::Body<'tcx>,