Improve macro diagnostics and validate account indices

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<InstructionAccount> {
+    pub fn from_account_attr(
+        attr: &Attribute,
+    ) -> ParseResult<InstructionAccount> {
         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::<ParseResult<Vec<InstructionAccount>>>()?;
 
-        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<Option<(Ident, String, String)>> {
     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,
         },

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(_)
+    );
 }

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());
     }
 }

shank-macro-impl/src/instruction/instruction.rs

@@ -141,6 +141,21 @@ 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) {
+                    // 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)
+                    ));
+                }
+            }
+        }
+
         Ok(Self {
             ident: ident.clone(),
             field_tys,

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'");
+}

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<Item = ModuleContext> {
+    pub fn modules(&self) -> impl Iterator<Item = ModuleContext<'_>> {
         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(),
         }