Copy trait docs to overridden functions without docs#1760
Copy trait docs to overridden functions without docs#1760leighmcculloch wants to merge 7 commits intomainfrom
Conversation
When using #[contractimpl(contracttrait)], overridden trait functions that lack their own doc comments now inherit docs from the trait function definition. This ensures contract specs are fully documented when using well-documented traits. Closes #1653
There was a problem hiding this comment.
Pull request overview
Improves Soroban contracttrait ergonomics by ensuring contract spec docs for overridden trait functions fall back to the trait’s docs when the override has no doc comments, aligning overridden behavior with existing default-function doc propagation.
Changes:
- Move spec generation for
#[contractimpl(contracttrait)]overridden functions into the contracttrait macro bridge so trait docs are available as a fallback source. - Serialize trait and impl function signatures with doc attributes to enable “inherit docs if missing” behavior during spec generation.
- Add regression tests asserting doc inheritance behavior and update test crate dev-dependencies accordingly.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
soroban-sdk-macros/src/lib.rs |
Skips #[contractspecfn] when contracttrait is enabled so spec generation can be handled in the macro bridge (where trait docs are available). |
soroban-sdk-macros/src/derive_contractimpl_trait_macro.rs |
Passes serialized trait fn docs+sigs (all fns) and impl fn docs+sigs into the macro bridge. |
soroban-sdk-macros/src/derive_contractimpl_trait_default_fns_not_overridden.rs |
Generates spec entries for impl-provided (overridden/required) functions, inheriting trait docs when impl docs are absent. |
tests/contracttrait_impl_partial/src/lib.rs |
Adds a test covering doc inheritance for overridden functions and a local-docs override case. |
tests/contracttrait_impl_partial/Cargo.toml |
Adds stellar-xdr dev-dependency needed for spec decoding in tests. |
tests-expanded/* |
Updated macro-expansion fixtures reflecting new spec generation and doc embedding. |
Cargo.lock |
Locks new test dev-dependency usage. |
| // When contracttrait is true, spec generation for overridden | ||
| // functions is handled by the macro bridge (so that trait docs can | ||
| // be used as fallback), so contractspecfn is not applied here. | ||
| let mut output = if args.contracttrait { | ||
| quote! { | ||
| #[#crate_path::contractargs(name = #args_ident, impl_only = true)] | ||
| #[#crate_path::contractclient(crate_path = #crate_path_str, name = #client_ident, impl_only = true)] | ||
| #imp |
There was a problem hiding this comment.
When contracttrait is set but the impl block is not actually implementing a trait (imp.trait_ is None), this new branch skips #[contractspecfn(...)] and also won’t invoke the contracttrait macro bridge, resulting in no spec being generated for the contract functions. Consider adding a validation that contracttrait requires imp.trait_.is_some() (emit a compile_error!/darling error), or fall back to applying contractspecfn when there is no trait impl.
There was a problem hiding this comment.
I think this is correct. We should emit a compiler error if the users does #[contractimpl(contracttrait)] without implementing a trait.
| // When contracttrait is true, spec generation for overridden | ||
| // functions is handled by the macro bridge (so that trait docs can | ||
| // be used as fallback), so contractspecfn is not applied here. | ||
| let mut output = if args.contracttrait { | ||
| quote! { | ||
| #[#crate_path::contractargs(name = #args_ident, impl_only = true)] | ||
| #[#crate_path::contractclient(crate_path = #crate_path_str, name = #client_ident, impl_only = true)] | ||
| #imp |
There was a problem hiding this comment.
I think this is correct. We should emit a compiler error if the users does #[contractimpl(contracttrait)] without implementing a trait.
| let impl_fn_strs: Vec<String> = pub_methods | ||
| .iter() | ||
| .map(|f| { | ||
| let doc_attrs: Vec<_> = f.attrs.iter().filter(|a| is_attr_doc(a)).collect(); |
There was a problem hiding this comment.
I believe this adds an edge case where a trait override functions can't be cfg gated:
Consider:
#[contract]
pub struct Contract;
#[contracttrait]
pub trait Trait {
fn some_fn() -> u32 {
7
}
}
#[contractimpl(contracttrait)]
impl Trait for Contract {
#[cfg(some_condition)]
fn some_fn() -> u32 {
9
}
}You would expect that this compiles, and either overrides the default trait function with the cfg condition is true, or uses the trait default if false.
I don't think this strictly is a blocker. Trait override functions have some existing edge cases with cfg flags that need to get worked out, so am OK pushing this to a separate issue.
| // When contracttrait is true, spec generation for overridden | ||
| // functions is handled by the macro bridge (so that trait docs can | ||
| // be used as fallback), so contractspecfn is not applied here. |
There was a problem hiding this comment.
I think this breaks associated type resolution for overridden functions, as we skip the code path that does flatten_associated_items_in_impl_fns.
Consider:
#[contract]
pub struct Contract;
#[contracttrait]
pub trait Trait {
type Value;
fn echo_value(value: u64) -> u64 {
value
}
}
#[contractimpl(contracttrait)]
impl Trait for Contract {
type Value = u64;
fn echo_value(value: Self::Value) -> Self::Value {
value + 1
}
}The resulting interface is:
#[soroban_sdk::contractargs(name = "Args")]
#[soroban_sdk::contractclient(name = "Client")]
pub trait Contract {
fn echo_value(env: soroban_sdk::Env, value: Value) -> Value;
}
What
When
#[contractimpl(contracttrait)]is used, overridden trait functions that lack their own doc comments now inherit docs from the trait function definition. Spec generation for overridden functions is moved fromcontractspecfnto the macro bridge, where trait docs are available for fallback. If an overridden function has its own doc comments, those are used (no change in behaviour).Example
Before:
transferin the contract spec has empty docs.After:
transferin the contract spec has "Transfers tokens between accounts."Why
When implementing a trait with
#[contractimpl(contracttrait)], doc strings from trait functions were only copied to the contract spec for default (non-overridden) functions. If a contract overrode a trait function without adding its own documentation, the function had empty docs in the generated contract spec, even when the trait had comprehensive documentation available. This forced developers to manually duplicate trait docs on every override.Close #1653
Target
v26