Skip to content

Preserve cfg attrs for contracttrait default functions#1869

Open
mootz12 wants to merge 2 commits into
mainfrom
support_contracttrait_cfg
Open

Preserve cfg attrs for contracttrait default functions#1869
mootz12 wants to merge 2 commits into
mainfrom
support_contracttrait_cfg

Conversation

@mootz12
Copy link
Copy Markdown
Contributor

@mootz12 mootz12 commented May 12, 2026

What

Preserve pass-through function attributes, including #[cfg(...)], when #[contracttrait] records default trait functions for later #[contractimpl(contracttrait)] code generation.

This also makes generated args and native test registration code cfg-aware, and adds regression coverage for cfg-disabled defaults, cfg-enabled defaults, cfg-gated overrides, and regular #[contractimpl] cfg-gated functions.

Why

#[contracttrait] previously serialized only doc attributes for default trait methods. If a default method was cfg-gated, the generated trait helper lost that cfg and later generated wrappers for a method that Rust had compiled out of the trait.

Cfg-gated overrides also need special handling: an override should suppress the trait default only when the override's cfg is active. Otherwise, the default implementation should remain available.

Native test registration needed the same treatment because registration ctors referenced generated raw wrapper functions. If a wrapper was cfg-disabled but its registration was not, the generated test code could reference a missing symbol.

Fixes #1868

Known limitations

cfg_attr is not interpreted for function gating. Use direct #[cfg(...)] on #[contracttrait] default functions and #[contractimpl] functions.

For cross-crate #[contracttrait] default functions, function-level cfgs are evaluated in the implementing contract crate's configuration context when generating wrappers. Shared trait crates should prefer cfg-gating the whole trait item, or require implementing contracts to define matching features for function-level cfgs.

Copilot AI review requested due to automatic review settings May 12, 2026 20:41
@claude
Copy link
Copy Markdown

claude Bot commented May 12, 2026

Claude encountered an error —— View job


PR Review In Progress

  • Read CLAUDE.md and understand repo conventions
  • Review macro changes in soroban-sdk-macros/src/
  • Review test additions in tests/contracttrait_*
  • Sanity-check expanded test output changes
  • Check for bugs, security, and performance issues
  • Post final review

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes #[contracttrait] / #[contractimpl(contracttrait)] codegen when default trait methods (and overrides) are #[cfg(...)]-gated by preserving pass-through function attributes during macro serialization and by making downstream wrapper/args/test registration generation cfg-aware. It also adds regression coverage for cfg-disabled defaults, cfg-enabled defaults, cfg-gated overrides, and cfg-gated #[contractimpl] functions, including a cross-crate case (issue #1868).

Changes:

  • Preserve pass-through function attributes (notably #[cfg(...)]) when recording default #[contracttrait] methods and when encoding impl override info for later comparison.
  • Make generated args helpers and native test registration constructors respect cfg gating (avoid referencing cfg-disabled wrapper symbols).
  • Add new contract tests + snapshots exercising cfg-disabled defaults, cfg-enabled defaults, cfg-gated overrides, and cross-crate behavior; update expanded-code fixtures accordingly.

Reviewed changes

Copilot reviewed 59 out of 60 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/contracttrait_trait/test_snapshots/test/test_cfg_gated_disabled_override_uses_default_fn.1.json New snapshot covering cfg-disabled override falling back to default.
tests/contracttrait_trait/test_snapshots/test/test_cfg_gated_default_with_unconditional_override.1.json New snapshot covering unconditional override behavior with cfg-gated defaults.
tests/contracttrait_trait/test_snapshots/test/test_cfg_gated_contracttrait_default_fn.1.json New snapshot for cfg-gated #[contracttrait] default fn behavior.
tests/contracttrait_trait/test_snapshots/test/test_cfg_gated_contractimpl_fn.1.json New snapshot for cfg-gated #[contractimpl] fn behavior.
tests/contracttrait_trait/src/lib.rs Adds cfg-gated contracttrait + multiple impl/test scenarios.
tests/contracttrait_trait/Cargo.toml Adds cfg-gated-fn feature (enabled by default for this test crate).
tests/contracttrait_impl_full/test_snapshots/test/test_cross_crate_cfg_gated_default_fn.1.json New snapshot for cross-crate cfg-gated default fn case.
tests/contracttrait_impl_full/src/lib.rs Imports and uses new CfgGated trait for cross-crate test.
tests/contracttrait_impl_full/Cargo.toml Adds cfg-gated-fn feature for cross-crate test configuration.
tests-expanded/test_workspace_contract_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_udt_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_tuples_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_spec_shaking_v2_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_spec_shaking_v1_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_mutability_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_multiimpl_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_modular_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_macros_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_logging_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_invoke_contract_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_import_contract_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_generics_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_fuzz_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_events_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_events_ref_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_errors_wasm32v1-none.rs Expanded fixture updates reflecting cfg-aware arg generation changes.
tests-expanded/test_errors_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_empty2_tests.rs Expanded fixture updates reflecting removal of empty/no-op registration ctor.
tests-expanded/test_empty_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_contracttrait_trait_wasm32v1-none.rs Expanded fixture updates showing new CfgGated trait/client/args/spec generation.
tests-expanded/test_contracttrait_path_super_tests.rs Expanded fixture updates reflecting removal of empty/no-op registration ctor.
tests-expanded/test_contracttrait_path_self_tests.rs Expanded fixture updates reflecting removal of empty/no-op registration ctor.
tests-expanded/test_contracttrait_path_relative_tests.rs Expanded fixture updates reflecting removal of empty/no-op registration ctor.
tests-expanded/test_contracttrait_path_global_wasm32v1-none.rs Expanded fixture updates reflecting args doc/attr pass-through changes.
tests-expanded/test_contracttrait_path_global_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_contracttrait_path_crate_tests.rs Expanded fixture updates reflecting removal of empty/no-op registration ctor.
tests-expanded/test_contracttrait_impl_partial_wasm32v1-none.rs Expanded fixture updates reflecting args doc/attr pass-through changes.
tests-expanded/test_contracttrait_impl_partial_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_contracttrait_impl_full_wasm32v1-none.rs Expanded fixture updates reflecting new CfgGated usage in full impl case.
tests-expanded/test_contract_data_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_constructor_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_bn254_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_bls_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_auth_tests.rs Expanded fixture updates reflecting pass-through attrs applied to args/ctors.
tests-expanded/test_associated_types_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_associated_types_contracttrait_tests.rs Expanded fixture updates reflecting removal of empty/no-op registration ctor.
tests-expanded/test_associated_type_chained_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_add_u64_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_add_u128_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_add_i128_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
tests-expanded/test_account_wasm32v1-none.rs Expanded fixture updates reflecting pass-through attrs applied to args.
tests-expanded/test_account_tests.rs Expanded fixture updates due to ctor hashing/registration changes.
soroban-sdk/docs/contracttrait.md Documentation updates describing preserved cfg attrs and limitations (cfg_attr).
soroban-sdk-macros/src/lib.rs Updates to pass full fn metadata into registration ctor generation.
soroban-sdk-macros/src/derive_fn.rs Makes native registration ctor generation cfg-aware and per-function.
soroban-sdk-macros/src/derive_contractimpl_trait_macro.rs Serializes default trait fns including pass-through attrs; encodes impl fns with attrs for cfg-aware override matching.
soroban-sdk-macros/src/derive_contractimpl_trait_default_fns_not_overridden.rs Implements cfg-aware default-vs-override selection and cfg-gated default emission.
soroban-sdk-macros/src/derive_args.rs Applies pass-through attrs (incl. #[cfg]) to generated Args methods.

Comment thread soroban-sdk-macros/src/derive_fn.rs
@mootz12 mootz12 force-pushed the support_contracttrait_cfg branch from 98712ca to c9d922d Compare May 12, 2026 21:15
@claude
Copy link
Copy Markdown

claude Bot commented May 12, 2026

Claude encountered an error —— View job


PR Review

  • Read repo conventions and core files
  • Review macro changes in soroban-sdk-macros/src/
  • Review test additions in tests/contracttrait_*
  • Sanity-check expanded test output changes
  • Verify Copilot's flagged Sha256::digest+to_string() claim
  • Post final review

Summary

The change is well-scoped and the cfg-propagation logic is correct across the three places it needs to take effect (derive_args, per-fn registration ctors in derive_fn, and the trait default-vs-override selection in derive_contractimpl_trait_default_fns_not_overridden). The override matching across mutually-exclusive #[cfg] overrides is sound, the registration ctor switch from "one ctor per impl" to "one ctor per fn" is the right shape for per-fn cfg gating, and the documented cfg_attr/cross-crate limitations are accurate. New tests cover the four relevant scenarios plus a cross-crate case.

Pushback on the Copilot inline comment

Sha256::digest expects bytes (impl AsRef<[u8]>), but this passes a String

This is incorrect. impl AsRef<[u8]> for String is in the standard library (it forwards to String::as_bytes), so Sha256::digest(some_string) compiles. The regenerated tests-expanded/ snapshots in this PR are the practical proof — the macro is exercised and the new ctor identifier hashes show up in many fixtures (__Contract__CustomAccountInterface__1cb152b3…_ctor etc.), which would be impossible if the macro didn't compile. No change needed at soroban-sdk-macros/src/derive_fn.rs:269.

Verification of the override-vs-default selection

derive_contractimpl_trait_default_fns_not_overridden::derive (the filter_map at lines 59–85) is the load-bearing piece. Walking the cases:

trait default cfg impl override cfg result for default-fn emission
none absent default emitted (unchanged behavior)
none unconditional default dropped (correct: covered by override)
none cfg(X) default gains cfg(not(X)) (correct: fall back when override is off)
cfg(Y) cfg(X) default has cfg(Y) AND cfg(not(X)) — multiple #[cfg] AND together, so default is present only when Y holds and X does not
cfg(Y) unconditional default dropped; user is responsible for ensuring the override matches the trait method's existence (matches the documented limitation)
cfg(Y) multiple cfg-gated overrides cfg(A), cfg(B) default kept under not(A) AND not(B) (correct: kept only when no override is active)

The cfg_condition helper's [cfg] => Some(quote!{ #cfg }) vs _ => Some(quote!{ all(...) }) distinction is fine — multiple #[cfg(...)] on the same item already AND together, so wrapping them in all(...) only when there are 2+ is correct and avoids all(single) noise.

Per-fn registration ctors

The rewrite of derive_contract_function_registration_ctor at soroban-sdk-macros/src/derive_fn.rs:236-290 is the right shape: one ctor per fn, each carrying the original fn's pass-through attrs (including #[cfg]), so a cfg-disabled wrapper drops its ctor with it. Filtering doc attrs out of the hash input at line 266 is also correct — otherwise docstring edits would invisibly rename the ctor.

The hash includes the fn ident and all pass-through attrs except doc, so two cfg-gated variants of the same fn name within one trait impl produce distinct ctor identifiers. The 64-char hex with leading underscores keeps format_ident! happy. No collision risk in practice.

Minor observations (non-blocking)

  1. Identical snapshot JSONs. tests/contracttrait_trait/test_snapshots/test/test_cfg_gated_*.1.json (4 files) are byte-identical (md5sum confirms). That is expected because the snapshot captures only ledger state and these tests register no-state contracts — return-value behavior is asserted via assert_eq! in the test body. But it means the snapshots add little verification value beyond confirming the contract registers. Worth flagging only because if a future contributor edits one of these tests in a way that changes behavior, the snapshot won't catch it.

  2. Attribute propagation widening. Pass-through attrs now flow into derive_args_impl and per-fn registration ctors in addition to the existing wrapper paths. Visible in the expanded fixtures as small noise like a duplicated #[allow(non_snake_case)] on the __check_auth Args method (tests-expanded/test_account_tests.rs:331) and on the per-fn ctor at line 412. Harmless — duplicate #[allow] is a no-op — but worth knowing if user-supplied #[deny(...)] ever causes a surprise on a generated args body. No action needed.

  3. Cross-crate cfg evaluation context. The cross-crate scenario evaluates fn-level #[cfg] in the impl crate's feature set, not the trait crate's. The PR description and soroban-sdk/docs/contracttrait.md:92-94 both call this out. The test setup matches: test_contracttrait_trait has default = ["cfg-gated-fn"] so the trait method exists, while test_contracttrait_impl_full does not enable cfg-gated-fn by default, and test_cross_crate_cfg_gated_default_fn only exercises the unconditional shown(). Good test discipline given the limitation.

Conclusion

I have no required changes. The Copilot inline comment on derive_fn.rs:269 can be dismissed — it's based on a false premise about String: AsRef<[u8]>.
| Branch: support_contracttrait_cfg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Function attributes are dropped for #[contracttrait] default methods used by #[contractimpl(contracttrait)]

2 participants