Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix borrowing issues #5

Merged
merged 4 commits into from
Jul 7, 2021
Merged

Fix borrowing issues #5

merged 4 commits into from
Jul 7, 2021

Conversation

SabrinaJewson
Copy link

Previously, code like this would fail to compile:

#[context("context")]
async fn borrows(val: &mut u32) -> anyhow::Result<&u32> {
    Ok(&*val)
}

With this error message:

error[E0373]: async block may outlive the current function, but it borrows `val`, which is owned by the current function
 --> src/lib.rs:4:1
  |
4 | #[context("context")]
  | ^^^^^^^^^^^^^^^^^^^^^ may outlive borrowed value `val`
5 | async fn borrows(val: &mut u32) -> anyhow::Result<&u32> {
6 |     Ok(&*val)
  |          --- `val` is borrowed here
  |
note: async block is returned here
 --> src/lib.rs:4:1
  |
4 | #[context("context")]
  | ^^^^^^^^^^^^^^^^^^^^^
  = note: this error originates in an attribute macro (in Nightly builds, run with -Z macro-backtrace for more info)
help: to force the async block to take ownership of `val` (and any other referenced variables), use the `move` keyword
  |
4 | move #[context("context")]
  |

This can be fixed by using an async move {} block instead of an async {} block.

@andrewhickman
Copy link
Owner

andrewhickman commented Jul 3, 2021

Thanks for the PR!

Looks like the sync version has the same issue. There's a bit of a tradeoff between supporting code like

#[context("failed to parse config at `{}`", path.as_ref().display())]
pub fn parse_config(path: impl AsRef<Path>) -> anyhow::Result<u32> {
    let text = read_to_string(path.as_ref())?;
    Ok(text.parse()?)
}

which fails when we use a move closure/async move block, and your example.

Do you think a way to opt into the move semantics would be reasonable? e.g.

#[context(move, "context")]
async fn borrows(val: &mut u32) -> anyhow::Result<&u32> {

@SabrinaJewson
Copy link
Author

Oh, you're right - I didn't consider that case. #[context(move)] seems like a better solution. I can implement it tomorrow in this PR, but feel free to do it now if you want.

@SabrinaJewson SabrinaJewson changed the title Use async move block to fix borrowing issues Fix borrowing issues Jul 4, 2021
@SabrinaJewson
Copy link
Author

I have updated the PR to add support for the move option. Additionally, I implemented a commit that should reduce incorrect borrowck errors in synchronous functions by forcing the closure to be FnOnce.

Copy link
Owner

@andrewhickman andrewhickman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks! The trick with forcing the closure to be FnOnce is quite cool. I just had a couple comments about variable hygiene, then it should be ready to merge

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@andrewhickman andrewhickman merged commit 5b1b9db into andrewhickman:master Jul 7, 2021
@andrewhickman
Copy link
Owner

Thanks for the PR! I'll release version 0.2.0 with these changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants