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

io-async: fix warnings for latest nightly (1.75.0) #515

Merged
merged 2 commits into from
Oct 16, 2023

Conversation

rmsyn
Copy link
Contributor

@rmsyn rmsyn commented Oct 15, 2023

Fixes cargo check warnings for the latest nightly version (1.75.0).

Recommended public API signatures for async trait functions has changed to recommending the desugared version for compatibility with different async runtimes.

@rmsyn rmsyn requested a review from a team as a code owner October 15, 2023 21:39
@Dirbaio
Copy link
Member

Dirbaio commented Oct 15, 2023

The warning against using async fn in trait definitions is because many async runtimes in non-embedded land default to requiring Send futures.

There's no way to express that with the async fn syntax, it is only possible with the -> impl Future<..> + Send syntax. This is why the warning recommends doing that. If you write your trait with async fn (or with -> impl Future your trait won't be usable with these runtimes. Only -> impl Future + Send is.

In the embedded case, all existing runtimes and HALs don't require Send futures (and are unlikely to require it, you rarely have multicore and when you do you dedicate each core to concrete tasks, instead of running a single multi-core executor). So in this case we should just allow the lint with #[allow(async_fn_in_trait)]

@rmsyn
Copy link
Contributor Author

rmsyn commented Oct 15, 2023

In the embedded case, all existing runtimes and HALs don't require Send futures (and are unlikely to require it, you rarely have multicore and when you do you dedicate each core to concrete tasks, instead of running a single multi-core executor). So in this case we should just allow the lint with #[allow(async_fn_in_trait)]

Ok, I finished with the remaining traits using the rewrite to the desugared version. I can change to use the #[allow(async_fn_trait)]. I'm also currently writing a HAL for a multicore platform, would these traits be useful in something like an RTOS that wanted to take advantage of multiple cores? (I can also rewrite with the + Send in the function signatures).

I think leaving it with the desugared version also might be better for any future changes that want to add the + Send trait, even if we leave it without it for now. Since this is functionally equivalent without + Send to the sugared async fn version.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 15, 2023

adding + Send breaks compatibility with all existing embedded async HALs, that's not an option.

Currently,
All embedded HALs are in the "HAL not implementing Send" category (embassy-nrf, embassy-stm32, embassy-rp, espxxx-hal).
All embedded executors are in the "Executor not requiring Send" category. (embassy-executor, RTIC, edge-executor, lilos).

This is an overview of the compatibility we get depending on the choice in the trait declaration:

Trait Executor requiring Send Executor not requiring Send HAL implementing Send HAL not implementing Send
async fn ⚠️ 🟢 🟢 🟢
fn -> impl Future ⚠️ 🟢 🟢 🟢
fn -> impl Future + Send 🟢 🟢 🟢

(note that async fn and fn -> impl Future are exactly equivalent, the 1st desugars to the latter)

🟢 = works
❌ = doesn't work
⚠️ = works for generic code that doesn't spawn, works for code that spawns with concrete types. Fails only with generic code that does spawn. Example:

trait Foo {
    async fn foo(self); // future may or may not be Send.
}

struct MyFoo;
impl Foo for MyFoo { 
    async fn foo(self) {
        // future that *does* implement Send.
    }
}

async fn use<T: Foo>(foo: T) {
    // works: nothing here requires `Send`, even if you do end up running this
    // code in an executor that does require `Send`.
    foo.foo().await;
}

fn spawn_concrete_type(foo: MyFoo) {
    // works: the compiler can see `t.foo()` is Send even if the signature for `foo()`
    // doesn't require `+ Send`, because it knows the concrete type.
    tokio::spawn(t.foo()) 
}

fn spawn_generic<T: Foo>(t: T) {
    // Doesn't work: the future for `t.foo()` may or may not be `Send` depending on `T`.
    tokio::spawn(t.foo()) 
}

So, first of all: there's no advantage in switching from async fn to -> impl Future (what this PR does), they're exactly equivalent.

About compat with executors that require Send, we can either:

  1. Do nothing, stay with async fn.
  2. Add a 2nd set of traits SendI2c, SendSpiBus, SendSpiDevice etc that are the same, but with -> impl Future + Send.

I prefer doing 1, because:

  • Two sets of traits split the ecosystem in two.
  • we're early in the ecosystem development phase, the benefit of Send* traits is still unclear
    • How many embedded executors we'll have that actually require Send? I think for MCUs it'll be rare. For embedded linux there's Tokio etc that do require Send, but they usually support creating non-Send executors too, you can just use that
    • How common will it be to hit the problematic code pattern? (the spawn_generic() above).
  • There's a new feature in the works, Return Type Notation (RTN), that will solve this. It allows adding the Send restriction at use site instead of at trait definition site. It'll allow using async fn traits in Send-requiring executors, like this:
fn spawn_generic<T>(t: T)
where T: Foo + Send,
      T::foo(): Send, // RTN! means "the future returned by foo() is Send"
{
    tokio::spawn(t.foo()) // requires Send
}
  • Once RTN lands, the Send* traits will no longer be idiomatic, but we won't be able to remove them them because it'll be a breaking change.

@rmsyn
Copy link
Contributor Author

rmsyn commented Oct 15, 2023

Do nothing, stay with async fn.

After reading your write-up, I agree. I'm new to async stuff, and was just going off the warning messages.

Once RTN lands, the Send* traits will no longer be idiomatic, but we won't be able to remove them them because it'll be a breaking change.

That completely makes sense, I was not aware of RTN.

I can make the changes to use the #[allow(async_fn_trait)] to fix the CI builds.

rmsyn added a commit to rmsyn/embedded-hal that referenced this pull request Oct 15, 2023
Adds `#[allow(async_fn_in_trait)]` to disable the lint in `async` trait
functions recommending `Send` in the return type.

Adding the type annotation is currently unstable, has little-to-no
utility in current crates using `embedded-hal`, and will break those users.

See
<rust-embedded#515 (comment)>
for details and discussion.
@rmsyn
Copy link
Contributor Author

rmsyn commented Oct 15, 2023

Looks like stable builds are failing for an unknown lint failure. Not sure how to block this out with conditional compilation without bringing in a new dependency.

@Dirbaio
Copy link
Member

Dirbaio commented Oct 15, 2023

you can add a single #![allow(async_fn_in_trait)] at the top of lib.rs to silence the lint for the whole crate.

clippy and rustdoc fail becuase you have to update nightly here
https://github.com/rust-embedded/embedded-hal/blob/master/.github/workflows/clippy.yml#L18
https://github.com/rust-embedded/embedded-hal/blob/master/.github/workflows/rustdoc.yml#L16

Adds `#[allow(async_fn_in_trait)]` to disable the lint in `async` trait
functions recommending `Send` in the return type.

Adding the type annotation is currently unstable, has little-to-no
utility in current crates using `embedded-hal`, and will break those users.

See
<rust-embedded#515 (comment)>
for details and discussion.
@Dirbaio Dirbaio added this pull request to the merge queue Oct 16, 2023
Merged via the queue into rust-embedded:master with commit 454f17f Oct 16, 2023
12 checks passed
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