Finalize single-expression light handlers#2381
Open
TUPM96 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR refines how the macro detects whether a handler function consumes its context argument. A new helper function ( ChangesContext-move detection refinement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes are focused and consistent: a straightforward helper function addition, a localized logic substitution, and parallel test cases that directly mirror the implementation logic. No complex branching or semantic interactions across distant code paths. Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk-libs/macros/src/light_pdas/program/parsing.rs`:
- Around line 621-650: is_delegation_body currently only inspects
MethodCall.args via call_moves_ctx_arg and misses cases where the context is
moved as the MethodCall.receiver (e.g., ctx.delegate(...)); update the logic to
also check the MethodCall.receiver for moved ctx. Specifically, modify the
MethodCall branch in is_delegation_body to test call.receiver for being an
identifier equal to ctx_name or being a MethodCall "into" whose receiver is that
identifier (analogous to the existing checks in call_moves_ctx_arg), or refactor
call_moves_ctx_arg to accept and check a receiver Expr as well and reuse the
same moved-ctx detection for receiver and args; ensure you reference
is_delegation_body, call_moves_ctx_arg, syn::Expr::MethodCall and call.receiver
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75ba3b19-bf26-4005-8ed3-a2be612873dd
📒 Files selected for processing (2)
sdk-libs/macros/src/light_pdas/program/parsing.rssdk-libs/macros/src/light_pdas_tests/parsing_tests.rs
Comment on lines
+621
to
+650
| syn::Expr::MethodCall(call) => call_moves_ctx_arg(&call.args, ctx_name), | ||
| _ => false, | ||
| } | ||
| } | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| /// Check if any argument consumes the context param. | ||
| /// | ||
| /// Passing `&ctx` or `&mut ctx` lets the wrapper continue using `ctx` for | ||
| /// finalization after the handler returns, so those are intentionally excluded. | ||
| fn call_moves_ctx_arg( | ||
| args: &syn::punctuated::Punctuated<syn::Expr, syn::token::Comma>, | ||
| ctx_name: &str, | ||
| ) -> bool { | ||
| for arg in args { | ||
| match arg { | ||
| syn::Expr::Path(path) if path.path.is_ident(ctx_name) => return true, | ||
| syn::Expr::MethodCall(method_call) | ||
| if method_call.method == "into" | ||
| && matches!(&*method_call.receiver, syn::Expr::Path(path) if path.path.is_ident(ctx_name)) => | ||
| { | ||
| return true; | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| false | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'is_delegation_body|Expr::MethodCall|call_moves_ctx_arg|call\\.receiver' sdk-libs/macros/src/light_pdas/program/parsing.rs
rg -n -C2 'test_wrap_.*finalize|instructions::handler\\(&mut ctx\\)|instructions::handler\\(ctx\\)' sdk-libs/macros/src/light_pdas_tests/parsing_tests.rsRepository: Lightprotocol/light-protocol
Length of output: 3518
Fix delegation detection to account for moved ctx used as a method-call receiver
In sdk-libs/macros/src/light_pdas/program/parsing.rs (~lines 611-623), is_delegation_body checks syn::Expr::MethodCall(call) for delegation using only call.args via call_moves_ctx_arg, ignoring call.receiver. If a handler consumes ctx as the receiver (e.g., ctx.delegate(...)), delegation can be misclassified and the wrapper may incorrectly skip/emit the finalize path. sdk-libs/macros/src/light_pdas_tests/parsing_tests.rs only covers instructions::handler(ctx) / instructions::handler(&mut ctx) (arg-style Expr::Call), not receiver consumption.
Suggested fix
fn is_delegation_body(block: &syn::Block, ctx_name: &str) -> bool {
@@
syn::Stmt::Expr(expr, _) => {
// Check if it's a function call that takes ctx as an argument
match expr {
syn::Expr::Call(call) => call_moves_ctx_arg(&call.args, ctx_name),
- syn::Expr::MethodCall(call) => call_moves_ctx_arg(&call.args, ctx_name),
+ syn::Expr::MethodCall(call) => {
+ expr_moves_ctx(&call.receiver, ctx_name)
+ || call_moves_ctx_arg(&call.args, ctx_name)
+ }
_ => false,
}
}
@@
}
+fn expr_moves_ctx(expr: &syn::Expr, ctx_name: &str) -> bool {
+ match expr {
+ syn::Expr::Path(path) if path.path.is_ident(ctx_name) => true,
+ syn::Expr::MethodCall(method_call)
+ if method_call.method == "into"
+ && matches!(&*method_call.receiver, syn::Expr::Path(path) if path.path.is_ident(ctx_name)) =>
+ {
+ true
+ }
+ _ => false,
+ }
+}
+
fn call_moves_ctx_arg(
args: &syn::punctuated::Punctuated<syn::Expr, syn::token::Comma>,
ctx_name: &str,
) -> bool {
for arg in args {
- match arg {
- syn::Expr::Path(path) if path.path.is_ident(ctx_name) => return true,
- syn::Expr::MethodCall(method_call)
- if method_call.method == "into"
- && matches!(&*method_call.receiver, syn::Expr::Path(path) if path.path.is_ident(ctx_name)) =>
- {
- return true;
- }
- _ => {}
+ if expr_moves_ctx(arg, ctx_name) {
+ return true;
}
}
false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| syn::Expr::MethodCall(call) => call_moves_ctx_arg(&call.args, ctx_name), | |
| _ => false, | |
| } | |
| } | |
| _ => false, | |
| } | |
| } | |
| /// Check if any argument consumes the context param. | |
| /// | |
| /// Passing `&ctx` or `&mut ctx` lets the wrapper continue using `ctx` for | |
| /// finalization after the handler returns, so those are intentionally excluded. | |
| fn call_moves_ctx_arg( | |
| args: &syn::punctuated::Punctuated<syn::Expr, syn::token::Comma>, | |
| ctx_name: &str, | |
| ) -> bool { | |
| for arg in args { | |
| match arg { | |
| syn::Expr::Path(path) if path.path.is_ident(ctx_name) => return true, | |
| syn::Expr::MethodCall(method_call) | |
| if method_call.method == "into" | |
| && matches!(&*method_call.receiver, syn::Expr::Path(path) if path.path.is_ident(ctx_name)) => | |
| { | |
| return true; | |
| } | |
| _ => {} | |
| } | |
| } | |
| false | |
| } | |
| syn::Expr::MethodCall(call) => { | |
| expr_moves_ctx(&call.receiver, ctx_name) | |
| || call_moves_ctx_arg(&call.args, ctx_name) | |
| } | |
| _ => false, | |
| } | |
| } | |
| _ => false, | |
| } | |
| } | |
| fn expr_moves_ctx(expr: &syn::Expr, ctx_name: &str) -> bool { | |
| match expr { | |
| syn::Expr::Path(path) if path.path.is_ident(ctx_name) => true, | |
| syn::Expr::MethodCall(method_call) | |
| if method_call.method == "into" | |
| && matches!(&*method_call.receiver, syn::Expr::Path(path) if path.path.is_ident(ctx_name)) => | |
| { | |
| true | |
| } | |
| _ => false, | |
| } | |
| } | |
| /// Check if any argument consumes the context param. | |
| /// | |
| /// Passing `&ctx` or `&mut ctx` lets the wrapper continue using `ctx` for | |
| /// finalization after the handler returns, so those are intentionally excluded. | |
| fn call_moves_ctx_arg( | |
| args: &syn::punctuated::Punctuated<syn::Expr, syn::token::Comma>, | |
| ctx_name: &str, | |
| ) -> bool { | |
| for arg in args { | |
| if expr_moves_ctx(arg, ctx_name) { | |
| return true; | |
| } | |
| } | |
| false | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk-libs/macros/src/light_pdas/program/parsing.rs` around lines 621 - 650,
is_delegation_body currently only inspects MethodCall.args via
call_moves_ctx_arg and misses cases where the context is moved as the
MethodCall.receiver (e.g., ctx.delegate(...)); update the logic to also check
the MethodCall.receiver for moved ctx. Specifically, modify the MethodCall
branch in is_delegation_body to test call.receiver for being an identifier equal
to ctx_name or being a MethodCall "into" whose receiver is that identifier
(analogous to the existing checks in call_moves_ctx_arg), or refactor
call_moves_ctx_arg to accept and check a receiver Expr as well and reuse the
same moved-ctx detection for receiver and args; ensure you reference
is_delegation_body, call_moves_ctx_arg, syn::Expr::MethodCall and call.receiver
when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1290.
Summary
Validation
Summary by CodeRabbit
Bug Fixes
Tests