Skip to content

Commit

Permalink
Try to point out when edition 2024 lifetime capture rules cause borro…
Browse files Browse the repository at this point in the history
…wck issues
  • Loading branch information
compiler-errors committed Oct 25, 2024
1 parent 0ccc39d commit b2fa80e
Show file tree
Hide file tree
Showing 9 changed files with 754 additions and 2 deletions.
8 changes: 7 additions & 1 deletion compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1489,6 +1489,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
&borrow_msg,
&value_msg,
);
self.note_due_to_edition_2024_opaque_capture_rules(borrow, &mut err);

borrow_spans.var_path_only_subdiag(&mut err, crate::InitializationRequiringAction::Borrow);

Expand Down Expand Up @@ -1561,6 +1562,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
borrow_span,
&self.describe_any_place(borrow.borrowed_place.as_ref()),
);
self.note_due_to_edition_2024_opaque_capture_rules(borrow, &mut err);

borrow_spans.var_subdiag(&mut err, Some(borrow.kind), |kind, var_span| {
use crate::session_diagnostics::CaptureVarCause::*;
let place = &borrow.borrowed_place;
Expand Down Expand Up @@ -1820,6 +1823,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
unreachable!()
}
};
self.note_due_to_edition_2024_opaque_capture_rules(issued_borrow, &mut err);

if issued_spans == borrow_spans {
borrow_spans.var_subdiag(&mut err, Some(gen_borrow_kind), |kind, var_span| {
Expand Down Expand Up @@ -2860,7 +2864,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {

debug!(?place_desc, ?explanation);

let err = match (place_desc, explanation) {
let mut err = match (place_desc, explanation) {
// If the outlives constraint comes from inside the closure,
// for example:
//
Expand Down Expand Up @@ -2939,6 +2943,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
explanation,
),
};
self.note_due_to_edition_2024_opaque_capture_rules(borrow, &mut err);

self.buffer_error(err);
}
Expand Down Expand Up @@ -3777,6 +3782,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
}

let mut err = self.cannot_assign_to_borrowed(span, loan_span, &descr_place);
self.note_due_to_edition_2024_opaque_capture_rules(loan, &mut err);

loan_spans.var_subdiag(&mut err, Some(loan.kind), |kind, var_span| {
use crate::session_diagnostics::CaptureVarCause::*;
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mod conflict_errors;
mod explain_borrow;
mod move_errors;
mod mutability_errors;
mod opaque_suggestions;
mod region_errors;

pub(crate) use bound_region_errors::{ToUniverseInfo, UniverseInfo};
Expand Down
224 changes: 224 additions & 0 deletions compiler/rustc_borrowck/src/diagnostics/opaque_suggestions.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,224 @@
#![allow(rustc::diagnostic_outside_of_impl)]
#![allow(rustc::untranslatable_diagnostic)]

use std::ops::ControlFlow;

use either::Either;
use rustc_data_structures::fx::FxIndexSet;
use rustc_errors::{Applicability, Diag};
use rustc_hir as hir;
use rustc_hir::def_id::DefId;
use rustc_middle::mir::{self, ConstraintCategory, Location};
use rustc_middle::ty::{
self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitableExt, TypeVisitor,
};
use rustc_span::Symbol;

use crate::MirBorrowckCtxt;
use crate::borrow_set::BorrowData;
use crate::consumers::RegionInferenceContext;
use crate::type_check::Locations;

impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
/// Try to note when an opaque is involved in a borrowck error and that
/// opaque captures lifetimes due to edition 2024.
// FIXME: This code is otherwise somewhat general, and could easily be adapted
// to explain why other things overcapture... like async fn and RPITITs.
pub(crate) fn note_due_to_edition_2024_opaque_capture_rules(
&self,
borrow: &BorrowData<'tcx>,
diag: &mut Diag<'_>,
) {
// We look at all the locals. Why locals? Because it's the best thing
// I could think of that's correlated with the *instantiated* higer-ranked
// binder for calls, since we don't really store those anywhere else.
for ty in self.body.local_decls.iter().map(|local| local.ty) {
if !ty.has_opaque_types() {
continue;
}

let tcx = self.infcx.tcx;
let ControlFlow::Break((opaque_def_id, offending_region_idx, location)) = ty
.visit_with(&mut FindOpaqueRegion {
regioncx: &self.regioncx,
tcx,
borrow_region: borrow.region,
})
else {
continue;
};

// If an opaque explicitly captures a lifetime, then no need to point it out.
// FIXME: We should be using a better heuristic for `use<>`.
if tcx.rendered_precise_capturing_args(opaque_def_id).is_some() {
continue;
}

// If one of the opaque's bounds mentions the region, then no need to
// point it out, since it would've been captured on edition 2021 as well.
//
// Also, while we're at it, collect all the lifetimes that the opaque
// *does* mention. We'll use that for the `+ use<'a>` suggestion below.
let mut visitor = CheckExplicitRegionMentionAndCollectGenerics {
tcx,
offending_region_idx,
seen_opaques: [opaque_def_id].into_iter().collect(),
seen_lifetimes: Default::default(),
};
if tcx
.explicit_item_bounds(opaque_def_id)
.skip_binder()
.visit_with(&mut visitor)
.is_break()
{
continue;
}

// If we successfully located a terminator, then point it out
// and provide a suggestion if it's local.
match self.body.stmt_at(location) {
Either::Right(mir::Terminator { source_info, .. }) => {
diag.span_note(
source_info.span,
"this call may capture more lifetimes than intended, \
because Rust 2024 has adjusted the `impl Trait` lifetime capture rules",
);
let mut seen_generics: Vec<_> =
visitor.seen_lifetimes.iter().map(ToString::to_string).collect();
// Capture all in-scope ty/const params.
seen_generics.extend(
ty::GenericArgs::identity_for_item(tcx, opaque_def_id)
.iter()
.filter(|arg| {
matches!(
arg.unpack(),
ty::GenericArgKind::Type(_) | ty::GenericArgKind::Const(_)
)
})
.map(|arg| arg.to_string()),
);
if opaque_def_id.is_local() {
diag.span_suggestion_verbose(
tcx.def_span(opaque_def_id).shrink_to_hi(),
"add a precise capturing bound to avoid overcapturing",
format!(" + use<{}>", seen_generics.join(", ")),
Applicability::MaybeIncorrect,
);
} else {
diag.span_help(
tcx.def_span(opaque_def_id),
format!(
"if you can modify this crate, add a precise \
capturing bound to avoid overcapturing: `+ use<{}>`",
seen_generics.join(", ")
),
);
}
return;
}
Either::Left(_) => {}
}
}
}
}

/// This visitor contains the bulk of the logic for this lint.
struct FindOpaqueRegion<'a, 'tcx> {
tcx: TyCtxt<'tcx>,
regioncx: &'a RegionInferenceContext<'tcx>,
borrow_region: ty::RegionVid,
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for FindOpaqueRegion<'_, 'tcx> {
type Result = ControlFlow<(DefId, usize, Location), ()>;

fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result {
// If we find an opaque in a local ty, then for each of its captured regions,
// try to find a path between that captured regions and our borrow region...
if let ty::Alias(ty::Opaque, opaque) = *ty.kind()
&& let hir::OpaqueTyOrigin::FnReturn { parent, in_trait_or_impl: None } =
self.tcx.opaque_ty_origin(opaque.def_id)
{
let variances = self.tcx.variances_of(opaque.def_id);
for (idx, (arg, variance)) in std::iter::zip(opaque.args, variances).enumerate() {
// Skip uncaptured args.
if *variance == ty::Bivariant {
continue;
}
// We only care about regions.
let Some(opaque_region) = arg.as_region() else {
continue;
};
// Don't try to convert a late-bound region, which shouldn't exist anyways (yet).
if opaque_region.is_bound() {
continue;
}
let opaque_region_vid = self.regioncx.to_region_vid(opaque_region);

// Find a path between the borrow region and our opaque capture.
if let Some((path, _)) =
self.regioncx.find_constraint_paths_between_regions(self.borrow_region, |r| {
r == opaque_region_vid
})
{
for constraint in path {
// If we find a call in this path, then check if it defines the opaque.
if let ConstraintCategory::CallArgument(Some(call_ty)) = constraint.category
&& let ty::FnDef(call_def_id, _) = *call_ty.kind()
// This function defines the opaque :D
&& call_def_id == parent
&& let Locations::Single(location) = constraint.locations
{
return ControlFlow::Break((opaque.def_id, idx, location));
}
}
}
}
}

ty.super_visit_with(self)
}
}

struct CheckExplicitRegionMentionAndCollectGenerics<'tcx> {
tcx: TyCtxt<'tcx>,
offending_region_idx: usize,
seen_opaques: FxIndexSet<DefId>,
seen_lifetimes: FxIndexSet<Symbol>,
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for CheckExplicitRegionMentionAndCollectGenerics<'tcx> {
type Result = ControlFlow<(), ()>;

fn visit_ty(&mut self, ty: Ty<'tcx>) -> Self::Result {
match *ty.kind() {
ty::Alias(ty::Opaque, opaque) => {
if self.seen_opaques.insert(opaque.def_id) {
for (bound, _) in self
.tcx
.explicit_item_bounds(opaque.def_id)
.iter_instantiated_copied(self.tcx, opaque.args)
{
bound.visit_with(self)?;
}
}
ControlFlow::Continue(())
}
_ => ty.super_visit_with(self),
}
}

fn visit_region(&mut self, r: ty::Region<'tcx>) -> Self::Result {
match r.kind() {
ty::ReEarlyParam(param) => {
if param.index as usize == self.offending_region_idx {
ControlFlow::Break(())
} else {
self.seen_lifetimes.insert(param.name);
ControlFlow::Continue(())
}
}
_ => ControlFlow::Continue(()),
}
}
}
2 changes: 1 addition & 1 deletion compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ use rustc_type_ir::lang_items::TraitSolverLangItem;
pub use rustc_type_ir::lift::Lift;
use rustc_type_ir::solve::SolverMode;
use rustc_type_ir::{CollectAndApply, Interner, TypeFlags, WithCachedTypeInfo, search_graph};
use tracing::{debug, trace};
use tracing::{debug, instrument};

use crate::arena::Arena;
use crate::dep_graph::{DepGraph, DepKindStruct};
Expand Down
6 changes: 6 additions & 0 deletions tests/ui/impl-trait/precise-capturing/auxiliary/foreign.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
//@ edition: 2024
//@ compile-flags: -Zunstable-options

use std::fmt::Display;

pub fn hello(x: &Vec<i32>) -> impl Display { 0 }
15 changes: 15 additions & 0 deletions tests/ui/impl-trait/precise-capturing/foreign-2021.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//@ aux-build: foreign.rs

extern crate foreign;

fn main() {
let mut x = vec![];
let h = foreign::hello(&x);
//~^ NOTE this call may capture more lifetimes than intended
//~| NOTE immutable borrow occurs here
x.push(0);
//~^ ERROR cannot borrow `x` as mutable
//~| NOTE mutable borrow occurs here
println!("{h}");
//~^ NOTE immutable borrow later used here
}
26 changes: 26 additions & 0 deletions tests/ui/impl-trait/precise-capturing/foreign-2021.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
error[E0502]: cannot borrow `x` as mutable because it is also borrowed as immutable
--> $DIR/foreign-2021.rs:10:5
|
LL | let h = foreign::hello(&x);
| -- immutable borrow occurs here
...
LL | x.push(0);
| ^^^^^^^^^ mutable borrow occurs here
...
LL | println!("{h}");
| --- immutable borrow later used here
|
note: this call may capture more lifetimes than intended, because Rust 2024 has adjusted the `impl Trait` lifetime capture rules
--> $DIR/foreign-2021.rs:7:13
|
LL | let h = foreign::hello(&x);
| ^^^^^^^^^^^^^^^^^^
help: if you can modify this crate, add a precise capturing bound to avoid overcapturing: `+ use<>`
--> $DIR/auxiliary/foreign.rs:6:31
|
LL | pub fn hello(x: &Vec<i32>) -> impl Display { 0 }
| ^^^^^^^^^^^^

error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0502`.
Loading

0 comments on commit b2fa80e

Please sign in to comment.