Relax self-borrow lifetime in Branch.upstream() #400
Conversation
The following code currently does not compile:
```rust
fn get_upstreams<'repo>(repo: &'repo git2::Repository) ->
Vec<(git2::Branch<'repo>, Option<git2::Branch<'repo>>)>
{
repo
.branches(None) // pub fn branches(&self, filter: Option<git2::BranchType>) -> Result<git2::Branches, git2::Error>
.unwrap() // git2::Branches<'a>: Iterator<Item = Result<(git2::Branch<'repo>, git2::BranchType), git2::Error>>
.flatten() // Iterator<Item = (git2::Branch<'repo>, git2::BranchType)>
.map(|(branch, _): (git2::Branch<'repo>, _)| {
let upstream: Option<git2::Branch<'repo>> =
branch
.upstream() // pub fn upstream<'a>(&'a self) -> Result<Branch<'a>, Error>
.ok();
(branch, upstream)
}) // Iterator<Item = (git2::Branch<'repo>, git2::Branch<'repo>)>
.collect()
}
```
Error:
```
error[E0597]: `branch` does not live long enough
--> src/config/data/project.rs:49:55
|
49 | let upstream: Option<git2::Branch<'repo>> = branch.upstream().ok();
| ^^^^^^ borrowed value does not live long enough
...
58 | })
| - borrowed value only lives until here
|
note: borrowed value must be valid for the lifetime 'repo as defined on the method body at 39:30...
--> src/config/data/project.rs:39:30
|
39 | fn local_branches_internal<'repo>(&self, repo: &'repo git2::Repository) -> Result<Vec<(Branch, git2::Branch<'repo>, Option<git2::Branch<'repo>>)>, git2::Error> {
```
This is because the `.upstream()` method is declared with the same
self-borrow lifetime as the return value:
```rust
pub fn upstream<'a>(&'a self) -> Result<Branch<'a>, Error> { /* ... */ }
```
which means that the `.upstream()` call is still borrowing `self` even
after it returns, even though it returns an owned value and not a
reference. Relaxing the self-borrow lifetime allows the above code
sample to compile successfully. The lifetime of the (maybe) returned
upstream `Branch` will also be that of the repository, but otherwise
unrelated to the lifetime of the `Branch` that constructed it.
emlun
force-pushed
the
relax-branch-upstream-self-borrow-lifetime
branch
from
c3a84e2 to
f4b5516
Compare
|
@joshtriplett can you take a look at this? |
|
Any movement on this? |
|
Sorry for the delay, but I can at least add my thoughts here. This new signature is memory unsafe since it returns an arbitrary lifetime that isn't connected to anything else. I don't have a chance to investigate this more closely, but that'll probably need fixing before merging. |
|
Oh right, of course. Looks like I mixed things up when I said
I suppose the signature to represent that would be pub fn upstream(&self) -> Result<Branch<'repo>, Error>Does that make sense? The lifetime of the returned |
|
That would tie it to something yeah but the C code needs to be reviewed to make sure that's the actual lifetime of the object returned. |
|
Ok. I'm afraid I don't know enough C (namely, close to none at all) to do that, but here's the beginning of the call stack as far as I can tell. It looks to me like a Is this useful? I would gladly dig further into this, but I would need some guidance on how to go about tracing the memory management.
|
|
Ok I think that roughly looks good to me, want to update the PR? |
|
Done! |
The lifetime requirement on
&selfin theBranch.upstream()method causes the self-borrow to be extended to the lifetime of the repository, which makes it hard to use. It seems like this lifetime can be relaxed.However, I'm quite new to Rust and even newer to this codebase, so I could very well be missing something here. But at least
cargo teststill succeeds after this. 🙂Below is a motivating code sample along with more details about the issue, copied from the commit message.
The following code currently does not compile:
Error:
This is because the
.upstream()method is declared with the same self-borrow lifetime as the return value:which means that the
.upstream()call is still borrowingselfevenafter it returns, even though it returns an owned value and not a reference. Relaxing the self-borrow lifetime allows the above code sample to compile successfully. The lifetime of the (maybe) returned upstream
Branchwill also be that of the repository, but otherwise unrelated to the lifetime of theBranchthat constructed it.