Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

RFC: Rvalue lifetimes #3511

Closed
nikomatsakis opened this Issue · 11 comments

6 participants

@nikomatsakis

We need to clarify our story on rvalues. @graydon, @bblum and I have discussed this. I wrote up my latest thoughts on the situation more here:

http://smallcultfollowing.com/babysteps/blog/2012/09/15/rvalue-lifetimes/

Currently I am tending towards the "DWIM"-style rule (obviously I'd have to spell it out more specifically). I was thinking that a purely inference-based solution, while flexible, seems less specifiable. Or rather, to standardize it, you'd have to specify the inference technique, which seems undesirable.

@nikomatsakis

Here is the rule I had in mind (based on C++). For any given temporary, you would walk up the AST, examining the parent nodes, to find the first one for which a rule is defined:

  • match rvalue {...}: lifetime of rvalue is IES
  • foo(..., rvalue, ...): lifetime of rvalue is IES
  • ... + ... (etc): lifetime of rvalue is IES
  • x = ..., lifetime of rvalue is block where x is declared
  • lvalue = ..., where lvalue is some path owned by a local x, same as previous line
  • '...;' (any statement), lifetime is that statement
  • loop or function body: loop is that block

"IES" == innermost enclosing statement

I think this more-or-less works "as expected" in all cases. But I'm not entirely sure, probably the thing to do is to implement it and see what kind of borrowck errors result.

It might be easier to specify the rule by the cases where the result is not the innermost enclosing statement...

@bblum

I'm not sure how I feel about this issue. Personally, I might even prefer if the compiler punted on some of the harder cases, saying something like "I couldn't find a lifetime for this rvalue that wouldn't be unexpected - please use a temporary to manually indicate the intended lifetime".

@graydon

Isn't the region inference algorithm something we'll need to spec anyway? Just for the sake of defining borrow/use errors?

@nikomatsakis

@graydon yes I realized that later.

@catamorphism

Just a note for the future that part of resolving this issue means refactoring trans to be more declarative about how it handles cleanups (i.e. relating them to AST nodes rather than calling add_cleanup hither and yon).

@catamorphism catamorphism referenced this issue from a commit
@catamorphism catamorphism Make borrowck's notion of scopes consistent with trans's notion of sc…
…opes

This eliminates an ICE in trans where the scope for a particular
borrow was a statement ID, but the code in trans that does cleanups
wasn't finding the block with that scope. As per #3860

preserve looks at a node ID to see if it's for a statement -- if it
is, it uses the enclosing scope instead when updating the map that
trans looks at later.

I added a comment noting that this is not the best fix (since it may
cause boxes to be frozen for longer than necessary) and referring
to #3511.

r=nmatsakis
9d67267
@ILyoan ILyoan referenced this issue from a commit in webconv/rust
@catamorphism catamorphism Make borrowck's notion of scopes consistent with trans's notion of sc…
…opes

This eliminates an ICE in trans where the scope for a particular
borrow was a statement ID, but the code in trans that does cleanups
wasn't finding the block with that scope. As per #3860

preserve looks at a node ID to see if it's for a statement -- if it
is, it uses the enclosing scope instead when updating the map that
trans looks at later.

I added a comment noting that this is not the best fix (since it may
cause boxes to be frozen for longer than necessary) and referring
to #3511.

r=nmatsakis
f6e5499
@nickdesaulniers nickdesaulniers referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@nikomatsakis

nominating for milestone #1

@graydon

accepted for well-defined milestone

@nikomatsakis

doing some hacking on this

@nikomatsakis nikomatsakis was assigned
@jayanderson jayanderson referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@nikomatsakis nikomatsakis referenced this issue from a commit in nikomatsakis/rust
@nikomatsakis nikomatsakis Issue #3511 - Rationalize temporary lifetimes.
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.
e7a90d4
@nikomatsakis nikomatsakis referenced this issue from a commit in nikomatsakis/rust
@nikomatsakis nikomatsakis Issue #3511 - Rationalize temporary lifetimes.
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.
419ac4a
@LiquidHelium LiquidHelium referenced this issue from a commit
@nikomatsakis nikomatsakis Issue #3511 - Rationalize temporary lifetimes.
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.
bbf7791
@adrientetar adrientetar referenced this issue from a commit in adrientetar/rust-tuts
@adrientetar adrientetar Cleanup now that rust-lang/rust#3511 is closed 9ef91ab
@huonw
Owner

Did #11585 address this completely?

@nikomatsakis

Yes, with the exception of writing up the rules in documentation.

@nikomatsakis nikomatsakis referenced this issue from a commit in nikomatsakis/rust
@nikomatsakis nikomatsakis Add test case for #3243, which was fixed as part of fix for #3511.
(Lifetime of stack allocated vectors was not being enforced)

Closes #3243.
81d5eff
@nikomatsakis nikomatsakis referenced this issue from a commit in nikomatsakis/rust
@nikomatsakis nikomatsakis Add test case for #3243, which was fixed as part of fix for #3511.
(Lifetime of stack allocated vectors was not being enforced)

Closes #3243.
afd8df6
@FlaPer87
Collaborator

As per last @nikomatsakis comment, this issue was completely addressed by #11585 and the only task left is writing the proper documentation for it, which is covered by #12032

@FlaPer87 FlaPer87 closed this
@Sawyer47 Sawyer47 referenced this issue from a commit in Sawyer47/rust
@Sawyer47 Sawyer47 Fix FIXME #3511 in Dlist code
Issue #3511 was closed, but dlist.rs contained a FIXME for it.
9b5bdfc
@bors bors referenced this issue from a commit
@bors bors auto merge of #14417 : Sawyer47/rust/dlist-fixme, r=huonw
Issue #3511 was closed, but dlist.rs contained a FIXME for it.
759517c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.