From a52965dc5d3d0c706310998d3eda8bc15cd45b02 Mon Sep 17 00:00:00 2001 From: Brezak Date: Tue, 22 Jul 2025 20:56:46 +0200 Subject: embassy-executor: unsafe tasks as unsafe --- embassy-executor-macros/src/macros/task.rs | 15 ++++++++++++- embassy-executor/CHANGELOG.md | 1 + embassy-executor/tests/ui.rs | 1 + embassy-executor/tests/ui/task_safety_attribute.rs | 25 ++++++++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) create mode 100644 embassy-executor/tests/ui/task_safety_attribute.rs diff --git a/embassy-executor-macros/src/macros/task.rs b/embassy-executor-macros/src/macros/task.rs index 1c5e3571d..f01cc3b6c 100644 --- a/embassy-executor-macros/src/macros/task.rs +++ b/embassy-executor-macros/src/macros/task.rs @@ -120,6 +120,18 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { task_inner.vis = syn::Visibility::Inherited; task_inner.sig.ident = task_inner_ident.clone(); + // Forcefully mark the inner task as safe. + // SAFETY: We only ever call task_inner in functions + // with the same safety preconditions as task_inner + task_inner.sig.unsafety = None; + let task_body = task_inner.body; + task_inner.body = quote! { + #[allow(unused_unsafe, reason = "Not all function bodies may require being in an unsafe block")] + unsafe { + #task_body + } + }; + // assemble the original input arguments, // including any attributes that may have // been applied previously @@ -186,6 +198,7 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { // Copy the generics + where clause to avoid more spurious errors. let generics = &f.sig.generics; let where_clause = &f.sig.generics.where_clause; + let unsafety = &f.sig.unsafety; let result = quote! { // This is the user's task function, renamed. @@ -196,7 +209,7 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { #task_inner #(#task_outer_attrs)* - #visibility fn #task_ident #generics (#fargs) -> #embassy_executor::SpawnToken #where_clause{ + #visibility #unsafety fn #task_ident #generics (#fargs) -> #embassy_executor::SpawnToken #where_clause{ #task_outer_body } diff --git a/embassy-executor/CHANGELOG.md b/embassy-executor/CHANGELOG.md index 914863a83..7404961f3 100644 --- a/embassy-executor/CHANGELOG.md +++ b/embassy-executor/CHANGELOG.md @@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added support for `-> impl Future` in `#[task]` - Fixed `Send` unsoundness with `-> impl Future` tasks - Marked `Spawner::for_current_executor` as `unsafe` +- `#[task]` now properly marks the generated function as unsafe if the task is marked unsafe ## 0.7.0 - 2025-01-02 diff --git a/embassy-executor/tests/ui.rs b/embassy-executor/tests/ui.rs index 7757775ee..8b83cd368 100644 --- a/embassy-executor/tests/ui.rs +++ b/embassy-executor/tests/ui.rs @@ -32,4 +32,5 @@ fn ui() { t.compile_fail("tests/ui/self.rs"); t.compile_fail("tests/ui/type_error.rs"); t.compile_fail("tests/ui/where_clause.rs"); + t.pass("tests/ui/task_safety_attribute.rs"); } diff --git a/embassy-executor/tests/ui/task_safety_attribute.rs b/embassy-executor/tests/ui/task_safety_attribute.rs new file mode 100644 index 000000000..ab5a2f99f --- /dev/null +++ b/embassy-executor/tests/ui/task_safety_attribute.rs @@ -0,0 +1,25 @@ +#![cfg_attr(feature = "nightly", feature(impl_trait_in_assoc_type))] +#![deny(unused_unsafe)] + +use std::mem; + +#[embassy_executor::task] +async fn safe() {} + +#[embassy_executor::task] +async unsafe fn not_safe() {} + +#[export_name = "__pender"] +fn pender(_: *mut ()) { + // The test doesn't link if we don't include this. + // We never call this anyway. +} + +fn main() { + let _forget_me = safe(); + // SAFETY: not_safe has not safety preconditions + let _forget_me2 = unsafe { not_safe() }; + + mem::forget(_forget_me); + mem::forget(_forget_me2); +} -- cgit From 1b42e624246f9355a91ef98ddf96d5af1b9b3687 Mon Sep 17 00:00:00 2001 From: Brezak Date: Wed, 23 Jul 2025 19:20:09 +0200 Subject: embassy-executor: explicitly return impl Future in task inner task --- embassy-executor-macros/src/macros/task.rs | 78 ++++++++++++++-------- .../tests/ui/nonstatic_struct_elided.stderr | 14 ++++ 2 files changed, 66 insertions(+), 26 deletions(-) diff --git a/embassy-executor-macros/src/macros/task.rs b/embassy-executor-macros/src/macros/task.rs index f01cc3b6c..5b360b128 100644 --- a/embassy-executor-macros/src/macros/task.rs +++ b/embassy-executor-macros/src/macros/task.rs @@ -112,25 +112,11 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { } } - let task_ident = f.sig.ident.clone(); - let task_inner_ident = format_ident!("__{}_task", task_ident); - - let mut task_inner = f.clone(); - let visibility = task_inner.vis.clone(); - task_inner.vis = syn::Visibility::Inherited; - task_inner.sig.ident = task_inner_ident.clone(); - - // Forcefully mark the inner task as safe. - // SAFETY: We only ever call task_inner in functions - // with the same safety preconditions as task_inner - task_inner.sig.unsafety = None; - let task_body = task_inner.body; - task_inner.body = quote! { - #[allow(unused_unsafe, reason = "Not all function bodies may require being in an unsafe block")] - unsafe { - #task_body - } - }; + // Copy the generics + where clause to avoid more spurious errors. + let generics = &f.sig.generics; + let where_clause = &f.sig.generics.where_clause; + let unsafety = &f.sig.unsafety; + let visibility = &f.vis; // assemble the original input arguments, // including any attributes that may have @@ -143,6 +129,51 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { )); } + let task_ident = f.sig.ident.clone(); + let task_inner_ident = format_ident!("__{}_task", task_ident); + + let task_inner_future_output = match &f.sig.output { + ReturnType::Default => quote! {-> impl ::core::future::Future}, + // Special case the never type since we can't stuff it into a `impl Future` + ReturnType::Type(arrow, maybe_never) if matches!(**maybe_never, Type::Never(_)) => quote! { + #arrow #maybe_never + }, + // Grab the arrow span, why not + ReturnType::Type(arrow, typ) if f.sig.asyncness.is_some() => quote! { + #arrow impl ::core::future::Future + }, + // We assume that if `f` isn't async, it must return `-> impl Future<...>` + // This is checked using traits later + ReturnType::Type(arrow, typ) => quote! { + #arrow #typ + }, + }; + + let task_inner_body = if errors.is_empty() { + quote! { + #f + + // SAFETY: All the preconditions to `#task_ident` apply to + // all contexts `#task_inner_ident` is called in + #unsafety { + #task_ident(#(#full_args,)*) + } + } + } else { + quote! { + async {::core::todo!()} + } + }; + + let task_inner = quote! { + #visibility fn #task_inner_ident #generics (#fargs) + #task_inner_future_output + #where_clause + { + #task_inner_body + } + }; + let spawn = if returns_impl_trait { quote!(spawn) } else { @@ -185,7 +216,7 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { unsafe { __task_pool_get(#task_inner_ident).#spawn(move || #task_inner_ident(#(#full_args,)*)) } }; - let task_outer_attrs = task_inner.attrs.clone(); + let task_outer_attrs = &f.attrs; if !errors.is_empty() { task_outer_body = quote! { @@ -195,11 +226,6 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { }; } - // Copy the generics + where clause to avoid more spurious errors. - let generics = &f.sig.generics; - let where_clause = &f.sig.generics.where_clause; - let unsafety = &f.sig.unsafety; - let result = quote! { // This is the user's task function, renamed. // We put it outside the #task_ident fn below, because otherwise @@ -226,7 +252,7 @@ fn check_arg_ty(errors: &mut TokenStream, ty: &Type) { impl<'a, 'ast> Visit<'ast> for Visitor<'a> { fn visit_type_reference(&mut self, i: &'ast syn::TypeReference) { - // only check for elided lifetime here. If not elided, it's checked by `visit_lifetime`. + // Only check for elided lifetime here. If not elided, it's checked by `visit_lifetime`. if i.lifetime.is_none() { error( self.errors, diff --git a/embassy-executor/tests/ui/nonstatic_struct_elided.stderr b/embassy-executor/tests/ui/nonstatic_struct_elided.stderr index 099ef8b4e..0ee1bfe0c 100644 --- a/embassy-executor/tests/ui/nonstatic_struct_elided.stderr +++ b/embassy-executor/tests/ui/nonstatic_struct_elided.stderr @@ -8,3 +8,17 @@ help: indicate the anonymous lifetime | 6 | async fn task(_x: Foo<'_>) {} | ++++ + +error[E0700]: hidden type for `impl Sized` captures lifetime that does not appear in bounds + --> tests/ui/nonstatic_struct_elided.rs:5:1 + | +5 | #[embassy_executor::task] + | ^^^^^^^^^^^^^^^^^^^^^^^^^ opaque type defined here +6 | async fn task(_x: Foo) {} + | --- hidden type `impl Sized` captures the anonymous lifetime defined here + | + = note: this error originates in the attribute macro `embassy_executor::task` (in Nightly builds, run with -Z macro-backtrace for more info) +help: add a `use<...>` bound to explicitly capture `'_` + | +5 | #[embassy_executor::task] + use<'_> + | +++++++++ -- cgit From 539ff78ebbdedbb75d0faf940e3ee69f5e7f276a Mon Sep 17 00:00:00 2001 From: Brezak Date: Wed, 23 Jul 2025 19:51:31 +0200 Subject: embassy-executor: explicitly return impl Future in task inner task --- embassy-executor-macros/src/macros/task.rs | 19 ++++++++++++++++--- embassy-executor/src/lib.rs | 8 ++++---- embassy-executor/tests/test.rs | 11 ++++++++++- .../tests/ui/bad_return_impl_future_nightly.stderr | 2 +- 4 files changed, 31 insertions(+), 9 deletions(-) diff --git a/embassy-executor-macros/src/macros/task.rs b/embassy-executor-macros/src/macros/task.rs index 5b360b128..fc8673743 100644 --- a/embassy-executor-macros/src/macros/task.rs +++ b/embassy-executor-macros/src/macros/task.rs @@ -5,7 +5,7 @@ use darling::FromMeta; use proc_macro2::{Span, TokenStream}; use quote::{format_ident, quote}; use syn::visit::{self, Visit}; -use syn::{Expr, ExprLit, Lit, LitInt, ReturnType, Type}; +use syn::{Expr, ExprLit, Lit, LitInt, ReturnType, Type, Visibility}; use crate::util::*; @@ -135,6 +135,13 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { let task_inner_future_output = match &f.sig.output { ReturnType::Default => quote! {-> impl ::core::future::Future}, // Special case the never type since we can't stuff it into a `impl Future` + ReturnType::Type(arrow, maybe_never) + if f.sig.asyncness.is_some() && matches!(**maybe_never, Type::Never(_)) => + { + quote! { + #arrow impl ::core::future::Future + } + } ReturnType::Type(arrow, maybe_never) if matches!(**maybe_never, Type::Never(_)) => quote! { #arrow #maybe_never }, @@ -149,14 +156,20 @@ pub fn run(args: TokenStream, item: TokenStream) -> TokenStream { }, }; + // We have to rename the function since it might be recursive; + let mut task_inner_function = f.clone(); + let task_inner_function_ident = format_ident!("__{}_task_inner_function", task_ident); + task_inner_function.sig.ident = task_inner_function_ident.clone(); + task_inner_function.vis = Visibility::Inherited; + let task_inner_body = if errors.is_empty() { quote! { - #f + #task_inner_function // SAFETY: All the preconditions to `#task_ident` apply to // all contexts `#task_inner_ident` is called in #unsafety { - #task_ident(#(#full_args,)*) + #task_inner_function_ident(#(#full_args,)*) } } } else { diff --git a/embassy-executor/src/lib.rs b/embassy-executor/src/lib.rs index e174a0594..0747db032 100644 --- a/embassy-executor/src/lib.rs +++ b/embassy-executor/src/lib.rs @@ -216,7 +216,7 @@ pub mod _export { ); #[allow(dead_code)] - trait HasOutput { + pub trait HasOutput { type Output; } @@ -225,7 +225,7 @@ pub mod _export { } #[allow(dead_code)] - type Never = ! as HasOutput>::Output; + pub type Never = ! as HasOutput>::Output; } /// Implementation details for embassy macros. @@ -242,7 +242,7 @@ pub mod _export { impl TaskReturnValue for Never {} #[allow(dead_code)] - trait HasOutput { + pub trait HasOutput { type Output; } @@ -251,5 +251,5 @@ pub mod _export { } #[allow(dead_code)] - type Never = ! as HasOutput>::Output; + pub type Never = ! as HasOutput>::Output; } diff --git a/embassy-executor/tests/test.rs b/embassy-executor/tests/test.rs index c1e7ec5d7..b84d3785a 100644 --- a/embassy-executor/tests/test.rs +++ b/embassy-executor/tests/test.rs @@ -7,7 +7,7 @@ use std::sync::{Arc, Mutex}; use std::task::Poll; use embassy_executor::raw::Executor; -use embassy_executor::task; +use embassy_executor::{task, Spawner}; #[export_name = "__pender"] fn __pender(context: *mut ()) { @@ -317,3 +317,12 @@ fn executor_task_cfg_args() { let (_, _, _) = (a, b, c); } } + +#[test] +fn recursive_task() { + #[embassy_executor::task(pool_size = 2)] + async fn task1() { + let spawner = unsafe { Spawner::for_current_executor().await }; + spawner.spawn(task1()); + } +} diff --git a/embassy-executor/tests/ui/bad_return_impl_future_nightly.stderr b/embassy-executor/tests/ui/bad_return_impl_future_nightly.stderr index 73ceb989d..3c3c9503b 100644 --- a/embassy-executor/tests/ui/bad_return_impl_future_nightly.stderr +++ b/embassy-executor/tests/ui/bad_return_impl_future_nightly.stderr @@ -7,4 +7,4 @@ error[E0277]: task futures must resolve to `()` or `!` = note: use `async fn` or change the return type to `impl Future` = help: the following other types implement trait `TaskReturnValue`: () - ! as _export::HasOutput>::Output + ! as HasOutput>::Output -- cgit From 54d9a7fed3ab211b1049aae0af0bc49f912c9df4 Mon Sep 17 00:00:00 2001 From: Brezak Date: Wed, 23 Jul 2025 21:17:12 +0200 Subject: embassy-executor: add macro ui test for unsafe ops in unsafe tasks Check if the #[task] macro properly handles unsafe functions so the `unsafe_op_in_unsafe_fn` lint still works --- embassy-executor/tests/ui.rs | 2 ++ embassy-executor/tests/ui/unsafe_op_in_unsafe_task.rs | 10 ++++++++++ .../tests/ui/unsafe_op_in_unsafe_task.stderr | 18 ++++++++++++++++++ 3 files changed, 30 insertions(+) create mode 100644 embassy-executor/tests/ui/unsafe_op_in_unsafe_task.rs create mode 100644 embassy-executor/tests/ui/unsafe_op_in_unsafe_task.stderr diff --git a/embassy-executor/tests/ui.rs b/embassy-executor/tests/ui.rs index 8b83cd368..5486a0624 100644 --- a/embassy-executor/tests/ui.rs +++ b/embassy-executor/tests/ui.rs @@ -32,5 +32,7 @@ fn ui() { t.compile_fail("tests/ui/self.rs"); t.compile_fail("tests/ui/type_error.rs"); t.compile_fail("tests/ui/where_clause.rs"); + t.compile_fail("tests/ui/unsafe_op_in_unsafe_task.rs"); + t.pass("tests/ui/task_safety_attribute.rs"); } diff --git a/embassy-executor/tests/ui/unsafe_op_in_unsafe_task.rs b/embassy-executor/tests/ui/unsafe_op_in_unsafe_task.rs new file mode 100644 index 000000000..ee7924838 --- /dev/null +++ b/embassy-executor/tests/ui/unsafe_op_in_unsafe_task.rs @@ -0,0 +1,10 @@ +#![cfg_attr(feature = "nightly", feature(impl_trait_in_assoc_type))] +#![deny(unsafe_op_in_unsafe_fn)] + +#[embassy_executor::task] +async unsafe fn task() { + let x = 5; + (&x as *const i32).read(); +} + +fn main() {} diff --git a/embassy-executor/tests/ui/unsafe_op_in_unsafe_task.stderr b/embassy-executor/tests/ui/unsafe_op_in_unsafe_task.stderr new file mode 100644 index 000000000..d987a4b95 --- /dev/null +++ b/embassy-executor/tests/ui/unsafe_op_in_unsafe_task.stderr @@ -0,0 +1,18 @@ +error[E0133]: call to unsafe function `std::ptr::const_ptr::::read` is unsafe and requires unsafe block + --> tests/ui/unsafe_op_in_unsafe_task.rs:7:5 + | +7 | (&x as *const i32).read(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function + | + = note: for more information, see + = note: consult the function's documentation for information on how to avoid undefined behavior +note: an unsafe function restricts its caller, but its body is safe by default + --> tests/ui/unsafe_op_in_unsafe_task.rs:5:1 + | +5 | async unsafe fn task() { + | ^^^^^^^^^^^^^^^^^^^^^^ +note: the lint level is defined here + --> tests/ui/unsafe_op_in_unsafe_task.rs:2:9 + | +2 | #![deny(unsafe_op_in_unsafe_fn)] + | ^^^^^^^^^^^^^^^^^^^^^^ -- cgit