Improve macro diagnostics and validate account indices#79
Improve macro diagnostics and validate account indices#79raushan728 wants to merge 2 commits intometaplex-foundation:masterfrom
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughMove per-account index enforcement out of account parsing into InstructionVariant construction (unique-index HashSet). Add empty account-name validation, tighten seed/pod_sentinel parsing error spans and lifetimes on some public return types. Changes
Sequence Diagram(s)(Skipped — changes are localized parsing/validation and do not introduce a multi-component sequential flow that benefits from a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)shank-macro-impl/src/parsed_struct/struct_attr.rs (6)
🔇 Additional comments (5)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shank-macro-impl/src/parsed_struct/struct_attr.rs (1)
338-348: Consider defensive handling for multi-segment paths inparam_args.While this code is outside the changed lines, these
unwrap()calls onget_ident()could panic if a multi-segment path is passed (e.g.,some::Type). The main parsing logic now handles this gracefully with fallback spans. For consistency with the PR's goal of improving error diagnostics, consider applying similar defensive handling here.♻️ Suggested defensive handling
NestedMeta::Meta(Meta::Path(path)) => { - Ok(Some(path.get_ident().unwrap().to_string())) + Ok(Some( + path.get_ident() + .map(|i| i.to_string()) + .unwrap_or_else(|| { + path.segments + .iter() + .map(|s| s.ident.to_string()) + .collect::<Vec<_>>() + .join("::") + }), + )) } NestedMeta::Meta(Meta::List(list)) => Err(ParseError::new( - list.path.get_ident().unwrap().span(), + list.path.get_ident().map(|i| i.span()).unwrap_or_else(Span::call_site), format!("Second arg to Param needs to be an exactly one Rust type, tuples or collections are not supported.\n{}", SUPPORTED_FORMATS), )), NestedMeta::Meta(Meta::NameValue(val),) => Err(ParseError::new( - val.path.get_ident().unwrap().span(), + val.path.get_ident().map(|i| i.span()).unwrap_or_else(Span::call_site), format!("Second arg to Param needs to be an exactly one Rust type, assignments are not supported.\n{}", SUPPORTED_FORMATS), )),
🤖 Fix all issues with AI agents
In @shank-macro-impl/src/instruction/instruction.rs:
- Around line 144-162: The duplicate-index check currently uses let mut indices
= std::collections::HashMap::new() and inserts the whole account; change this to
a HashSet of the index type (e.g., HashSet<u32> or usize as used by
InstructionAccount::index) and call indices.insert(index) to detect duplicates
(insert returns false if already present). Update the loop over accounts.0 to
only store the index in the set and, on insert returning false, return the same
ParseError using ident.span() and the existing error message; remove storing
account references since they are unnecessary.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
shank-idl/src/file.rsshank-idl/src/idl_type.rsshank-idl/src/idl_type_definition.rsshank-idl/tests/accounts.rsshank-idl/tests/fixtures/accounts/single_file/field_attributes.rsshank-macro-impl/src/instruction/account_attrs.rsshank-macro-impl/src/instruction/account_attrs_test.rsshank-macro-impl/src/instruction/extract_instructions.rsshank-macro-impl/src/instruction/idl_instruction_attrs.rsshank-macro-impl/src/instruction/instruction.rsshank-macro-impl/src/instruction/instruction_test.rsshank-macro-impl/src/krate/crate_context.rsshank-macro-impl/src/parsed_struct/struct_attr.rsshank-macro-impl/src/parsed_struct/struct_field_attr.rsshank-macro-impl/src/types/resolve_rust_ty.rsshank-macro/src/lib.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format all Rust source files according to rustfmt.toml
Use #[derive(ShankAccount)] to mark account structs; optionally annotate with #[seeds] for PDA generation
Use #[derive(ShankInstruction)] to mark instruction enums; use #[account] attributes on fields as needed
Use #[derive(ShankType)] to mark custom types for IDL inclusion
Use #[derive(ShankBuilder)] to generate instruction builders
Use #[derive(ShankContext)] to generate account context structs
Use #[padding] on fields to mark padding in the generated IDL
Use #[idl_type("TypeName")] to override a field’s IDL type
Use #[idl_name("name")] to rename a field in the IDL while keeping the Rust field name
Use #[skip] on fields to exclude them from the generated IDL
Files:
shank-macro-impl/src/instruction/instruction_test.rsshank-macro-impl/src/types/resolve_rust_ty.rsshank-idl/src/idl_type_definition.rsshank-idl/tests/accounts.rsshank-macro-impl/src/parsed_struct/struct_field_attr.rsshank-idl/tests/fixtures/accounts/single_file/field_attributes.rsshank-macro/src/lib.rsshank-macro-impl/src/instruction/instruction.rsshank-idl/src/file.rsshank-macro-impl/src/krate/crate_context.rsshank-idl/src/idl_type.rsshank-macro-impl/src/instruction/extract_instructions.rsshank-macro-impl/src/instruction/account_attrs_test.rsshank-macro-impl/src/parsed_struct/struct_attr.rsshank-macro-impl/src/instruction/account_attrs.rsshank-macro-impl/src/instruction/idl_instruction_attrs.rs
**/tests/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place test files and fixtures under each crate’s tests/ directory
Files:
shank-idl/tests/accounts.rsshank-idl/tests/fixtures/accounts/single_file/field_attributes.rs
🧬 Code graph analysis (7)
shank-idl/tests/accounts.rs (5)
shank-idl/tests/instructions.rs (1)
fixtures_dir(5-8)shank-idl/tests/macros.rs (1)
fixtures_dir(5-8)shank-idl/tests/errors.rs (1)
fixtures_dir(5-8)shank-idl/tests/types.rs (1)
fixtures_dir(5-8)shank-idl/src/file.rs (2)
parse_file(64-103)optional_program_address(51-56)
shank-macro-impl/src/instruction/instruction.rs (1)
shank-idl/src/file.rs (1)
accounts(105-114)
shank-idl/src/idl_type.rs (2)
shank-macro-impl/src/parsed_struct/parsed_struct_test.rs (1)
vec(195-210)shank-idl/src/file.rs (1)
types(145-168)
shank-macro-impl/src/instruction/account_attrs_test.rs (1)
shank-macro-impl/src/instruction/strategy_attrs_test.rs (1)
parse_first_enum_variant_attrs(18-27)
shank-macro-impl/src/parsed_struct/struct_attr.rs (2)
shank-macro-impl/src/parsed_struct/seed.rs (2)
new(53-55)new(65-67)shank-macro-impl/src/parsed_struct/struct_field_attr.rs (1)
meta(100-106)
shank-macro-impl/src/instruction/account_attrs.rs (3)
shank-macro-impl/src/instruction/strategy_attrs.rs (1)
from_account_attr(17-24)shank-macro-impl/src/types/render_rust_ty.rs (1)
ident(108-110)shank-macro-impl/src/parsed_struct/struct_field_attr.rs (1)
meta(100-106)
shank-macro-impl/src/instruction/idl_instruction_attrs.rs (3)
shank-macro-impl/src/instruction/account_attrs.rs (1)
identifier_from_nested_meta(195-208)shank-macro-impl/src/types/render_rust_ty.rs (1)
ident(108-110)shank-macro-impl/src/types/resolve_rust_ty.rs (1)
reference(137-149)
🔇 Additional comments (20)
shank-idl/src/idl_type_definition.rs (1)
59-127: LGTM! Formatting improvements enhance readability.All changes in this file are formatting refinements that improve code readability without altering behavior:
- Multi-line serde attribute formatting
- Improved closure and struct initialization formatting
As per coding guidelines, these changes align with rustfmt standards.
shank-idl/src/idl_type.rs (1)
49-66: LGTM! Consistent formatting improvements.The changes improve code clarity:
- Sentinel byte literal formatting with explicit values enhances readability while maintaining identical behavior
- Multi-line error message and test assertion formatting aligns with rustfmt standards
All functional logic remains unchanged.
Also applies to: 179-197, 359-537
shank-macro-impl/src/parsed_struct/struct_field_attr.rs (1)
51-56: LGTM! Formatting aligns with rustfmt standards.All changes are multi-line formatting improvements for attribute parsing logic. Error handling and control flow remain unchanged.
Also applies to: 73-76, 122-124
shank-idl/src/file.rs (1)
252-320: LGTM! Pattern matching and error formatting improvements.The formatting changes enhance readability:
- Multi-line pattern matches for
IdlTypeDefinitionTyandEnumFieldsvariants- Improved closure and error macro formatting
All logic and control flow remain semantically identical.
Also applies to: 373-440, 460-463, 505-508
shank-idl/tests/fixtures/accounts/single_file/field_attributes.rs (1)
23-23: LGTM! Whitespace formatting improvement.Added blank line improves visual separation between field declarations in the test fixture.
shank-idl/tests/accounts.rs (2)
81-83: ✅ Formatting improvements for fixture path construction.The multi-line chaining of
.join()calls improves readability and aligns with the workspace formatting cleanup objectives. Consistent with the pattern used in other test files (types.rs, errors.rs, instructions.rs, macros.rs).Also applies to: 103-105
108-128: ✅ Improved assertion formatting for better error diagnostics.Splitting assertion messages across multiple lines enhances readability and provides clearer error output when tests fail. The assertion logic remains unchanged and functionally equivalent to the original inline assertions.
shank-macro-impl/src/instruction/idl_instruction_attrs.rs (7)
6-18: LGTM! Import reorganization improves readability.The import restructuring groups related types logically and follows Rust formatting conventions.
33-40: LGTM! Formatting improves readability.The multi-line formatting makes the matching logic clearer without changing behavior.
43-45: LGTM! Consistent formatting improvements.Function signature and match arm formatting enhancements improve code readability without altering behavior.
Also applies to: 147-147, 167-167, 196-196
219-222: LGTM! Signature formatting is consistent.Multi-line formatting aligns with the codebase style.
224-232: LGTM! Explicit type construction improves clarity.The explicit RustType construction for
data_lenmakes the type definition more maintainable. All fields (kind, context, reference) are correctly configured for a U64 instruction field.
233-245: LGTM! Explicit type construction for SetAuthority.The braced block and explicit RustType construction for
new_authoritycorrectly specifies the Pubkey type with appropriate context and ownership semantics.
246-262: LGTM! Proper Vec type representation.The explicit nested RustType construction correctly represents
idl_dataasVec<u8>:
- Outer type: Composite::Vec with Default context and Owned reference
- Inner type: Primitive::U8 with CollectionItem context (correct for Vec elements)
This verbose but explicit approach improves type structure clarity and maintainability.
shank-macro-impl/src/instruction/extract_instructions.rs (1)
123-129: LGTM! Test correctly reflects sparse index support.The test rename and updated assertion properly validate that sparse (non-sequential) account indices are now accepted, aligning with the PR's objective to increase indexing flexibility.
shank-macro-impl/src/instruction/account_attrs_test.rs (1)
381-408: LGTM!The test correctly validates that sparse/non-sequential indices are now accepted at the account parsing level. The clarifying comment on lines 407-408 is helpful for understanding the validation architecture, making it clear that duplicate index detection happens at the instruction level (covered by separate tests in
instruction_test.rs).shank-macro-impl/src/instruction/instruction_test.rs (1)
210-253: LGTM! Good test coverage for duplicate index validation.The tests effectively verify:
- Immediate duplicate indices (consecutive accounts with same index)
- Non-adjacent duplicates (same index with other accounts in between)
Both tests assert the exact error message format, which helps prevent regression in error reporting quality.
shank-macro-impl/src/krate/crate_context.rs (1)
31-39: LGTM! Explicit lifetimes improve code clarity.Adding explicit
'_lifetime annotations toModuleContextreturn types makes the borrowing relationship with&selfclear to readers. This is a good practice that improves maintainability without changing behavior.shank-macro-impl/src/parsed_struct/struct_attr.rs (2)
39-41: LGTM!Adding the explicit
'_lifetime annotation to theiter()return type is consistent with the similar changes incrate_context.rsand improves code clarity.
163-253: Good refactoring with improved error diagnostics.The refactored seed parsing logic:
- Provides more precise error spans by extracting spans from path segments when
get_ident()returnsNone- Handles all
Metavariants explicitly (Path,List,NameValue)- Uses appropriate
SAFETYcomment (line 189) documenting theunwrap()after the length checkThis aligns well with the PR objective of replacing unwrap/panic with better error reporting.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @shank-macro-impl/src/instruction/instruction.rs:
- Around line 144-163: The validation is fine; remove or condense the overly
verbose explanatory comments around the duplicate-index check (the block
describing inability to get previous span) and replace them with a short note.
Keep the logic using a HashSet named indices over accounts.0 and the error path
that returns ParseError::new(ident.span(), ...) when account.index is
duplicated; you only need a concise comment like "Fail on duplicate account
index; use variant ident span as fallback."
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
shank-macro-impl/src/instruction/instruction.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format all Rust source files according to rustfmt.toml
Use #[derive(ShankAccount)] to mark account structs; optionally annotate with #[seeds] for PDA generation
Use #[derive(ShankInstruction)] to mark instruction enums; use #[account] attributes on fields as needed
Use #[derive(ShankType)] to mark custom types for IDL inclusion
Use #[derive(ShankBuilder)] to generate instruction builders
Use #[derive(ShankContext)] to generate account context structs
Use #[padding] on fields to mark padding in the generated IDL
Use #[idl_type("TypeName")] to override a field’s IDL type
Use #[idl_name("name")] to rename a field in the IDL while keeping the Rust field name
Use #[skip] on fields to exclude them from the generated IDL
Files:
shank-macro-impl/src/instruction/instruction.rs
🔇 Additional comments (1)
shank-macro-impl/src/instruction/instruction.rs (1)
1-4: LGTM: HashSet is the right tool for duplicate detection.The existing HashSet import is appropriately leveraged for O(1) duplicate index detection in the new validation logic.
|
Thank you for your PR contribution! Could you please separate out all formatting changes into a separate PR? This makes review easier and keeps functional vs formatting scopes separate for cleaner PR scoping. |
raushan728
force-pushed
the
improve/macro-diagnostics-and-indices
branch
from
7a8ce08 to
5cb0454
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shank-macro-impl/src/parsed_struct/struct_attr.rs (2)
328-346: Potential panic on complex type paths.The
.unwrap()calls on lines 332, 335, and 339 can panic if the path contains multiple segments (e.g.,std::u32). While unlikely in typical usage, consider usingok_or_elsefor defensive error handling, consistent with lines 206-211.Suggested fix for line 332
NestedMeta::Meta(Meta::Path(path)) => { - Ok(Some(path.get_ident().unwrap().to_string())) + let ident = path.get_ident().ok_or_else(|| { + ParseError::new( + path.segments.first().map(|s| s.ident.span()).unwrap_or_else(Span::call_site), + format!("Second arg to Param needs to be a simple type identifier.\n{}", SUPPORTED_FORMATS), + ) + })?; + Ok(Some(ident.to_string())) }
273-289: LGTM with minor span improvement opportunity.The error handling for byte parsing is improved. Consider using the argument's span instead of
Span::call_site()on line 284 for more precise error location:_ => { return Err(ParseError::new( - Span::call_site(), + arg.span(), "pod_sentinel must contain only u8 integers, e.g., #[pod_sentinel(255, 255)]", )); }
🤖 Fix all issues with AI agents
In @shank-macro-impl/src/instruction/instruction.rs:
- Around line 144-163: Condense the verbose inline comments in the
duplicate-index check: replace the multi-line commentary around the HashSet
usage and fallback span logic with a concise single-line comment stating intent
(e.g., "Detect duplicate account indices and error on the current variant using
ident.span() as fallback"). Keep the actual logic unchanged (retain indices
HashSet, loop over accounts.0, the if let Some(index) = account.index branch,
and the ParseError::new call using ident.span() and the formatted message).
Ensure references to indices, account.index, ident.span(), and ParseError::new
remain intact.
In @shank-macro-impl/src/parsed_struct/struct_attr.rs:
- Around line 153-161: The code currently swallows malformed seeds attributes by
returning Ok(StructAttrs(HashSet::new())) when seed_attrs_meta is Path or
NameValue; instead, raise a syn::Error to surface the misconfiguration (like the
pod_sentinel handling). Replace the early return for Path(_) | NameValue(_) with
returning Err(syn::Error::new_spanned(seed_attrs_meta, "<descriptive message>"))
(or similar error construction used for pod_sentinel) so callers see a clear
compile-time diagnostic; keep StructAttrs and nested_args handling unchanged
otherwise.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
shank-macro-impl/src/instruction/account_attrs.rsshank-macro-impl/src/instruction/account_attrs_test.rsshank-macro-impl/src/instruction/extract_instructions.rsshank-macro-impl/src/instruction/instruction.rsshank-macro-impl/src/instruction/instruction_test.rsshank-macro-impl/src/krate/crate_context.rsshank-macro-impl/src/parsed_struct/struct_attr.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format all Rust source files according to rustfmt.toml
Use #[derive(ShankAccount)] to mark account structs; optionally annotate with #[seeds] for PDA generation
Use #[derive(ShankInstruction)] to mark instruction enums; use #[account] attributes on fields as needed
Use #[derive(ShankType)] to mark custom types for IDL inclusion
Use #[derive(ShankBuilder)] to generate instruction builders
Use #[derive(ShankContext)] to generate account context structs
Use #[padding] on fields to mark padding in the generated IDL
Use #[idl_type("TypeName")] to override a field’s IDL type
Use #[idl_name("name")] to rename a field in the IDL while keeping the Rust field name
Use #[skip] on fields to exclude them from the generated IDL
Files:
shank-macro-impl/src/instruction/instruction.rsshank-macro-impl/src/krate/crate_context.rsshank-macro-impl/src/instruction/instruction_test.rsshank-macro-impl/src/parsed_struct/struct_attr.rsshank-macro-impl/src/instruction/account_attrs_test.rsshank-macro-impl/src/instruction/account_attrs.rsshank-macro-impl/src/instruction/extract_instructions.rs
🧬 Code graph analysis (3)
shank-macro-impl/src/parsed_struct/struct_attr.rs (3)
shank-macro-impl/src/parsed_struct/seed.rs (2)
new(53-55)new(65-67)shank-macro-impl/src/parsed_struct/parsed_struct_test.rs (1)
vec(195-210)shank-macro-impl/src/parsed_struct/struct_field_attr.rs (1)
meta(93-99)
shank-macro-impl/src/instruction/account_attrs_test.rs (1)
shank-macro-impl/src/instruction/strategy_attrs_test.rs (1)
parse_first_enum_variant_attrs(18-27)
shank-macro-impl/src/instruction/account_attrs.rs (3)
shank-macro-impl/src/instruction/strategy_attrs.rs (1)
from_account_attr(17-24)shank-macro-impl/src/types/render_rust_ty.rs (1)
ident(108-110)shank-macro-impl/src/parsed_struct/struct_field_attr.rs (1)
meta(93-99)
🔇 Additional comments (9)
shank-macro-impl/src/instruction/extract_instructions.rs (1)
122-129: LGTM!The test rename and assertion update correctly reflect the behavioral change: sparse (non-sequential) indices are now allowed at the account parsing level. Duplicate index validation has been moved to the
Instructionlevel, making this test appropriate.shank-macro-impl/src/instruction/account_attrs_test.rs (1)
382-419: LGTM!The test suite correctly validates sparse indexing behavior and includes a helpful comment explaining that duplicate checks occur at the
Instructionlevel. The three cases cover:
- Non-sequential indices (gaps in sequence)
- Non-zero starting index
- Duplicate indices passing at this level (validated elsewhere)
This aligns well with the PR's architectural decision to centralize duplicate validation.
shank-macro-impl/src/krate/crate_context.rs (1)
31-39: LGTM!Making the lifetime
'_explicit in the return types ofmodules()androot_module()is a good practice for public API clarity. This makes the borrowing relationship with&selfvisible to API consumers without changing behavior.shank-macro-impl/src/instruction/account_attrs.rs (3)
84-89: Good defensive validation.Adding validation for empty account names prevents downstream issues with empty identifiers. The error message is clear and uses proper span for error location.
153-164: Index consistency check removed as expected.The
TryFromimplementation now collects accounts without per-account index validation, correctly deferring duplicate detection to theInstructionlevel. This aligns with the PR's architectural change.
195-197: No action needed —pubvisibility is appropriate.The function is used outside
account_attrs.rs: it's imported inidl_instruction_attrs.rs(line 10) and called there (line 70), confirming thatpubvisibility is intentional and correctly reflects its role as part of the module's public API.shank-macro-impl/src/instruction/instruction_test.rs (1)
209-242: LGTM! Tests adequately verify duplicate index detection.The tests correctly validate that duplicate account indices within a single instruction variant produce the expected error. Consider adding a test case for duplicate non-zero indices (e.g., two accounts with index
2) to confirm the detection isn't specific to index0.shank-macro-impl/src/parsed_struct/struct_attr.rs (2)
39-41: LGTM! Explicit lifetime improves public API clarity.Making the lifetime explicit with
Iter<'_, Seed>is a good practice for public APIs and aligns with Rust 2018 edition recommendations.
162-246: LGTM! Improved error handling with precise spans.The refactored seeds processing logic provides better error messages with accurate source locations. The
unwrap()on line 190 is safe since it's guarded by thesegments.len() != 1check.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shank-macro-impl/src/parsed_struct/struct_attr.rs (1)
328-346: Potential panics fromunwrap()onget_ident()calls.The PR aims to replace panics with proper errors, but these
unwrap()calls can still panic if a user provides a qualified path (e.g.,some::module::Type) as the type argument:
- Line 332:
path.get_ident().unwrap()panics for multi-segment paths- Line 335:
list.path.get_ident().unwrap()panics before error is returned- Line 339:
val.path.get_ident().unwrap()panics before error is returnedProposed fix
NestedMeta::Meta(Meta::Path(path)) => { - Ok(Some(path.get_ident().unwrap().to_string())) + match path.get_ident() { + Some(ident) => Ok(Some(ident.to_string())), + None => Err(ParseError::new( + path.segments.first().map(|s| s.ident.span()).unwrap_or_else(Span::call_site), + format!("Second arg to Param needs to be a simple type identifier, qualified paths are not supported.\n{}", SUPPORTED_FORMATS), + )), + } } NestedMeta::Meta(Meta::List(list)) => Err(ParseError::new( - list.path.get_ident().unwrap().span(), + list.path.get_ident().map(|i| i.span()).unwrap_or_else(Span::call_site), format!("Second arg to Param needs to be an exactly one Rust type, tuples or collections are not supported.\n{}", SUPPORTED_FORMATS), )), NestedMeta::Meta(Meta::NameValue(val),) => Err(ParseError::new( - val.path.get_ident().unwrap().span(), + val.path.get_ident().map(|i| i.span()).unwrap_or_else(Span::call_site), format!("Second arg to Param needs to be an exactly one Rust type, assignments are not supported.\n{}", SUPPORTED_FORMATS), )),
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
shank-macro-impl/src/instruction/instruction.rsshank-macro-impl/src/parsed_struct/struct_attr.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.rs: Format all Rust source files according to rustfmt.toml
Use #[derive(ShankAccount)] to mark account structs; optionally annotate with #[seeds] for PDA generation
Use #[derive(ShankInstruction)] to mark instruction enums; use #[account] attributes on fields as needed
Use #[derive(ShankType)] to mark custom types for IDL inclusion
Use #[derive(ShankBuilder)] to generate instruction builders
Use #[derive(ShankContext)] to generate account context structs
Use #[padding] on fields to mark padding in the generated IDL
Use #[idl_type("TypeName")] to override a field’s IDL type
Use #[idl_name("name")] to rename a field in the IDL while keeping the Rust field name
Use #[skip] on fields to exclude them from the generated IDL
Files:
shank-macro-impl/src/parsed_struct/struct_attr.rsshank-macro-impl/src/instruction/instruction.rs
🧬 Code graph analysis (1)
shank-macro-impl/src/parsed_struct/struct_attr.rs (6)
shank-macro-impl/src/parsed_struct/seed.rs (2)
new(53-55)new(65-67)shank-macro-impl/src/error/program_error.rs (1)
syn(147-147)shank-macro-impl/src/macros/program_id.rs (1)
syn(69-69)shank-macro-impl/src/parsed_struct/mod.rs (1)
syn(18-18)shank-macro-impl/src/parsed_struct/struct_field_attr.rs (1)
meta(93-99)shank-macro-impl/src/types/render_rust_ty.rs (1)
ident(108-110)
🔇 Additional comments (5)
shank-macro-impl/src/instruction/instruction.rs (1)
144-157: LGTM! Clean duplicate index validation.The HashSet-based approach is efficient and the error message clearly identifies both the duplicate index and the affected variant. Using the variant's ident span provides reasonable error locality.
shank-macro-impl/src/parsed_struct/struct_attr.rs (4)
39-41: LGTM!Explicit lifetime annotation improves clarity and follows Rust 2018 edition conventions.
146-148: LGTM!Using
is_empty()is more idiomatic thanfirst().is_none().
153-246: Well-structured seed parsing refactor.The match-based parsing provides clearer control flow and better error messages. The span fallback logic (lines 173-182, 222-231) properly handles edge cases where
get_ident()might returnNone. The use ofsegments.first().unwrap()on line 190 is safe since it's in the else branch wheresegments.len() == 1.
273-279: LGTM!Good error handling with precise span pointing to the invalid integer literal.
|
Reset branch to remove global formatting. Included only functional logic and tests. Shall I provide a separate PR for formatting and clippy cleanup? |
2 participants
unwrap/panicwithsyn::Errorin macro parsing for better error reporting.