dropck: must assume Box<Trait + 'a> has a destructor of interest.
#25212
Conversation
There are two interesting kinds of breakage illustrated here:
1. `Box<Trait>` in many contexts is treated as `Box<Trait + 'static>`,
due to [RFC 599]. However, in a type like `&'a Box<Trait>`, the
`Box<Trait>` type will be expanded to `Box<Trait + 'a>`, again due
to [RFC 599]. This, combined with the fix to Issue 25199, leads to
a borrowck problem due the combination of this function signature
(in src/libstd/net/parser.rs):
```rust
fn read_or<T>(&mut self, parsers: &mut [Box<FnMut(&mut Parser) -> Option<T>>]) -> Option<T>;
```
with this call site (again in src/libstd/net/parser.rs):
```rust
fn read_ip_addr(&mut self) -> Option<IpAddr> {
let ipv4_addr = |p: &mut Parser| p.read_ipv4_addr().map(|v4| IpAddr::V4(v4));
let ipv6_addr = |p: &mut Parser| p.read_ipv6_addr().map(|v6| IpAddr::V6(v6));
self.read_or(&mut [Box::new(ipv4_addr), Box::new(ipv6_addr)])
}
```
yielding borrowck errors like:
```
parser.rs:265:27: 265:69 error: borrowed value does not live long enough
parser.rs:265 self.read_or(&mut [Box::new(ipv4_addr), Box::new(ipv6_addr)])
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
(full log at: https://gist.github.com/pnkfelix/e2e80f1a71580f5d3103 )
The issue here is perhaps subtle: the `parsers` argument is
inferred to be taking a slice of boxed objects with the implicit
lifetime bound attached to the `self` parameter to `read_or`.
Meanwhile, the fix to Issue 25199 (added in a forth-coming commit)
is forcing us to assume that each boxed object may have a
destructor that could refer to state of that lifetime, and
*therefore* that inferred lifetime is required to outlive the boxed
object itself.
In this case, the relevant boxed object here is not going to make
any such references; I believe it is just an artifact of how the
expression was built that it is not assigned type:
`Box<FnMut(&mut Parser) -> Option<T> + 'static>`.
(i.e., mucking with the expression is probably one way to fix this
problem).
But the other way to fix it, adopted here, is to change the
`read_or` method type to force make the (presumably-intended)
`'static` bound explicit on the boxed `FnMut` object.
(Note: this is still just the *first* example of breakage.)
2. In `macro_rules.rs`, the `TTMacroExpander` trait defines a method
with signature:
```rust
fn expand<'cx>(&self, cx: &'cx mut ExtCtxt, ...) -> Box<MacResult+'cx>;
```
taking a `&'cx mut ExtCtxt` as an argument and returning a
`Box<MacResult'cx>`.
The fix to Issue 25199 (added in aforementioned forth-coming
commit) assumes that a value of type `Box<MacResult+'cx>` may, in
its destructor, refer to a reference of lifetime `'cx`; thus the
`'cx` lifetime is forced to outlive the returned value.
Meanwhile, within `expand.rs`, the old code was doing:
```rust
match expander.expand(fld.cx, ...).make_pat() { ... => immutable borrow of fld.cx ... }
```
The problem is that the `'cx` lifetime, inferred for the
`expander.expand` call, has now been extended so that it has to
outlive the temporary R-value returned by `expanded.expand`. But
call is also reborrowing `fld.cx` *mutably*, which means that this
reborrow must end before any immutable borrow of `fld.cx`; but
there is one of those within the match body. (Note that the
temporary R-values for the input expression to `match` all live as
long as the whole `match` expression itself (see Issue rust-lang#3511 and PR
rust-lang#11585).
To address this, I moved the construction of the pat value into its
own `let`-statement, so that the `Box<MacResult>` will only live
for as long as the initializing expression for the `let`-statement,
and thus allow the subsequent immutable borrow within the `match`.
[RFC 599]: https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md
Implements this (previously overlooked) note from [RFC 769]: > (Note: When encountering a D of the form `Box<Trait+'b>`, we > conservatively assume that such a type has a Drop implementation > parametric in 'b.) Fix rust-lang#25199. [breaking-change] The breakage here falls into both obvious and non-obvious cases. The obvious case: if you were relying on the unsoundness this exposes (namely being able to reference dead storage from a destructor, by doing it via a boxed trait object bounded by the lifetime of the dead storage), then this change disallows that. The non-obvious cases: The way dropck works, it causes lifetimes to be extended to longer extents than they covered before. I.e. lifetimes that are attached as trait-bounds may become longer than they were previously. * This includes lifetimes that are only *implicitly* attached as trait-bounds (due to [RFC 599]). So you may have code that was e.g. taking a parameter of type `&'a Box<Trait>` (which expands to `&'a Box<Trait+'a>`), that now may need to be assigned type `&'a Box<Trait+'static>` to ensure that `'a` is not inadvertantly inferred to a region that is actually too long. (See earlier commit in this PR for an example of this.) [RFC 769]: https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#the-drop-check-rule [RFC 599]: https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
running |
pnkfelix
added
the
beta-nominated
|
(there is fallout in |
|
(there is also fallout in |
This change is worrisome to me, both because: 1. I thought the rules in RFC 599 imply that the `Box<Trait>` without `'static` in the first case would expand to the second case, but their behaviors here differ. And, 2. The explicit handling of `'static` should mean `dropck` has no application here and thus we should have seen no change to the expected error messages. Nonetheless, the error messages changed.
|
So, adding a trait SomeTrait {
fn dummy(&self) { }
}
impl SomeTrait for i32 { }
fn arr<'a>(x: &mut [Box<SomeTrait+'a>]) {
}
fn main() {
arr(&mut [Box::new(1)])
}In particular, one of the downsides of rust-lang/rfcs#599 was specifically around this case of This makes me wonder if rust-lang/rfcs#599 should be adjusted to say that things like |
|
@nikomatsakis i assume when you say "the RFC" you are referring to RFC 599 -- you may want to update your comment to be explicit with the numbers. |
|
@pnkfelix indeed. |
|
@pnkfelix interestingly, one cannot just promote the |
|
@bors r+ |
|
📌 Commit 0fa1c16 has been approved by |
|
@bors: p=100 |
|
⌛ Testing commit 0fa1c16 with merge c1c7091... |
|
@bors: retry force clean |
dropck: must assume `Box<Trait + 'a>` has a destructor of interest. Fix #25199. This detail was documented in [RFC 769]; the implementation was just missing. [breaking-change] The breakage here falls into both obvious and non-obvious cases. The obvious case: if you were relying on the unsoundness this exposes (namely being able to reference dead storage from a destructor, by doing it via a boxed trait object bounded by the lifetime of the dead storage), then this change disallows that. The non-obvious cases: The way dropck works, it causes lifetimes to be extended to longer extents than they covered before. I.e. lifetimes that are attached as trait-bounds may become longer than they were previously. * This includes lifetimes that are only *implicitly* attached as trait-bounds (due to [RFC 599]). So you may have code that was e.g. taking a parameter of type `&'a Box<Trait>` (which expands to `&'a Box<Trait+'a>`), that now may need to be assigned type `&'a Box<Trait+'static>` to ensure that `'a` is not inadvertantly inferred to a region that is actually too long. (See commit ee06263 for an example of this.) [RFC 769]: https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#the-drop-check-rule [RFC 599]: https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md
|
💔 Test failed - auto-mac-64-opt |
|
(sigh, my make check hadn't gotten that far; will fix.) |
|
@nikomatsakis wrote:
Do we dare consider making such a change before the release? I do admit, it is tempting -- especially if it helps either reduce the fallout from this fix, or at least improves the error messages one gets (and/or makes the user experience over all more consistent). So, yes, we probably should consider doing this. Maybe in tandem with testing this PR against @brson's crater, we should also test this PR plus your revision to rust-lang/rfcs#599 as well. |
|
@bors r=nikomatsakis 8654dfb p=100 |
dropck: must assume `Box<Trait + 'a>` has a destructor of interest. Fix #25199. This detail was documented in [RFC 769]; the implementation was just missing. [breaking-change] The breakage here falls into both obvious and non-obvious cases. The obvious case: if you were relying on the unsoundness this exposes (namely being able to reference dead storage from a destructor, by doing it via a boxed trait object bounded by the lifetime of the dead storage), then this change disallows that. The non-obvious cases: The way dropck works, it causes lifetimes to be extended to longer extents than they covered before. I.e. lifetimes that are attached as trait-bounds may become longer than they were previously. * This includes lifetimes that are only *implicitly* attached as trait-bounds (due to [RFC 599]). So you may have code that was e.g. taking a parameter of type `&'a Box<Trait>` (which expands to `&'a Box<Trait+'a>`), that now may need to be assigned type `&'a Box<Trait+'static>` to ensure that `'a` is not inadvertantly inferred to a region that is actually too long. (See commit ee06263 for an example of this.) [RFC 769]: https://github.com/rust-lang/rfcs/blob/master/text/0769-sound-generic-drop.md#the-drop-check-rule [RFC 599]: https://github.com/rust-lang/rfcs/blob/master/text/0599-default-object-bound.md
|
⌛ Testing commit 8654dfb with merge 3906edf... |
brson
added
beta-accepted
alexcrichton
added
the
beta-nominated
brson
removed
the
beta-nominated
|
On Sat, May 09, 2015 at 12:39:41AM -0700, Felix S Klock II wrote:
I don't think we can do it before the release. However, I have a |
dropck: must assume
Box<Trait + 'a>has a destructor of interest.Fix #25199.
This detail was documented in RFC 769; the implementation was just missing.
[breaking-change]
The breakage here falls into both obvious and non-obvious cases.
The obvious case: if you were relying on the unsoundness this exposes (namely being able to reference dead storage from a destructor, by doing it via a boxed trait object bounded by the lifetime of the dead storage), then this change disallows that.
The non-obvious cases: The way dropck works, it causes lifetimes to be extended to longer extents than they covered before. I.e. lifetimes that are attached as trait-bounds may become longer than they were previously.
&'a Box<Trait>(which expands to&'a Box<Trait+'a>), that now may need to be assigned type&'a Box<Trait+'static>to ensure that'ais not inadvertantly inferred to a region that is actually too long. (See commit ee06263 for an example of this.)