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

Properly handle mutability for awaited futures #239

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

borchero
Copy link
Contributor

@borchero borchero commented Apr 6, 2024

Motivation

Currently, the following code fails to compile:

#[rstest]
#[case(async { 3 })]
async fn my_test(
    #[case]
    #[future(awt)]
    mut a: u8,
) {
    a = 4;
    assert_eq!(a, 4);
}

as (the inner function) expands to

async fn my_test(mut a: impl std::future::Future<Output = u8>) {
    let a = a.await;
    {
        a = 4;  // <-- this is an issue, `a` is not mutable, but the future is!
        assert_eq!(a, 4);
    }
}

This is most likely undesired: by marking the awaited future value as mut, one would expect that a is mutable.

Changes

This PR augments the mutability identifier to result in the following expanded code that now compiles as expected:

async fn my_test(a: impl std::future::Future<Output = u8>) {  // <-- no `mut` for the future anymore -- await NEVER requires `mut`
    let mut a = a.await;  // <-- this is now correctly marked mutable
    {
        a = 4;
        assert_eq!(a, 4);
    }
}

Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Can you add at least an e2e tests, an a line in change log. Maybe it would be good also to have some unit tests here.

rstest_macros/src/refident.rs Outdated Show resolved Hide resolved
rstest_macros/src/render/apply_argumets.rs Outdated Show resolved Hide resolved
@la10736
Copy link
Owner

la10736 commented Apr 7, 2024

@borchero
Copy link
Contributor Author

borchero commented Apr 7, 2024

Take a look to https://github.com/la10736/rstest/tree/my_await_mut

Nice! Thanks for taking over, this looks much better 😁 I'm not too familiar with writing procedural macros yet 👀 shall I close this PR in favor of your changes?

@la10736
Copy link
Owner

la10736 commented Apr 7, 2024

No, include these changes like are yours and handle tests and change log.
I didn't do it if you didn't prompt me : smile:

@borchero
Copy link
Contributor Author

borchero commented Apr 7, 2024

@la10736 latest change now includes your suggestion for improvement along with e2e and unit tests :)

@borchero borchero requested a review from la10736 April 7, 2024 20:33
Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

Nice. Can you check my notes?

rstest_macros/src/render/apply_argumets.rs Outdated Show resolved Hide resolved
rstest_macros/src/refident.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@borchero borchero requested a review from la10736 April 8, 2024 20:05
Copy link
Owner

@la10736 la10736 left a comment

Choose a reason for hiding this comment

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

LGTM

@la10736 la10736 merged commit 3c2fb9c into la10736:master Apr 9, 2024
2 checks passed
@borchero borchero deleted the await-mut branch April 9, 2024 08:30
@borchero
Copy link
Contributor Author

borchero commented Apr 9, 2024

Thanks! Can you create a new release with this change @la10736? 🙏🏼

@la10736
Copy link
Owner

la10736 commented Apr 9, 2024

I'll try to do it later in the day

@la10736
Copy link
Owner

la10736 commented Apr 9, 2024

@borchero Done

@borchero
Copy link
Contributor Author

borchero commented Apr 9, 2024

Thanks a lot!

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