From 29905dd952bff4314b27005e7f4d0f267b619e38 Mon Sep 17 00:00:00 2001 From: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com> Date: Thu, 23 Jan 2025 20:06:19 -0800 Subject: [PATCH] Limit attributes copied into generated code (#1429) ### 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. --- soroban-sdk-macros/src/attribute.rs | 10 ++++++++++ soroban-sdk-macros/src/derive_client.rs | 8 ++++++-- soroban-sdk-macros/src/derive_fn.rs | 8 +++++++- soroban-sdk-macros/src/derive_spec_fn.rs | 7 +++++++ soroban-sdk-macros/src/lib.rs | 1 + 5 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 soroban-sdk-macros/src/attribute.rs diff --git a/soroban-sdk-macros/src/attribute.rs b/soroban-sdk-macros/src/attribute.rs new file mode 100644 index 000000000..5f58473d1 --- /dev/null +++ b/soroban-sdk-macros/src/attribute.rs @@ -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") +} diff --git a/soroban-sdk-macros/src/derive_client.rs b/soroban-sdk-macros/src/derive_client.rs index aa896c633..bc9770a9f 100644 --- a/soroban-sdk-macros/src/derive_client.rs +++ b/soroban-sdk-macros/src/derive_client.rs @@ -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(); @@ -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::>(); if cfg!(not(feature = "testutils")) { quote! { #(#fn_attrs)* diff --git a/soroban-sdk-macros/src/derive_fn.rs b/soroban-sdk-macros/src/derive_fn.rs index 0c24bfe72..bc0858b3f 100644 --- a/soroban-sdk-macros/src/derive_fn.rs +++ b/soroban-sdk-macros/src/derive_fn.rs @@ -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}; @@ -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::>(); + // Generated code. Ok(quote! { #[doc(hidden)] diff --git a/soroban-sdk-macros/src/derive_spec_fn.rs b/soroban-sdk-macros/src/derive_spec_fn.rs index 2c0afe999..ecb9d7fc3 100644 --- a/soroban-sdk-macros/src/derive_spec_fn.rs +++ b/soroban-sdk-macros/src/derive_spec_fn.rs @@ -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)] @@ -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::>(); + // Generated code. Ok(quote! { #[doc(hidden)] diff --git a/soroban-sdk-macros/src/lib.rs b/soroban-sdk-macros/src/lib.rs index aae0303ed..e3f8c8ffb 100644 --- a/soroban-sdk-macros/src/lib.rs +++ b/soroban-sdk-macros/src/lib.rs @@ -1,6 +1,7 @@ extern crate proc_macro; mod arbitrary; +mod attribute; mod derive_args; mod derive_client; mod derive_enum;