Skip to content

Commit

Permalink
Limit attributes copied into generated code (#1429)
Browse files Browse the repository at this point in the history
### What
Limit the attributes that get copied into code generated to:
- `doc` - rust docs
- `cfg` - used to control whether functions are included or not
depending on features
- `allow`/`deny` - used to ignore or require compiler warnings

### Why
The copying of attributes into generated code is from early code
developed for the SDK where we were trying to deal with functions that
weren't named consistently with Rust's naming scheme. We wanted to carry
through to the generated code `allow` statements so that the generated
code didn't generate warnings that were ignored at the source level.

However, this has introduced a problem. For any dev who is introducing
macros into their contracts and where those macros appear as attributes
on contract functions, the macros are then being duplicated into the
generated code and injected onto the contract client functions, and
modules.

Docs attributes are retained because it's valuable if docs show up when
hovering over generated contract client functions. The value there is
that when someone hovers over a call to a contract client function in an
IDE, it'll display the docs for that function.

Cfg attributes are retained because folks might put a contract function
behind a feature, or behind the test flag, so that the function is only
present when that feature is enabled, or tests are enabled.

Allow and deny are added because folks might be allowing or denying
non-std function names and it'd benefit to pass those allows down to the
client at least.

Close #1426

### Known Limitations

A limitation with this change, and a risk, is that it is a breaking
change that is very difficult to pick up on. Someone may be attaching an
attribute to their contract function and expecting it to carry over to
the client. After this change is merged unless there is an error
compiling or an easily observable change in behaviour, the developer may
not know that the attribute no longer applies to the client. I think the
risk is lowered because it's unexpected and not normal that the
attributes are copied over, so unlikely someone knows they could depend
on it, and it's undocumented and not a difficult "feature" to discover.
Any use of this kind is likely to be accidental.

We could keep this change for the v23 major version to reduce risk,
although this change is actually a bug fix blocking development, so I
think we should treat it as a bug fix and merge it into a v22 patch
release.
  • Loading branch information
leighmcculloch authored Jan 24, 2025
1 parent 3ca5988 commit 29905dd
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 3 deletions.
10 changes: 10 additions & 0 deletions soroban-sdk-macros/src/attribute.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
use syn::Attribute;

/// Returns true if the attribute is an attribute that should be preserved and
/// passed through to code generated for the item the attribute is on.
pub fn pass_through_attr_to_gen_code(attr: &Attribute) -> bool {
attr.path().is_ident("doc")
|| attr.path().is_ident("cfg")
|| attr.path().is_ident("allow")
|| attr.path().is_ident("deny")
}
8 changes: 6 additions & 2 deletions soroban-sdk-macros/src/derive_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use proc_macro2::TokenStream;
use quote::{format_ident, quote};
use syn::{Error, FnArg, LitStr, Path, Type, TypePath, TypeReference};

use crate::{symbol, syn_ext};
use crate::{attribute::pass_through_attr_to_gen_code, symbol, syn_ext};

pub fn derive_client_type(crate_path: &Path, ty: &str, name: &str) -> TokenStream {
let ty_str = quote!(#ty).to_string();
Expand Down Expand Up @@ -197,7 +197,11 @@ pub fn derive_client_impl(crate_path: &Path, name: &str, fns: &[syn_ext::Fn]) ->
.unzip();
let fn_output = f.output();
let fn_try_output = f.try_output(crate_path);
let fn_attrs = f.attrs;
let fn_attrs = f
.attrs
.iter()
.filter(|attr| pass_through_attr_to_gen_code(attr))
.collect::<Vec<_>>();
if cfg!(not(feature = "testutils")) {
quote! {
#(#fn_attrs)*
Expand Down
8 changes: 7 additions & 1 deletion soroban-sdk-macros/src/derive_fn.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::map_type::map_type;
use crate::{attribute::pass_through_attr_to_gen_code, map_type::map_type};
use itertools::MultiUnzip;
use proc_macro2::TokenStream as TokenStream2;
use quote::{format_ident, quote};
Expand Down Expand Up @@ -144,6 +144,12 @@ pub fn derive_pub_fn(
None
};

// Filter attributes to those that should be passed through to the generated code.
let attrs = attrs
.iter()
.filter(|attr| pass_through_attr_to_gen_code(attr))
.collect::<Vec<_>>();

// Generated code.
Ok(quote! {
#[doc(hidden)]
Expand Down
7 changes: 7 additions & 0 deletions soroban-sdk-macros/src/derive_spec_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use syn::{
ReturnType, Type, TypePath,
};

use crate::attribute::pass_through_attr_to_gen_code;
use crate::{doc::docs_from_attrs, map_type::map_type, DEFAULT_XDR_RW_LIMITS};

#[allow(clippy::too_many_arguments)]
Expand Down Expand Up @@ -162,6 +163,12 @@ pub fn derive_fn_spec(
None
};

// Filter attributes to those that should be passed through to the generated code.
let attrs = attrs
.iter()
.filter(|attr| pass_through_attr_to_gen_code(attr))
.collect::<Vec<_>>();

// Generated code.
Ok(quote! {
#[doc(hidden)]
Expand Down
1 change: 1 addition & 0 deletions soroban-sdk-macros/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
extern crate proc_macro;

mod arbitrary;
mod attribute;
mod derive_args;
mod derive_client;
mod derive_enum;
Expand Down

0 comments on commit 29905dd

Please sign in to comment.