Skip to content

Copy trait docs to overridden functions without docs #1760

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.

Sign up for GitHub

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

Open
leighmcculloch wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Copy trait docs to overridden functions without docs #1760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f7c4477
Copy trait function docs to overridden functions that lack their own
leighmcculloch Mar 12, 2026
5b48409
reformat derive_fns_spec call for readability
leighmcculloch Mar 12, 2026
b6ecbea
Merge branch 'main' into trait-docs
leighmcculloch Mar 23, 2026
aecbe73
Merge branch 'main' into trait-docs
leighmcculloch Apr 1, 2026
724e6c5
Merge branch 'main' into trait-docs
leighmcculloch Apr 1, 2026
ec29c6c
update expanded test snapshots for trait docs
leighmcculloch Apr 1, 2026
e40ea24
Merge branch 'main' into trait-docs
mootz12 Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

37 changes: 36 additions & 1 deletion soroban-sdk-macros/src/derive_contractimpl_trait_default_fns_not_overridden.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use crate::{
attribute::is_attr_doc,
default_crate_path,
derive_args::derive_args_impl,
derive_client::derive_client_impl,
Expand All @@ -9,7 +10,7 @@ use crate::{
use darling::{ast::NestedMeta, Error, FromMeta};
use proc_macro2::{Ident, TokenStream as TokenStream2};
use quote::quote;
use std::collections::HashSet;
use std::collections::{HashMap, HashSet};
use syn::{LitStr, Path, Type};

// See soroban-sdk/docs/contracttrait.md for documentation on how this works.
Expand All @@ -20,8 +21,10 @@ struct Args {
crate_path: Path,
trait_ident: Path,
trait_default_fns: Vec<LitStr>,
trait_all_fns: Vec<LitStr>,
impl_ident: Ident,
impl_fns: Vec<LitStr>,
impl_fns_with_docs: Vec<LitStr>,
client_name: String,
args_name: String,
spec_name: Type,
Expand Down Expand Up @@ -84,5 +87,37 @@ fn derive(args: &Args) -> Result<TokenStream2, Error> {
fns.iter().map(|f| &f.ident),
));

// Generate spec for overridden functions, inheriting trait docs for
// functions that lack their own doc comments.
let trait_all_fns = syn_ext::strs_to_fns(&args.trait_all_fns)?;
let trait_fn_docs: HashMap<String, Vec<syn::Attribute>> = trait_all_fns
.into_iter()
.map(|f| (f.ident.to_string(), f.attrs))
.collect();

let impl_fns_parsed = syn_ext::strs_to_fns(&args.impl_fns_with_docs)?;
let impl_fns_for_spec: Vec<syn_ext::Fn> = impl_fns_parsed
.into_iter()
.map(|f| {
let has_docs = f.attrs.iter().any(|a| is_attr_doc(a));
if !has_docs {
if let Some(trait_attrs) = trait_fn_docs.get(&f.ident.to_string()) {
return syn_ext::Fn {
ident: f.ident,
attrs: trait_attrs.clone(),
inputs: f.inputs,
output: f.output,
};
}
}
f
})
.collect();
output.extend(derive_fns_spec(
&args.spec_name,
&impl_fns_for_spec,
spec_export,
));

Ok(output)
}
25 changes: 25 additions & 0 deletions soroban-sdk-macros/src/derive_contractimpl_trait_macro.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ fn derive(args: &Args, input: &ItemTrait) -> Result<TokenStream2, Error> {
_ => None,
});

// Serialize all trait functions' docs+sigs (including non-default ones) so
// that overridden functions can inherit trait docs when they lack their own.
let all_fns = input.items.iter().filter_map(|i| match i {
TraitItem::Fn(TraitItemFn { sig, attrs, .. }) => {
let doc_attrs: Vec<_> = attrs.iter().filter(|a| is_attr_doc(a)).collect();
Some(quote!(#(#doc_attrs)* #sig).to_token_stream().to_string())
}
_ => None,
});

let macro_ident = macro_ident(&input.ident);

let output = quote! {
Expand All @@ -67,15 +77,18 @@ fn derive(args: &Args, input: &ItemTrait) -> Result<TokenStream2, Error> {
$trait_ident:path,
$impl_ident:ty,
$impl_fns:expr,
$impl_fns_with_docs:expr,
$client_name:literal,
$args_name:literal,
$spec_name:literal $(,)?
) => {
#path::contractimpl_trait_default_fns_not_overridden!(
trait_ident = $trait_ident,
trait_default_fns = [#(#fns),*],
trait_all_fns = [#(#all_fns),*],
impl_ident = $impl_ident,
impl_fns = $impl_fns,
impl_fns_with_docs = $impl_fns_with_docs,
client_name = $client_name,
args_name = $args_name,
spec_name = $spec_name,
Expand All @@ -99,11 +112,23 @@ pub fn generate_call_to_contractimpl_for_trait(
spec_ident: &str,
) -> TokenStream2 {
let impl_fn_idents = pub_methods.iter().map(|f| f.sig.ident.to_string());
// Serialize impl functions with their doc attrs so the macro bridge can
// generate spec entries, falling back to trait docs when the impl function
// lacks its own.
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();
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 Apr 13, 2026

Choose a reason for hiding this comment

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

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.

let sig = &f.sig;
quote!(#(#doc_attrs)* #sig).to_token_stream().to_string()
})
.collect();
quote! {
#trait_ident!(
#trait_ident,
#impl_ident,
[#(#impl_fn_idents),*],
[#(#impl_fn_strs),*],
#client_ident,
#args_ident,
#spec_ident,
Expand Down
24 changes: 18 additions & 6 deletions soroban-sdk-macros/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,12 +269,24 @@ pub fn contractimpl(metadata: TokenStream, input: TokenStream) -> TokenStream {

match derived {
Ok(derived_ok) => {
let mut output = quote! {
#[#crate_path::contractargs(name = #args_ident, impl_only = true)]
#[#crate_path::contractclient(crate_path = #crate_path_str, name = #client_ident, impl_only = true)]
#[#crate_path::contractspecfn(name = #ty_str)]
#imp
#derived_ok
// 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.
Comment on lines +272 to +274
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 Apr 13, 2026

Choose a reason for hiding this comment

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

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;
}

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
Comment on lines +272 to +279
Copy link

Copilot AI Apr 1, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 Apr 13, 2026

Choose a reason for hiding this comment

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

I think this is correct. We should emit a compiler error if the users does #[contractimpl(contracttrait)] without implementing a trait.

#derived_ok
}
} else {
quote! {
#[#crate_path::contractargs(name = #args_ident, impl_only = true)]
#[#crate_path::contractclient(crate_path = #crate_path_str, name = #client_ident, impl_only = true)]
#[#crate_path::contractspecfn(name = #ty_str)]
#imp
#derived_ok
}
};

// See soroban-sdk/docs/contracttrait.md for documentation on how
Expand Down
Loading
Loading