From 5cb04544f83d92cac53e42840bad7ea250b5d997 Mon Sep 17 00:00:00 2001 From: Raushan kumar Date: Mon, 12 Jan 2026 11:17:44 +0000 Subject: [PATCH 1/2] refactor: improve macro diagnostics and validate unique account indices --- .../src/instruction/account_attrs.rs | 57 ++++--- .../src/instruction/account_attrs_test.rs | 67 ++++---- .../src/instruction/extract_instructions.rs | 7 +- .../src/instruction/instruction.rs | 21 +++ .../src/instruction/instruction_test.rs | 34 ++++ shank-macro-impl/src/krate/crate_context.rs | 4 +- .../src/parsed_struct/struct_attr.rs | 153 +++++++++++------- 7 files changed, 215 insertions(+), 128 deletions(-) diff --git a/shank-macro-impl/src/instruction/account_attrs.rs b/shank-macro-impl/src/instruction/account_attrs.rs index 9a01a9b..d6c0d87 100644 --- a/shank-macro-impl/src/instruction/account_attrs.rs +++ b/shank-macro-impl/src/instruction/account_attrs.rs @@ -2,8 +2,8 @@ use std::convert::TryFrom; use proc_macro2::Span; use syn::{ - punctuated::Punctuated, Attribute, Error as ParseError, Ident, Lit, Meta, MetaList, - MetaNameValue, NestedMeta, Result as ParseResult, Token, + punctuated::Punctuated, Attribute, Error as ParseError, Ident, Lit, Meta, + MetaList, MetaNameValue, NestedMeta, Result as ParseResult, Token, }; const IX_ACCOUNT: &str = "account"; @@ -35,7 +35,9 @@ impl InstructionAccount { } } - pub fn from_account_attr(attr: &Attribute) -> ParseResult { + pub fn from_account_attr( + attr: &Attribute, + ) -> ParseResult { let meta = &attr.parse_meta()?; match meta { @@ -73,7 +75,9 @@ impl InstructionAccount { let mut optional = false; for meta in nested { - if let Some((ident, name, value)) = string_assign_from_nested_meta(meta)? { + if let Some((ident, name, value)) = + string_assign_from_nested_meta(meta)? + { // name/desc match name.as_str() { "desc" | "description" | "docs" => desc = Some(value), @@ -84,14 +88,14 @@ impl InstructionAccount { )) } "name" => account_name = Some(value), - _ => { - return Err(ParseError::new_spanned( - ident, - "Only desc/description or name can be assigned strings", - )) - } + _ => return Err(ParseError::new_spanned( + ident, + "Only desc/description or name can be assigned strings", + )), }; - } else if let Some((ident, name)) = identifier_from_nested_meta(meta) { + } else if let Some((ident, name)) = + identifier_from_nested_meta(meta) + { // signer, writable, optional ... match name.as_str() { "signer" | "sign" | "sig" | "s" => signer = true, @@ -139,7 +143,9 @@ impl InstructionAccount { desc, optional, }), - None => Err(ParseError::new_spanned(nested, "Missing account name")), + None => { + Err(ParseError::new_spanned(nested, "Missing account name")) + } } } } @@ -154,21 +160,6 @@ impl TryFrom<&[Attribute]> for InstructionAccounts { .map(InstructionAccount::from_account_attr) .collect::>>()?; - for (idx, acc) in accounts.iter().enumerate() { - match acc.index { - Some(acc_idx) if acc_idx != idx as u32 => { - return Err(ParseError::new_spanned( - &acc.ident, - format!( - "Account index {} does not match its position {}", - acc_idx, idx, - ), - )); - } - _ => {} - } - } - Ok(InstructionAccounts(accounts)) } } @@ -180,7 +171,9 @@ fn string_assign_from_nested_meta( nested_meta: &NestedMeta, ) -> ParseResult> { match nested_meta { - NestedMeta::Meta(Meta::NameValue(MetaNameValue { path, lit, .. })) => { + NestedMeta::Meta(Meta::NameValue(MetaNameValue { + path, lit, .. + })) => { let ident = path.get_ident(); if let Some(ident) = ident { let token = match lit { @@ -199,10 +192,14 @@ fn string_assign_from_nested_meta( } } -pub fn identifier_from_nested_meta(nested_meta: &NestedMeta) -> Option<(Ident, String)> { +pub fn identifier_from_nested_meta( + nested_meta: &NestedMeta, +) -> Option<(Ident, String)> { match nested_meta { NestedMeta::Meta(meta) => match meta { - Meta::Path(_) => meta.path().get_ident().map(|x| (x.clone(), x.to_string())), + Meta::Path(_) => { + meta.path().get_ident().map(|x| (x.clone(), x.to_string())) + } // ignore named values and lists _ => None, }, diff --git a/shank-macro-impl/src/instruction/account_attrs_test.rs b/shank-macro-impl/src/instruction/account_attrs_test.rs index 45f3d6a..c269617 100644 --- a/shank-macro-impl/src/instruction/account_attrs_test.rs +++ b/shank-macro-impl/src/instruction/account_attrs_test.rs @@ -378,34 +378,43 @@ fn account_invalid_empty_name() { } #[test] -fn account_invalid_indexes() { - assert_matches!(parse_first_enum_variant_attrs(quote! { - #[derive(ShankInstruction)] - pub enum Instructions { - #[account(0, name ="authority", sig, desc = "Signer account")] - #[account(1, name ="storage", mut, desc = "Writable account")] - #[account(3, name ="funnel", desc = "Readonly account")] - Indexed - } - }) , - Err(err) if err.to_string().contains("index 3 does not match")); - assert_matches!(parse_first_enum_variant_attrs(quote! { - #[derive(ShankInstruction)] - pub enum Instructions { - #[account(1, name ="authority", sig, desc = "Signer account")] - Indexed - } - }) , - Err(err) if err.to_string().contains("index 1 does not match")); - assert_matches!(parse_first_enum_variant_attrs(quote! { - #[derive(ShankInstruction)] - pub enum Instructions { - #[account(0, name ="authority", sig, desc = "Signer account")] - #[account(2, name ="storage", mut, desc = "Writable account")] - #[account(2, name ="funnel", desc = "Readonly account")] - Indexed - } - }) , - Err(err) if err.to_string().contains("index 2 does not match")); +fn account_valid_sparse_indexes() { + assert_matches!( + parse_first_enum_variant_attrs(quote! { + #[derive(ShankInstruction)] + pub enum Instructions { + #[account(0, name ="authority", sig, desc = "Signer account")] + #[account(1, name ="storage", mut, desc = "Writable account")] + #[account(3, name ="funnel", desc = "Readonly account")] + Indexed + } + }), + Ok(_) + ); + + assert_matches!( + parse_first_enum_variant_attrs(quote! { + #[derive(ShankInstruction)] + pub enum Instructions { + #[account(1, name ="authority", sig, desc = "Signer account")] + Indexed + } + }), + Ok(_) + ); + + // This returns Ok here because duplicate checks are done at Instruction level, not InstructionAccounts level + assert_matches!( + parse_first_enum_variant_attrs(quote! { + #[derive(ShankInstruction)] + pub enum Instructions { + #[account(0, name ="authority", sig, desc = "Signer account")] + #[account(2, name ="storage", mut, desc = "Writable account")] + #[account(2, name ="funnel", desc = "Readonly account")] + Indexed + } + }), + Ok(_) + ); } diff --git a/shank-macro-impl/src/instruction/extract_instructions.rs b/shank-macro-impl/src/instruction/extract_instructions.rs index 07aef5b..b61744f 100644 --- a/shank-macro-impl/src/instruction/extract_instructions.rs +++ b/shank-macro-impl/src/instruction/extract_instructions.rs @@ -120,14 +120,11 @@ mod tests { } #[test] - fn extract_valid_instruction_and_invalid_account_idx() { + fn extract_valid_instruction_with_sparse_idx() { let res = parse_instructions(vec![ instruction_invalid_account_idx(), instruction_valid(), ]); - assert!(res - .unwrap_err() - .to_string() - .contains("Account index 1 does not match")); + assert!(res.is_ok()); } } diff --git a/shank-macro-impl/src/instruction/instruction.rs b/shank-macro-impl/src/instruction/instruction.rs index 52c07bb..a0e8a8a 100644 --- a/shank-macro-impl/src/instruction/instruction.rs +++ b/shank-macro-impl/src/instruction/instruction.rs @@ -141,6 +141,27 @@ impl TryFrom<&ParsedEnumVariant> for InstructionVariant { Err(_) => (attrs.try_into()?, attrs.into()), }; + // Validate unique indices in accounts + let mut indices = HashSet::new(); + for account in &accounts.0 { + if let Some(index) = account.index { + // Track seen indices using a HashSet for better efficiency. + if !indices.insert(index) { + // We can't easily get the span of the *previous* account here without more complex tracking, + // but we can error on the current duplicate. + // Ideally we'd point to the attribute span, but account.index is just a usize. + // However, we can use the span of the account name or the variant ident as a fallback context. + // Since InstructionAccount doesn't seem to carry the attribute Span, we use the variant ident. + + // Best effort: fail with a message. + return Err(ParseError::new( + ident.span(), + format!("Duplicate account index {} found in instruction variant '{}'", index, ident) + )); + } + } + } + Ok(Self { ident: ident.clone(), field_tys, diff --git a/shank-macro-impl/src/instruction/instruction_test.rs b/shank-macro-impl/src/instruction/instruction_test.rs index b1afbf0..2da5ddf 100644 --- a/shank-macro-impl/src/instruction/instruction_test.rs +++ b/shank-macro-impl/src/instruction/instruction_test.rs @@ -206,3 +206,37 @@ fn parse_non_instruction_enum() { "should be none" ); } + +// duplicate index tests +#[test] +fn fail_duplicate_account_indices() { + let result = parse_instruction(quote! { + #[derive(ShankInstruction)] + pub enum Instruction { + #[account(0, name = "creator", sig)] + #[account(0, name = "thing", mut)] + CreateThing, + } + }); + + assert!(result.is_err(), "Should have failed due to duplicate index 0"); + let err = result.err().unwrap(); + assert_eq!(err.to_string(), "Duplicate account index 0 found in instruction variant 'CreateThing'"); +} + +#[test] +fn fail_duplicate_account_indices_mixed() { + let result = parse_instruction(quote! { + #[derive(ShankInstruction)] + pub enum Instruction { + #[account(0, name = "creator", sig)] + #[account(1, name = "thing", mut)] + #[account(0, name = "duplicate", mut)] + CreateThing, + } + }); + + assert!(result.is_err(), "Should have failed due to duplicate index 0"); + let err = result.err().unwrap(); + assert_eq!(err.to_string(), "Duplicate account index 0 found in instruction variant 'CreateThing'"); +} diff --git a/shank-macro-impl/src/krate/crate_context.rs b/shank-macro-impl/src/krate/crate_context.rs index c682521..8b56914 100644 --- a/shank-macro-impl/src/krate/crate_context.rs +++ b/shank-macro-impl/src/krate/crate_context.rs @@ -28,11 +28,11 @@ impl CrateContext { self.modules.iter().flat_map(|(_, ctx)| ctx.macros()) } - pub fn modules(&self) -> impl Iterator { + pub fn modules(&self) -> impl Iterator> { self.modules.values().map(|detail| ModuleContext { detail }) } - pub fn root_module(&self) -> ModuleContext { + pub fn root_module(&self) -> ModuleContext<'_> { ModuleContext { detail: self.modules.get("crate").unwrap(), } diff --git a/shank-macro-impl/src/parsed_struct/struct_attr.rs b/shank-macro-impl/src/parsed_struct/struct_attr.rs index 87f0f38..98fe1de 100644 --- a/shank-macro-impl/src/parsed_struct/struct_attr.rs +++ b/shank-macro-impl/src/parsed_struct/struct_attr.rs @@ -36,7 +36,7 @@ impl Seeds { self.0.iter().filter_map(|x| x.get_param()).collect() } - pub fn iter(&self) -> Iter { + pub fn iter(&self) -> Iter<'_, Seed> { self.0.iter() } @@ -143,80 +143,107 @@ impl TryFrom<&[Attribute]> for StructAttrs { } // For now we only handle seeds as attributes on the `struct` itself - if seed_attrs.first().is_none() && pod_sentinel_attrs.first().is_none() { + if seed_attrs.is_empty() && pod_sentinel_attrs.is_empty() { return Ok(StructAttrs(HashSet::new())); } // Process seeds attribute if present if let Some(seed_attr) = seed_attrs.first() { let seed_attrs_meta = seed_attr.parse_meta()?; - let nested_args: Punctuated = { - use syn::Meta::*; - match seed_attrs_meta { - List(MetaList { nested, .. }) => nested, - Path(_) | NameValue(_) => { - return Ok(StructAttrs(HashSet::new())) + let nested_args: Punctuated = { + use syn::Meta::*; + match seed_attrs_meta { + List(MetaList { nested, .. }) => nested, + Path(_) | NameValue(_) => { + return Ok(StructAttrs(HashSet::new())) + } } - } - }; - let mut seeds = vec![]; - for arg in nested_args.iter() { - let seed = match arg { - NestedMeta::Meta(meta) => { - match meta { - // #[seeds(program_id)] - Meta::Path(path) => { - let Path { segments, .. } = path; - // Should be exactly one segment - if segments.len() != 1 { - Err(ParseError::new( - path.get_ident().unwrap().span(), - format!( + }; + let mut seeds = vec![]; + for arg in nested_args.iter() { + let seed = match arg { + NestedMeta::Meta(meta) => { + match meta { + // #[seeds(program_id)] + Meta::Path(path) => { + let Path { segments, .. } = path; + // Should be exactly one segment + if segments.len() != 1 { + Err(ParseError::new( + path.get_ident() + .map(|i| i.span()) + .unwrap_or_else(|| { + path.segments + .first() + .map(|s| s.ident.span()) + .unwrap_or_else( + Span::call_site, + ) + }), + format!( "This seed definition is invalid.\n{}", SUPPORTED_FORMATS ), - )) - } else { - let ident = &segments.first().unwrap().ident; - - match ident.to_string().as_str() { - "program_id" => Ok(Seed::ProgramId), - _ => Err(ParseError::new( - ident.span(), - format!( + )) + } else { + let ident = + &segments.first().unwrap().ident; + + match ident.to_string().as_str() { + "program_id" => Ok(Seed::ProgramId), + _ => Err(ParseError::new( + ident.span(), + format!( "This seed definition is invalid.\n{}", SUPPORTED_FORMATS ), - )), + )), + } } } + // #[seeds(some_pubkey("description of some pubkey", type?))] + Meta::List(MetaList { path, nested, .. }) => { + let ident = path.get_ident().ok_or_else(|| { + ParseError::new( + path.segments.first().map(|s| s.ident.span()).unwrap_or_else(Span::call_site), + "Seed attributes must be simple identifiers", + ) + })?; + let (desc, ty_str) = + param_args(nested, &ident.span())?; + let seed = Seed::Param( + ident.to_string(), + desc, + ty_str, + ); + Ok(seed) + } + Meta::NameValue(val) => Err(ParseError::new( + val.path + .get_ident() + .map(|i| i.span()) + .unwrap_or_else(|| { + val.path + .segments + .first() + .map(|s| s.ident.span()) + .unwrap_or_else(Span::call_site) + }), + format!( + "This seed definition is invalid.\n{}", + SUPPORTED_FORMATS + ), + )), } - // #[seeds(some_pubkey("description of some pubkey", type?))] - Meta::List(MetaList { path, nested, .. }) => { - let ident = path.get_ident().unwrap(); - let (desc, ty_str) = - param_args(nested, &ident.span())?; - let seed = - Seed::Param(ident.to_string(), desc, ty_str); - Ok(seed) - } - Meta::NameValue(val) => Err(ParseError::new( - val.path.get_ident().unwrap().span(), - format!( - "This seed definition is invalid.\n{}", - SUPPORTED_FORMATS - ), - )), } - } - // #[seeds("some:literal:string")] - NestedMeta::Lit(lit) => { - let seed = Seed::Literal(extract_lit_str(lit)?); - Ok(seed) - } - }?; - seeds.push(seed); - } + // #[seeds("some:literal:string")] + NestedMeta::Lit(lit) => { + let seed = Seed::Literal(extract_lit_str(lit)?); + Ok(seed) + } + }?; + seeds.push(seed); + } let seeds_struct_attr = StructAttr::Seeds(Seeds(seeds)); struct_attrs.insert(seeds_struct_attr); @@ -243,12 +270,13 @@ impl TryFrom<&[Attribute]> for StructAttrs { for arg in nested_args.iter() { match arg { NestedMeta::Lit(Lit::Int(int_lit)) => { - let byte_value = int_lit.base10_parse::().map_err(|_| { - ParseError::new( + let byte_value = + int_lit.base10_parse::().map_err(|_| { + ParseError::new( int_lit.span(), "Sentinel values must be u8 integers (0-255)", ) - })?; + })?; sentinel_bytes.push(byte_value); } _ => { @@ -267,7 +295,8 @@ impl TryFrom<&[Attribute]> for StructAttrs { )); } - let pod_sentinel_struct_attr = StructAttr::PodSentinel(sentinel_bytes); + let pod_sentinel_struct_attr = + StructAttr::PodSentinel(sentinel_bytes); struct_attrs.insert(pod_sentinel_struct_attr); } From 4d17694f2a5854c4e25636938ba33c3cb948022c Mon Sep 17 00:00:00 2001 From: Raushan kumar Date: Mon, 12 Jan 2026 11:44:51 +0000 Subject: [PATCH 2/2] refactor: address review feedback on comments and seed validation --- shank-macro-impl/src/instruction/instruction.rs | 8 +------- shank-macro-impl/src/parsed_struct/struct_attr.rs | 2 +- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/shank-macro-impl/src/instruction/instruction.rs b/shank-macro-impl/src/instruction/instruction.rs index a0e8a8a..0ed316b 100644 --- a/shank-macro-impl/src/instruction/instruction.rs +++ b/shank-macro-impl/src/instruction/instruction.rs @@ -147,13 +147,7 @@ impl TryFrom<&ParsedEnumVariant> for InstructionVariant { if let Some(index) = account.index { // Track seen indices using a HashSet for better efficiency. if !indices.insert(index) { - // We can't easily get the span of the *previous* account here without more complex tracking, - // but we can error on the current duplicate. - // Ideally we'd point to the attribute span, but account.index is just a usize. - // However, we can use the span of the account name or the variant ident as a fallback context. - // Since InstructionAccount doesn't seem to carry the attribute Span, we use the variant ident. - - // Best effort: fail with a message. + // Detect duplicate account indices and error on the current variant using ident.span() as fallback. return Err(ParseError::new( ident.span(), format!("Duplicate account index {} found in instruction variant '{}'", index, ident) diff --git a/shank-macro-impl/src/parsed_struct/struct_attr.rs b/shank-macro-impl/src/parsed_struct/struct_attr.rs index 98fe1de..ed2f7b7 100644 --- a/shank-macro-impl/src/parsed_struct/struct_attr.rs +++ b/shank-macro-impl/src/parsed_struct/struct_attr.rs @@ -155,7 +155,7 @@ impl TryFrom<&[Attribute]> for StructAttrs { match seed_attrs_meta { List(MetaList { nested, .. }) => nested, Path(_) | NameValue(_) => { - return Ok(StructAttrs(HashSet::new())) + return Err(ParseError::new(Span::call_site(), "seeds requires a comma-separated list of seeds, e.g., #[seeds(\"const\", pubkey(\"description\"))]")); } } };