Issue #3511 - Rationalize temporary lifetimes. #11585

nikomatsakis · 2014-01-15T23:34:53Z

Major changes:

Define temporary scopes in a syntax-based way that basically defaults
to the innermost statement or conditional block, except for in
a let initializer, where we default to the innermost block. Rules
are documented in the code, but not in the manual (yet).
See new test run-pass/cleanup-value-scopes.rs for examples.
Refactors Datum to better define cleanup roles.
Refactor cleanup scopes to not be tied to basic blocks, permitting
us to have a very large number of scopes (one per AST node).
Introduce nascent documentation in trans/doc.rs covering datums and
cleanup in a more comprehensive way.

Major changes: - Define temporary scopes in a syntax-based way that basically defaults to the innermost statement or conditional block, except for in a `let` initializer, where we default to the innermost block. Rules are documented in the code, but not in the manual (yet). See new test run-pass/cleanup-value-scopes.rs for examples. - Refactors Datum to better define cleanup roles. - Refactor cleanup scopes to not be tied to basic blocks, permitting us to have a very large number of scopes (one per AST node). - Introduce nascent documentation in trans/doc.rs covering datums and cleanup in a more comprehensive way.

huonw · 2014-01-15T23:54:47Z

src/test/run-pass/cleanup-rvalue-scopes.rs

+            {
+                // Destructor here does not run until exit from the block,
+                // because value is assigned to.
+                let $pat = $expr;


Is this macro meant to just be $expr; rather than let $pat = $expr;?

(At the very least, the comment seems to be incorrect?)

No, it is meant to be what it is. The point is that, depending on the pattern, temporaries in the rvalue may be promoted to either block or statement.

Oh, I think I commented on the incorrect one... the end_of_stmt macro seems to have an identical comment, even though the behaviour is different.

On Wed, Jan 15, 2014 at 04:02:18PM -0800, Huon Wilson wrote:

(At the very least, the comment seems to be incorrect?)

Yes, that is true.

nikomatsakis · 2014-01-16T01:01:58Z

Ah, I forgot, I added some accessors to convert a &T (and &Option<T>) into a slice without copying. Notable because it's a public API. Just added the commit that makes use of them, which I forgot to push earlier.

luqmana · 2014-01-16T01:14:27Z

src/librustc/middle/trans/meth.rs

+     * Generates the code to convert from a pointer (`~T`, `&T`, etc)
+     * into an object (`~Trait`, `&Trait`, etc). This means creating a
+     * pair where the first word is the pointer and the second word is
+     * an appropriate vtable.


Isn't it the other way around? i.e (vtable, pointer). The code itself is fine since it just uses abi::trt_field_{vtable, box}

brson · 2014-01-16T01:48:41Z

I'm super excited about this! Heroic work, Niko.

pcwalton · 2014-01-16T01:50:14Z

src/librustc/middle/region.rs

+         * Applied to an expression `expr` if `expr` -- or something
+         * owned or partially owned by `expr` -- is going to be
+         * indirectly referenced by a variable in a let statement. In
+         * that case, the "temporary lifetime" or `expr` is extended


nit: "of", not "or"

pcwalton · 2014-01-16T02:29:01Z

Looks good from a first pass. I have just a few nits and comments. Thanks!

… "quirky" parts of LLVM (see e.g. LLVM bug 9900) but also generating better code

nikomatsakis · 2014-01-16T20:20:09Z

@pcwalton I interpreted your comment as r+. I fixed the nits, as well as a minor-ish patch to handle 0-sized data structures in a better way, bypassing an LLVM crash I was encountering. If I misinterpreted your comment, no harm, as I expect it won't land on the first try anyhow. ;)

… channel to be dropped prematurely.

edwardw · 2014-01-17T07:19:00Z

While the issue is still open, I wonder whether the following observation is relevant. I used a freshly build rustc minutes ago from the source.

// this compiles and works
fn xs_1() {
    let xs = ['1','2','3','4','5','6','7','8','9','0'];
    for i in xs.iter() { println!("{:c}", *i); }
}

// this works, too
fn xs_2() {
    for i in "1234567890".chars() { println!("{:c}", i); }
}

// but this doesn't with compiler error
//    error: borrowed value does not live long enough
fn xs_3() {
    for i in ['1','2','3','4','5','6','7','8','9','0'].iter() {
        println!("{:c}", *i);
    }
}

too. Previously I had omitted this case since function calls don't get the same treatment on the RHS, but it's different on the pattern and is more consistent -- the goal is to identify `let` statements where `ref` bindings create interior pointers.

nikomatsakis · 2014-01-17T13:17:32Z

@edwardw ah, thanks for pointing that out. This can be corrected by modifying the way we expand for loops into equivalent code...I'll do that.

nikomatsakis · 2014-01-17T13:18:12Z

Based on the rules I wrote, I would expect the lifetime of the [...] rvalue in your example to be extended to the innermost enclosing statement, which is the for loop itself in your example.

so that the "innermost enclosing statement" used for rvalue temporaries matches up with user expectations

nikomatsakis · 2014-01-17T15:12:39Z

@eddyb sigh. I hate that. I wonder if it is possible to have license text that omits the date, or maybe have a script to update the dates somehow based on git commit information :)

@pcwalton

… r=pcwalton Major changes: - Define temporary scopes in a syntax-based way that basically defaults to the innermost statement or conditional block, except for in a `let` initializer, where we default to the innermost block. Rules are documented in the code, but not in the manual (yet). See new test run-pass/cleanup-value-scopes.rs for examples. - Refactors Datum to better define cleanup roles. - Refactor cleanup scopes to not be tied to basic blocks, permitting us to have a very large number of scopes (one per AST node). - Introduce nascent documentation in trans/doc.rs covering datums and cleanup in a more comprehensive way. r? @pcwalton

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

huonw reviewed Jan 15, 2014
View reviewed changes

nikomatsakis added 2 commits January 15, 2014 19:35

Use as_slice() method on option

e71571a

Remove FIXMEs and add license

6badef4

luqmana reviewed Jan 16, 2014
View reviewed changes

Cleanup trait callees

84f33fb

pcwalton reviewed Jan 16, 2014
View reviewed changes

nikomatsakis added 2 commits January 16, 2014 05:56

Remove typo

7ff6b09

Consider all zero-sized data structures to be voidish, bypassing some…

14b0abf

… "quirky" parts of LLVM (see e.g. LLVM bug 9900) but also generating better code

alexcrichton mentioned this pull request Jan 16, 2014

cancels of immediate rvalue cleanups are broken #9758

Closed

Further refine treatment of voidish arrays

4b52d89

nikomatsakis added 3 commits January 16, 2014 18:47

Fix test to account for new temporary lifetime rules, which cause the…

fd31830

… channel to be dropped prematurely.

Fix uninit() intrinsic when used with empty types

76c9028

Distinguish zero-size types from those that we return as void

5e7657f

Make main() public in uninit-empty-types

0c916c5

nikomatsakis added 2 commits January 17, 2014 08:04

Link lifetimes in let patterns just as we do for match patterns

56f4d18

Change expansion of for loop to use a match statement

b1da8c6

so that the "innermost enclosing statement" used for rvalue temporaries matches up with user expectations

nikomatsakis added 3 commits January 17, 2014 10:13

Update year on license header

578b9d1

Update years on more license headers

483ae32

Adjust comments in test case

b520c2f

bors closed this Jan 17, 2014

This was referenced Jan 17, 2014

Fix semantics of let _ = ... not to drop #10488

Closed

Add a lint for lets/matches which won't do anything #11122

Closed

huonw mentioned this pull request Jan 27, 2014

RFC: Rvalue lifetimes #3511

Closed

alexcrichton mentioned this pull request Mar 26, 2014

Destructors and wild-card patterns (when do destructors run?) #3181

Closed

nikomatsakis deleted the issue-3511-rvalue-lifetimes branch March 30, 2016 16:15

pnkfelix mentioned this pull request May 30, 2018

Lifetimes: Shouldn't the compiler figure out and prolong lifetimes of temporary values where it makes sense? #49118

Closed

This was referenced Oct 16, 2018

File instance dropped before leaving scope #54494

Closed

new scoping rules for safe dtors can yield spurious semi-colon or trailing unit expr #21114

Closed

m-ou-se mentioned this pull request Sep 15, 2021

Allow panic!("{}", computed_str) in const fn. #88954

Merged

dtolnay mentioned this pull request Nov 29, 2021

Emit simpler code from format_args #91359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3511 - Rationalize temporary lifetimes. #11585

Issue #3511 - Rationalize temporary lifetimes. #11585

nikomatsakis commented Jan 15, 2014

huonw Jan 15, 2014

huonw Jan 16, 2014

nikomatsakis Jan 16, 2014

huonw Jan 16, 2014

nikomatsakis Jan 16, 2014

nikomatsakis commented Jan 16, 2014

luqmana Jan 16, 2014

nikomatsakis Jan 16, 2014

brson commented Jan 16, 2014

pcwalton Jan 16, 2014

pcwalton commented Jan 16, 2014

nikomatsakis commented Jan 16, 2014

edwardw commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

Issue #3511 - Rationalize temporary lifetimes. #11585

Issue #3511 - Rationalize temporary lifetimes. #11585

Conversation

nikomatsakis commented Jan 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Jan 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brson commented Jan 16, 2014

Choose a reason for hiding this comment

pcwalton commented Jan 16, 2014

nikomatsakis commented Jan 16, 2014

edwardw commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014

nikomatsakis commented Jan 17, 2014