Skip to content

Commit

Permalink
Rollup merge of rust-lang#88708 - Aaron1011:aggregate-usage, r=oli-obk
Browse files Browse the repository at this point in the history
Add `ConstraintCategory::Usage` for handling aggregate construction

In some cases, we emit borrowcheck diagnostics pointing
at a particular field expression in a struct expression
(e.g. `MyStruct { field: my_expr }`). However, this
behavior currently relies on us choosing the
`ConstraintCategory::Boring` with the 'correct' span.
When adding additional variants to `ConstraintCategory`,
(or changing existing usages away from `ConstraintCategory::Boring`),
the current behavior can easily get broken, since a non-boring
constraint will get chosen over a boring one.

To make the diagnostic output less fragile, this commit
adds a `ConstraintCategory::Usage` variant. We use this variant
for the temporary assignments created for each field of
an aggregate we are constructing.

Using this new variant, we can emit a message mentioning
"this usage", emphasizing the fact that the error message
is related to the specific use site (in the struct expression).

This is preparation for additional work on improving NLL error messages
(see rust-lang#57374)
  • Loading branch information
GuillaumeGomez authored Sep 16, 2021
2 parents d7a656f + 3feddf7 commit 0dd5e22
Show file tree
Hide file tree
Showing 14 changed files with 71 additions and 36 deletions.
1 change: 1 addition & 0 deletions compiler/rustc_borrowck/src/diagnostics/region_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ impl ConstraintDescription for ConstraintCategory {
ConstraintCategory::CopyBound => "copying this value ",
ConstraintCategory::OpaqueType => "opaque type ",
ConstraintCategory::ClosureUpvar(_) => "closure capture ",
ConstraintCategory::Usage => "this usage ",
ConstraintCategory::Boring
| ConstraintCategory::BoringNoLocation
| ConstraintCategory::Internal => "",
Expand Down
13 changes: 13 additions & 0 deletions compiler/rustc_borrowck/src/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1388,11 +1388,24 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
ConstraintCategory::Return(ReturnConstraint::Normal)
}
}
Some(l)
if matches!(
body.local_decls[l].local_info,
Some(box LocalInfo::AggregateTemp)
) =>
{
ConstraintCategory::Usage
}
Some(l) if !body.local_decls[l].is_user_variable() => {
ConstraintCategory::Boring
}
_ => ConstraintCategory::Assignment,
};
debug!(
"assignment category: {:?} {:?}",
category,
place.as_local().map(|l| &body.local_decls[l])
);

let place_ty = place.ty(body, tcx).ty;
let place_ty = self.normalize(place_ty, location);
Expand Down
3 changes: 3 additions & 0 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -992,6 +992,9 @@ pub enum LocalInfo<'tcx> {
StaticRef { def_id: DefId, is_thread_local: bool },
/// A temporary created that references the const with the given `DefId`
ConstRef { def_id: DefId },
/// A temporary created during the creation of an aggregate
/// (e.g. a temporary for `foo` in `MyStruct { my_field: foo }`)
AggregateTemp,
}

impl<'tcx> LocalDecl<'tcx> {
Expand Down
8 changes: 3 additions & 5 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,17 +332,15 @@ pub enum ConstraintCategory {
CopyBound,
SizedBound,
Assignment,
/// A constraint that came from a usage of a variable (e.g. in an ADT expression
/// like `Foo { field: my_val }`)
Usage,
OpaqueType,
ClosureUpvar(hir::HirId),

/// A "boring" constraint (caused by the given location) is one that
/// the user probably doesn't want to see described in diagnostics,
/// because it is kind of an artifact of the type system setup.
/// Example: `x = Foo { field: y }` technically creates
/// intermediate regions representing the "type of `Foo { field: y
/// }`", and data flows from `y` into those variables, but they
/// are not very interesting. The assignment into `x` on the other
/// hand might be.
Boring,
// Boring and applicable everywhere.
BoringNoLocation,
Expand Down
17 changes: 13 additions & 4 deletions compiler/rustc_mir_build/src/build/expr/as_operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
expr: &Expr<'tcx>,
) -> BlockAnd<Operand<'tcx>> {
let local_scope = self.local_scope();
self.as_operand(block, Some(local_scope), expr)
self.as_operand(block, Some(local_scope), expr, None)
}

/// Returns an operand suitable for use until the end of the current scope expression and
Expand Down Expand Up @@ -85,6 +85,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
/// temporary `tmp = x`, so that we capture the value of `x` at
/// this time.
///
/// If we end up needing to create a temporary, then we will use
/// `local_info` as its `LocalInfo`, unless `as_temporary`
/// has already assigned it a non-`None` `LocalInfo`.
/// Normally, you should use `None` for `local_info`
///
/// The operand is known to be live until the end of `scope`.
///
/// Like `as_local_call_operand`, except that the argument will
Expand All @@ -94,15 +99,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
mut block: BasicBlock,
scope: Option<region::Scope>,
expr: &Expr<'tcx>,
local_info: Option<Box<LocalInfo<'tcx>>>,
) -> BlockAnd<Operand<'tcx>> {
debug!("as_operand(block={:?}, expr={:?})", block, expr);
debug!("as_operand(block={:?}, expr={:?} local_info={:?})", block, expr, local_info);
let this = self;

if let ExprKind::Scope { region_scope, lint_level, value } = expr.kind {
let source_info = this.source_info(expr.span);
let region_scope = (region_scope, source_info);
return this.in_scope(region_scope, lint_level, |this| {
this.as_operand(block, scope, &this.thir[value])
this.as_operand(block, scope, &this.thir[value], local_info)
});
}

Expand All @@ -115,6 +121,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
Category::Place | Category::Rvalue(..) => {
let operand = unpack!(block = this.as_temp(block, scope, expr, Mutability::Mut));
if this.local_decls[operand].local_info.is_none() {
this.local_decls[operand].local_info = local_info;
}
block.and(Operand::Move(Place::from(operand)))
}
}
Expand Down Expand Up @@ -167,6 +176,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
}

this.as_operand(block, scope, expr)
this.as_operand(block, scope, expr, None)
}
}
24 changes: 14 additions & 10 deletions compiler/rustc_mir_build/src/build/expr/as_rvalue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
}
ExprKind::Repeat { value, count } => {
let value_operand =
unpack!(block = this.as_operand(block, scope, &this.thir[value]));
unpack!(block = this.as_operand(block, scope, &this.thir[value], None));
block.and(Rvalue::Repeat(value_operand, count))
}
ExprKind::Binary { op, lhs, rhs } => {
let lhs = unpack!(block = this.as_operand(block, scope, &this.thir[lhs]));
let rhs = unpack!(block = this.as_operand(block, scope, &this.thir[rhs]));
let lhs = unpack!(block = this.as_operand(block, scope, &this.thir[lhs], None));
let rhs = unpack!(block = this.as_operand(block, scope, &this.thir[rhs], None));
this.build_binary_op(block, op, expr_span, expr.ty, lhs, rhs)
}
ExprKind::Unary { op, arg } => {
let arg = unpack!(block = this.as_operand(block, scope, &this.thir[arg]));
let arg = unpack!(block = this.as_operand(block, scope, &this.thir[arg], None));
// Check for -MIN on signed integers
if this.check_overflow && op == UnOp::Neg && expr.ty.is_signed() {
let bool_ty = this.tcx.types.bool;
Expand Down Expand Up @@ -116,11 +116,13 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
block.and(Rvalue::Use(Operand::Move(Place::from(result))))
}
ExprKind::Cast { source } => {
let source = unpack!(block = this.as_operand(block, scope, &this.thir[source]));
let source =
unpack!(block = this.as_operand(block, scope, &this.thir[source], None));
block.and(Rvalue::Cast(CastKind::Misc, source, expr.ty))
}
ExprKind::Pointer { cast, source } => {
let source = unpack!(block = this.as_operand(block, scope, &this.thir[source]));
let source =
unpack!(block = this.as_operand(block, scope, &this.thir[source], None));
block.and(Rvalue::Cast(CastKind::Pointer(cast), source, expr.ty))
}
ExprKind::Array { ref fields } => {
Expand Down Expand Up @@ -155,7 +157,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fields: Vec<_> = fields
.into_iter()
.copied()
.map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f])))
.map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f], None)))
.collect();

block.and(Rvalue::Aggregate(Box::new(AggregateKind::Array(el_ty)), fields))
Expand All @@ -166,7 +168,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fields: Vec<_> = fields
.into_iter()
.copied()
.map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f])))
.map(|f| unpack!(block = this.as_operand(block, scope, &this.thir[f], None)))
.collect();

block.and(Rvalue::Aggregate(Box::new(AggregateKind::Tuple), fields))
Expand Down Expand Up @@ -242,7 +244,9 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
&this.thir[arg],
)
),
_ => unpack!(block = this.as_operand(block, scope, upvar)),
_ => {
unpack!(block = this.as_operand(block, scope, upvar, None))
}
}
}
}
Expand Down Expand Up @@ -304,7 +308,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
Category::of(&expr.kind),
Some(Category::Rvalue(RvalueFunc::AsRvalue))
));
let operand = unpack!(block = this.as_operand(block, scope, expr));
let operand = unpack!(block = this.as_operand(block, scope, expr, None));
block.and(Rvalue::Use(operand))
}
}
Expand Down
11 changes: 9 additions & 2 deletions compiler/rustc_mir_build/src/build/expr/into.rs
Original file line number Diff line number Diff line change
Expand Up @@ -326,10 +326,16 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
let fields_map: FxHashMap<_, _> = fields
.into_iter()
.map(|f| {
let local_info = Box::new(LocalInfo::AggregateTemp);
(
f.name,
unpack!(
block = this.as_operand(block, Some(scope), &this.thir[f.expr])
block = this.as_operand(
block,
Some(scope),
&this.thir[f.expr],
Some(local_info)
)
),
)
})
Expand Down Expand Up @@ -508,7 +514,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {

ExprKind::Yield { value } => {
let scope = this.local_scope();
let value = unpack!(block = this.as_operand(block, Some(scope), &this.thir[value]));
let value =
unpack!(block = this.as_operand(block, Some(scope), &this.thir[value], None));
let resume = this.cfg.start_new_block();
this.cfg.terminate(
block,
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/issues/issue-61882-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | Self(&x);
| ^^
| |
| borrowed value does not live long enough
| requires that `x` is borrowed for `'static`
| this usage requires that `x` is borrowed for `'static`
LL |
LL | }
| - `x` dropped here while still borrowed
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/nll/issue-46036.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | let foo = Foo { x: &a };
| ^^
| |
| borrowed value does not live long enough
| requires that `a` is borrowed for `'static`
| this usage requires that `a` is borrowed for `'static`
LL | loop { }
LL | }
| - `a` dropped here while still borrowed
Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/nll/user-annotations/adt-brace-enums.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | SomeEnum::SomeVariant::<&'static u32> { t: &c };
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'static`
| this usage requires that `c` is borrowed for `'static`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -19,7 +19,7 @@ LL | SomeEnum::SomeVariant::<&'a u32> { t: &c };
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -33,7 +33,7 @@ LL | SomeEnum::SomeVariant::<&'a u32> { t: &c };
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | };
| - `c` dropped here while still borrowed

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/nll/user-annotations/adt-brace-structs.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | SomeStruct::<&'static u32> { t: &c };
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'static`
| this usage requires that `c` is borrowed for `'static`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -19,7 +19,7 @@ LL | SomeStruct::<&'a u32> { t: &c };
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -33,7 +33,7 @@ LL | SomeStruct::<&'a u32> { t: &c };
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | };
| - `c` dropped here while still borrowed

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/nll/user-annotations/adt-tuple-enums.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | SomeEnum::SomeVariant::<&'static u32>(&c);
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'static`
| this usage requires that `c` is borrowed for `'static`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -19,7 +19,7 @@ LL | SomeEnum::SomeVariant::<&'a u32>(&c);
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -33,7 +33,7 @@ LL | SomeEnum::SomeVariant::<&'a u32>(&c);
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | };
| - `c` dropped here while still borrowed

Expand Down
6 changes: 3 additions & 3 deletions src/test/ui/nll/user-annotations/adt-tuple-struct.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ LL | SomeStruct::<&'static u32>(&c);
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'static`
| this usage requires that `c` is borrowed for `'static`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -19,7 +19,7 @@ LL | SomeStruct::<&'a u32>(&c);
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | }
| - `c` dropped here while still borrowed

Expand All @@ -33,7 +33,7 @@ LL | SomeStruct::<&'a u32>(&c);
| ^^
| |
| borrowed value does not live long enough
| requires that `c` is borrowed for `'a`
| this usage requires that `c` is borrowed for `'a`
LL | };
| - `c` dropped here while still borrowed

Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/nll/where_clauses_in_structs.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ LL | fn bar<'a, 'b>(x: Cell<&'a u32>, y: Cell<&'b u32>) {
| |
| lifetime `'a` defined here
LL | Foo { x, y };
| ^ requires that `'a` must outlive `'b`
| ^ this usage requires that `'a` must outlive `'b`
|
= help: consider adding the following bound: `'a: 'b`

Expand Down

0 comments on commit 0dd5e22

Please sign in to comment.