-
Notifications
You must be signed in to change notification settings - Fork 4
Fix generating getter functions in wasm client #628
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.
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
Conversation
- call `to_wasm_value` instead of `into_odra_value`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR fixes a method call in WASM client generation but introduces a critical breaking change that could break existing client code, along with a potential performance issue.
⚡ Key Risks & Improvements (P1)
- odra-wasm-client-builder/src/codegen/custom_types.rs: Introduces a breaking change that could cause compilation errors or runtime type mismatches for downstream WASM clients.
💡 Suggestions (P2)
- odra-wasm-client-builder/src/codegen/custom_types.rs: The unnecessary
.clone()operation could lead to performance overhead, especially for non-Copy types.
📈 Risk Diagram
This diagram illustrates the breaking change risk in the WASM client getter generation.
sequenceDiagram
participant Client
participant Getter
participant Trait
Client->>Getter: get_field()
Getter->>Trait: to_wasm_value(field.clone())
Trait-->>Getter: wasm_value
Getter-->>Client: wasm_value
note over Client,Getter: R1(P1): Breaking change: return type differs from previous into_odra_value,<br/>causing potential downstream errors.
💡 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.
| let ty = WasmType::from(&member.ty); | ||
| let ident = format_ident!("{}", member.name); | ||
|
|
||
| quote::quote! { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Contextual Comment]
This comment refers to code near real line 266. Anchored to nearest_changed(270) line 270.
P1 | Confidence: High
- This is a potentially breaking change to the public API of generated WebAssembly client code. The method
IntoWasmValue::into_odra_valueis replaced byIntoWasmValue::to_wasm_value, which likely have different signatures or return types, causing compilation errors or runtime type mismatches for downstream code. - Speculative: The change introduces an unnecessary
.clone()operation onself.#ident. If the field's type does not implementCopy, this could lead to expensive, avoidable clones, impacting performance.
Code Suggestion:
If a reference-taking method exists (e.g., `fn as_wasm_value(&self) -> #ty`), the code should use it:
odra_wasm_client::types::IntoWasmValue::as_wasm_value(&self.#ident)
Otherwise, the trait should be extended, or the clone may be necessary but should be documented as such.Evidence: symbol:IntoWasmValue, method:to_wasm_value, method:into_odra_value
Benchmark report
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/2.4.1 #628 +/- ##
=================================================
- Coverage 77.11% 76.87% -0.24%
=================================================
Files 204 227 +23
Lines 25894 29549 +3655
=================================================
+ Hits 19968 22717 +2749
- Misses 5926 6832 +906 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Benchmark report
|
to_wasm_valueinstead ofinto_odra_value