Generate _no_ret functions in HostRef for mutable entrypoints returning some value

[kpob]

No description provided.

[llamapreview]

AI Code Review by LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Request Changes
This PR introduces a no-return feature for mutable functions but contains a critical bug in code generation that will cause compilation errors, alongside minor inconsistencies and maintenance issues.

📄 Documentation Diagram

This diagram documents the new workflow for generating _no_ret functions in the Odra macro system.

sequenceDiagram
    participant U as User Code
    participant M as Macro System
    participant H as HostRef Generator
    participant G as Generated Code
    U->>M: Define mutable function with return value
    note over M: PR #35;635 adds _no_ret generation
    M->>H: Process function signature
    H->>G: Generate try_* and try_*_no_ret functions
    note over G: try_*_no_ret returns Ok(())
    G-->>U: Callable no-return variant

Loading

🌟 Strengths

• Adds a useful feature for ignoring return values in mutable contract calls.
• Well-documented example module demonstrating the new functionality.

📋 Findings Summary

Priority	File	Category	Impact Summary (≤12 words)	Anchors
P1	odra-macros/src/ast/ref_utils.rs	Bug	Critical type mismatch in generated _no_ret functions causes compilation errors.	method:env_call
P2	odra-macros/src/ast/host_ref_item.rs	Architecture	Inconsistent HostRef generation could affect downstream code expectations.
P2	odra-wasm-client/src/client.rs	Maintainability	Hardcoded WASM file size constant is a maintenance hazard.
P2	examples/src/features/no_ret.rs	Testing	Unclear test naming reduces maintainability and clarity.
P2	odra-macros/src/ir/mod.rs	Architecture	Filtering logic has subtle assumptions that may not handle future return types.	method:host_functions

🔍 Notable Themes

• Code Generation Integrity: The P1 bug highlights a fundamental issue in the macro's code generation logic that must be fixed for the feature to work.
• API Consistency: Multiple findings point to inconsistencies in generated code shapes and naming, which could confuse users or break expectations.
• Maintenance Pitfalls: Small issues like hardcoded constants and unclear test names introduce avoidable friction for future development.

📈 Risk Diagram

This diagram illustrates the type mismatch risk in the generated _no_ret function implementation.

sequenceDiagram
    participant G as Generated try_swap_no_ret
    participant E as Env Call
    participant C as Contract Call
    G->>E: call_contract
    note over E: R1(P1): Incorrect return type
    E->>C: Execute swap
    C-->>E: Return U256
    E-->>G: OdraResult<U256>
    note over G: Expects OdraResult<()>
    G->>G: Type mismatch error

Loading

---

💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.

[llamapreview]

P1 | Confidence: High

The generated try_*_no_ret function incorrectly returns the same value as the original try_* function instead of discarding it. The env_call function generates a body that returns self.env.call_contract(...), which for the original try_swap would return OdraResult<U256>. However, the try_swap_no_ret function is declared to return OdraResult<()>, creating a type mismatch. This will cause compilation errors for any generated _no_ret function that returns a value. The impact is that the core feature of the PR (generating no-return variants) will not compile.

Code Suggestion:

pub fn host_mut_try_no_ret_function_item(fun: &FnIR) -> syn::ItemFn {
    let mut attrs = function_filtered_attrs(fun);
    attrs.push(parse_quote!(#[doc = " Ignores the result of the call."]));
    let ret_ty = utils::ty::odra_result_unit();
    let signature = function_signature(fun);
    let signature = syn::Signature {
        ident: fun.try_no_ret_name(),
        output: syn::parse_quote!(-> #ret_ty),
        ..signature
    };
    let try_func_name = fun.try_name();
    let args = fun.arg_names();
    syn::parse_quote!(
        #(#attrs)*
        pub #signature {
            let _ = self.#try_func_name(#(#args),*)?;
            Ok(())
        }
    )
}

Evidence: method:env_call

[llamapreview]

P2 | Confidence: High

Speculative: The HostRefItem::try_from implementation now conditionally includes the mut_no_ret_impl_item only when there are mutable functions that return a value. However, this creates an inconsistency: HostRef structs for modules with only immutable functions or void-returning mutable functions will have a different set of impl blocks than those with value-returning mutable functions. This could affect downstream code that uses reflection or expects certain trait implementations to be present. While not a breaking change, it creates two subtly different HostRef shapes in the ecosystem.

Code Suggestion:

Consider always generating the `mut_no_ret_impl_item` block (even if empty) for consistency, or document this behavioral difference clearly in the module's public API documentation.

[llamapreview]

P2 | Confidence: High

The test demonstrates the new _no_ret functionality but has a naming inconsistency. The module is called no_ret and demonstrates a MockDex with a swap function. However, the test function is named init_test, which doesn't accurately reflect what's being tested (swap_no_ret behavior). This reduces clarity for future maintainers.

Suggested change

	#[test]
	fn init_test() {
	let test_env = odra_test::env();
	let mut contract = MockDex::deploy(&test_env, NoArgs);
	assert_eq!(contract.try_swap_no_ret(1.into()), Ok(()));
	assert_eq!(contract.try_swap(1.into()), Ok(2.into()));
	}
	#[test]
	fn no_ret_function_works() {
	let test_env = odra_test::env();
	let mut contract = MockDex::deploy(&test_env, NoArgs);
	assert_eq!(contract.try_swap_no_ret(1.into()), Ok(()));
	assert_eq!(contract.try_swap(1.into()), Ok(2.into()));
	}

[llamapreview]

P2 | Confidence: Medium

The hardcoded byte array size 52814 for PROXY_CALLER is a maintenance hazard. If the proxy_caller_with_return.wasm file is rebuilt and changes size, this constant will cause a compilation error. This is unrelated to the PR's main feature but was modified (line break removed). The risk is that future developers might not realize they need to update the size when regenerating the WASM file.

Suggested change

	pub const PROXY_CALLER: &[u8; 52814] = include_bytes!("../resources/proxy_caller_with_return.wasm");
	pub const PROXY_CALLER: &[u8] = include_bytes!("../resources/proxy_caller_with_return.wasm");

[llamapreview]

P2 | Confidence: Medium

Speculative: The filtering logic for host_mut_ret_functions excludes functions with -> () return type (syn::ReturnType::Default). However, it doesn't consider functions that return Result<(), E> or other unit-like wrapper types. If the framework evolves to support such return types, they would incorrectly be excluded from _no_ret generation (since a _no_ret variant for Result<(), E> might still be useful to discard the Result). While not an issue with current usage patterns, this is a subtle assumption baked into the filtering logic.

Code Suggestion:

/// Returns mutable functions that return a non-unit type.
/// Note: This uses `syn::ReturnType::Default` which matches `-> ()`.
/// Functions returning `Result<(), E>` or other unit-containing types are excluded.

Evidence: method:host_functions