Skip to content

Change signature of from_async_fn to allow capturing the context#4215

Merged
jedel1043 merged 1 commit intomainfrom
new-from-async-fn
Mar 31, 2025
Merged

Change signature of from_async_fn to allow capturing the context#4215
jedel1043 merged 1 commit intomainfrom
new-from-async-fn

Conversation

@jedel1043
Copy link
Member

This allows carrying the context through await points, making it much more ergonomic to use for big async functions.

@jedel1043 jedel1043 added the A-Enhancement New feature or request label Mar 23, 2025
@jedel1043 jedel1043 added this to the next-release milestone Mar 23, 2025
@jedel1043 jedel1043 requested a review from a team March 23, 2025 01:48
@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 50,254 50,254 0
Passed 46,656 46,656 0
Ignored 1,634 1,634 0
Failed 1,964 1,964 0
Panics 0 0 0
Conformance 92.84% 92.84% 0.00%

@Razican
Copy link
Member

Razican commented Mar 23, 2025

Wow, the cool thing about this is that we can use non-desugared normal async functions:

async fn get_record(
    _this: &JsValue,
    _args: &[JsValue],
    context: &RefCell<&mut Context>,
) -> JsResult<JsValue> {
    todo!()
}

@jedel1043
Copy link
Member Author

@Razican That's always been the case if you don't use the arguments inside the function. The moment you try to get any argument, the borrow checker complains that you're capturing a lifetime for too long.

Also, I think this is only valid for edition 2021, because on edition 2024 all async functions capture the lifetime of all arguments by default.

@Razican
Copy link
Member

Razican commented Mar 29, 2025

@Razican That's always been the case if you don't use the arguments inside the function. The moment you try to get any argument, the borrow checker complains that you're capturing a lifetime for too long.

Also, I think this is only valid for edition 2021, because on edition 2024 all async functions capture the lifetime of all arguments by default.

Hmmm interesting, I'm able to use it with edition 2024, using the arguments and so on:

/// Gets a record from the database.
    async fn get(
        this: &JsValue,
        args: &[JsValue],
        context: &RefCell<&mut Context>,
    ) -> JsResult<JsValue> {
        let sys_id = args.get_or_undefined(0).as_string().ok_or_else(|| {
            JsNativeError::typ()
                .with_message("you must specify a record ID to fetch from the database")
        });
        let realm = context.borrow().realm().clone();

        todo!() // actually some more code that uses the host_defined() from realm and so on
    }

@jedel1043
Copy link
Member Author

jedel1043 commented Mar 29, 2025

@Razican Ohhhh, maybe rustc changed the desugaring of async functions?

Copy link
Member

@HalidOdat HalidOdat 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! This greatly improves the API's ergonomics. :)

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

This is really great :) I also saw some comments in Matrix about possibly hiding the RefCell and have Context use interior mutability and cheap cloning, but for now, this improves the API quite a lot.

@jedel1043 jedel1043 added this pull request to the merge queue Mar 31, 2025
Merged via the queue into main with commit 6b68df5 Mar 31, 2025
14 checks passed
@jedel1043 jedel1043 deleted the new-from-async-fn branch March 31, 2025 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants