Repository navigation
Conversation
… bodies An `async fn` under `#[tool]` or `#[prompt]` is rewritten into `-> Pin<Box<dyn Future<Output = T> + Send + '_>>`, because the router keeps every handler as a `Send` boxed future, so the whole call tree has to pass the `Send` check. A closure that takes a reference and returns an `async move` block, buffered by a `futures` combinator, fails that check with "implementation of `FnOnce` is not general enough", and the span points at the attribute macro. The trigger is the `Send` bound, not the codegen: the same call tree under a bare `F: Future + Send` bound, with no macro, no `Box::pin` and no trait object, produces the identical error. That is rustc issues 104382 and 126551 in rust-lang/rust, both open. Document the limitation and the ways round it on `#[tool]`, cross-reference it from `#[prompt]`, which shares the codegen and the same failure, and add regression tests so the documented workarounds keep compiling. Refs #1092 Co-authored-by: Igor Kasianenko <igor.kasianenko@outlook.com>
… a time `.then(..)` compiles with the original inline closure too, so the `async fn` helper isn't what fixes the error. It does run the futures one after another, unlike `buffer_unordered(n)`, so the bullet now says when it's a fair trade. Refs #1092
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Contributor
|
@jamadeo is on triage rotation this week and is the reviewer for this one. Labels come from the PR labeler. Please review the change. Automated triage. Labels are a first pass and the assigned maintainer has the final say. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1092. The docs and tests are @IgorKasianenko's work (IgorKasianenko/rust-sdk@a408ab9), carried over with them as co-author.
Motivation and Context
#[tool]and#[prompt]turn anasync fninto aSendboxed future. So a closure that borrows its item and returns anasync moveblock, fed into.buffer_unordered(..)or.buffered(..), fails with "implementation ofFnOnceis not general enough", and the error points at the macro. Igor showed in the issue that the same code fails under anySendbound with no macro involved, so this documents the limitation rather than changing the codegen.#[tool]docs now describe the limitation, include the error text so it's searchable, and list the ways round it:.boxed(), owning the items,.then(..)if the futures don't need to run at the same time, or thelocalfeature.#[prompt]docs point to the same section, since it shares the codegen.Two changes from Igor's commit (the clippy one is folded into the first commit):
.then(..)bullet used to say to move the closure body into anasync fnhelper..then(..)compiles with the inline closure too, so the helper isn't the fix. It does run the futures one at a time, so the bullet now says when that's a fair trade.stream::iter(items.into_iter())is nowstream::iter(items)(clippyuseless_conversion).How Has This Been Tested?
test_async_tool_with_buffered_stream(test_tool_macros.rs): one tool fans out over borrowed items with.boxed(), another over owned items. Removing the.boxed()brings back the exact error, so the test pins the workaround.test_async_prompt_with_buffered_stream(test_prompt_macros.rs): the same.boxed()shape under#[prompt]..then(..)claim inside a real#[tool]on rustc 1.96 and 1.94: it compiles with both the helper and the inline closure, and a counting test showed at most 1 future in flight with.then(..)vs 4 with.buffer_unordered(4).Validation:
cargo test -p rmcp --features 'server client macros schemars' --test test_tool_macros --test test_prompt_macros: passed (16 and 8).cargo test -p rmcp --all-features: passed.cargo clippy --all-targets --all-features -- -D warnings, clippy on the two test files withoutlocal(--features 'server client macros schemars'),RUSTDOCFLAGS=-Dwarnings cargo doc -p rmcp-macros --no-depsandcargo +nightly fmt --all -- --check: clean.Breaking Changes
None.
Types of changes
Checklist
Additional context
Both test files start with
#![cfg(not(feature = "local"))], so only the "Run Tests (no local feature)" job runs them.--all-featuresturns onlocal.