From abd5522d6af92068ab364bb866c114815142da0b Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Thu, 7 Nov 2024 01:57:38 +0800 Subject: [PATCH 1/3] lint ForbidKeysRemoveCall --- build.rs | 1 + support/linting/src/forbid_keys_remove.rs | 94 +++++++++++++++++++++++ support/linting/src/lib.rs | 2 + 3 files changed, 97 insertions(+) create mode 100644 support/linting/src/forbid_keys_remove.rs diff --git a/build.rs b/build.rs index 7261a28e1..1abd7456b 100644 --- a/build.rs +++ b/build.rs @@ -60,6 +60,7 @@ fn main() { }; track_lint(ForbidAsPrimitiveConversion::lint(&parsed_file)); + track_lint(ForbidKeysRemoveCall::lint(&parsed_file)); track_lint(RequireFreezeStruct::lint(&parsed_file)); track_lint(RequireExplicitPalletIndex::lint(&parsed_file)); }); diff --git a/support/linting/src/forbid_keys_remove.rs b/support/linting/src/forbid_keys_remove.rs new file mode 100644 index 000000000..d70ef568c --- /dev/null +++ b/support/linting/src/forbid_keys_remove.rs @@ -0,0 +1,94 @@ +use super::*; +use syn::{ + punctuated::Punctuated, spanned::Spanned, token::Comma, visit::Visit, Expr, ExprCall, ExprPath, + File, +}; + +pub struct ForbidKeysRemoveCall; + +impl Lint for ForbidKeysRemoveCall { + fn lint(source: &File) -> Result { + let mut visitor = KeysRemoveVisitor::default(); + visitor.visit_file(source); + + if visitor.errors.is_empty() { + Ok(()) + } else { + Err(visitor.errors) + } + } +} + +#[derive(Default)] +struct KeysRemoveVisitor { + errors: Vec, +} + +impl<'ast> Visit<'ast> for KeysRemoveVisitor { + fn visit_expr_call(&mut self, node: &'ast syn::ExprCall) { + let ExprCall { func, args, .. } = node; + if is_keys_remove_call(func, args) { + let msg = "Keys::::remove()` is banned to prevent accidentally breaking \ + the neuron sequence. If you need to replace neuron, try `SubtensorModule::replace_neuron()`"; + self.errors.push(syn::Error::new(node.func.span(), msg)); + } + } +} + +fn is_keys_remove_call(func: &Expr, args: &Punctuated) -> bool { + let Expr::Path(ExprPath { path, .. }) = func else { + return false; + }; + let func = &path.segments; + if func.len() != 2 || args.len() != 2 { + return false; + } + + func[0].ident == "Keys" + && !func[0].arguments.is_none() + && func[1].ident == "remove" + && func[1].arguments.is_none() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn lint(input: &str) -> Result { + let mut visitor = KeysRemoveVisitor::default(); + visitor + .visit_expr_call(&syn::parse_str(input).expect("should only use on a function call")); + + if visitor.errors.is_empty() { + Ok(()) + } else { + Err(visitor.errors) + } + } + + #[test] + fn test_keys_remove_forbidden() { + let input = r#"Keys::::remove(netuid, uid_to_replace)"#; + assert!(lint(input).is_err()); + let input = r#"Keys::::remove(netuid, uid_to_replace)"#; + assert!(lint(input).is_err()); + let input = r#"Keys::::remove(1, "2".parse().unwrap(),)"#; + assert!(lint(input).is_err()); + } + + #[test] + fn test_non_keys_remove_not_forbidden() { + let input = r#"remove(netuid, uid_to_replace)"#; + assert!(lint(input).is_ok()); + let input = r#"Keys::remove(netuid, uid_to_replace)"#; + assert!(lint(input).is_ok()); + let input = r#"Keys::::remove::(netuid, uid_to_replace)"#; + assert!(lint(input).is_ok()); + let input = r#"Keys::::remove(netuid, uid_to_replace, third_wheel)"#; + assert!(lint(input).is_ok()); + let input = r#"ParentKeys::remove(netuid, uid_to_replace)"#; + assert!(lint(input).is_ok()); + let input = r#"ChildKeys::::remove(netuid, uid_to_replace)"#; + assert!(lint(input).is_ok()); + } +} diff --git a/support/linting/src/lib.rs b/support/linting/src/lib.rs index 7aaf471c7..a65466e6a 100644 --- a/support/linting/src/lib.rs +++ b/support/linting/src/lib.rs @@ -2,9 +2,11 @@ pub mod lint; pub use lint::*; mod forbid_as_primitive; +mod forbid_keys_remove; mod pallet_index; mod require_freeze_struct; pub use forbid_as_primitive::ForbidAsPrimitiveConversion; +pub use forbid_keys_remove::ForbidKeysRemoveCall; pub use pallet_index::RequireExplicitPalletIndex; pub use require_freeze_struct::RequireFreezeStruct; From 1d6c938770529724c825d76678e445e6b79ab681 Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Tue, 12 Nov 2024 17:12:03 +0800 Subject: [PATCH 2/3] allow Keys::::remove() in replace_neuron() --- pallets/subtensor/src/subnets/uids.rs | 1 + support/linting/src/forbid_keys_remove.rs | 44 +++++++++++++++++------ support/linting/src/lint.rs | 29 ++++++++++++++- 3 files changed, 63 insertions(+), 11 deletions(-) diff --git a/pallets/subtensor/src/subnets/uids.rs b/pallets/subtensor/src/subnets/uids.rs index 2a5ceedb4..7ef51b8a4 100644 --- a/pallets/subtensor/src/subnets/uids.rs +++ b/pallets/subtensor/src/subnets/uids.rs @@ -29,6 +29,7 @@ impl Pallet { // 2. Remove previous set memberships. Uids::::remove(netuid, old_hotkey.clone()); IsNetworkMember::::remove(old_hotkey.clone(), netuid); + #[allow(unknown_lints)] Keys::::remove(netuid, uid_to_replace); // 2a. Check if the uid is registered in any other subnetworks. diff --git a/support/linting/src/forbid_keys_remove.rs b/support/linting/src/forbid_keys_remove.rs index d70ef568c..8f05aa021 100644 --- a/support/linting/src/forbid_keys_remove.rs +++ b/support/linting/src/forbid_keys_remove.rs @@ -1,7 +1,7 @@ use super::*; use syn::{ punctuated::Punctuated, spanned::Spanned, token::Comma, visit::Visit, Expr, ExprCall, ExprPath, - File, + File, Path, }; pub struct ForbidKeysRemoveCall; @@ -26,25 +26,30 @@ struct KeysRemoveVisitor { impl<'ast> Visit<'ast> for KeysRemoveVisitor { fn visit_expr_call(&mut self, node: &'ast syn::ExprCall) { - let ExprCall { func, args, .. } = node; - if is_keys_remove_call(func, args) { + let ExprCall { + func, args, attrs, .. + } = node; + + if is_keys_remove_call(func, args) && !is_allowed(attrs) { let msg = "Keys::::remove()` is banned to prevent accidentally breaking \ - the neuron sequence. If you need to replace neuron, try `SubtensorModule::replace_neuron()`"; + the neuron sequence. If you need to replace neurons, try `SubtensorModule::replace_neuron()`"; self.errors.push(syn::Error::new(node.func.span(), msg)); } } } fn is_keys_remove_call(func: &Expr, args: &Punctuated) -> bool { - let Expr::Path(ExprPath { path, .. }) = func else { + let Expr::Path(ExprPath { + path: Path { segments: func, .. }, + .. + }) = func + else { return false; }; - let func = &path.segments; - if func.len() != 2 || args.len() != 2 { - return false; - } - func[0].ident == "Keys" + func.len() == 2 + && args.len() == 2 + && func[0].ident == "Keys" && !func[0].arguments.is_none() && func[1].ident == "remove" && func[1].arguments.is_none() @@ -91,4 +96,23 @@ mod tests { let input = r#"ChildKeys::::remove(netuid, uid_to_replace)"#; assert!(lint(input).is_ok()); } + + #[test] + fn test_keys_remove_allowed() { + let input = r#" + #[allow(unknown_lints)] + Keys::::remove(netuid, uid_to_replace) + "#; + assert!(lint(input).is_ok()); + let input = r#" + #[allow(unknown_lints)] + Keys::::remove(netuid, uid_to_replace) + "#; + assert!(lint(input).is_ok()); + let input = r#" + #[allow(unknown_lints)] + Keys::::remove(1, "2".parse().unwrap(),) + "#; + assert!(lint(input).is_ok()); + } } diff --git a/support/linting/src/lint.rs b/support/linting/src/lint.rs index 3c099d40c..fdf35ae7c 100644 --- a/support/linting/src/lint.rs +++ b/support/linting/src/lint.rs @@ -1,4 +1,5 @@ -use syn::File; +use proc_macro2::TokenTree; +use syn::{Attribute, File, Meta, MetaList, Path}; pub type Result = core::result::Result<(), Vec>; @@ -11,3 +12,29 @@ pub trait Lint: Send + Sync { /// Lints the given Rust source file, returning a compile error if any issues are found. fn lint(source: &File) -> Result; } + +pub fn is_allowed(attibutes: &[Attribute]) -> bool { + attibutes.iter().any(|attribute| { + let Attribute { + meta: + Meta::List(MetaList { + path: Path { segments: attr, .. }, + tokens: attr_args, + .. + }), + .. + } = attribute + else { + return false; + }; + + attr.len() == 1 + && attr[0].ident == "allow" + && attr_args.clone().into_iter().any(|arg| { + let TokenTree::Ident(ref id) = arg else { + return false; + }; + id == "unknown_lints" + }) + }) +} From 63e971575b11fd047d94af1498703d2e618e622d Mon Sep 17 00:00:00 2001 From: Julian Eager Date: Wed, 13 Nov 2024 23:45:26 +0800 Subject: [PATCH 3/3] use TokenStream in place of str --- support/linting/src/forbid_as_primitive.rs | 17 ++++---- support/linting/src/forbid_keys_remove.rs | 49 +++++++++++----------- 2 files changed, 34 insertions(+), 32 deletions(-) diff --git a/support/linting/src/forbid_as_primitive.rs b/support/linting/src/forbid_as_primitive.rs index b60cf0a49..2af8e0132 100644 --- a/support/linting/src/forbid_as_primitive.rs +++ b/support/linting/src/forbid_as_primitive.rs @@ -45,10 +45,11 @@ fn is_as_primitive(ident: &Ident) -> bool { #[cfg(test)] mod tests { use super::*; + use quote::quote; - fn lint(input: &str) -> Result { - let expr: ExprMethodCall = syn::parse_str(input).expect("should only use on a method call"); + fn lint(input: proc_macro2::TokenStream) -> Result { let mut visitor = AsPrimitiveVisitor::default(); + let expr: ExprMethodCall = syn::parse2(input).expect("should be a valid method call"); visitor.visit_expr_method_call(&expr); if !visitor.errors.is_empty() { return Err(visitor.errors); @@ -58,21 +59,21 @@ mod tests { #[test] fn test_as_primitives() { - let input = r#"x.as_u32()"#; + let input = quote! {x.as_u32() }; assert!(lint(input).is_err()); - let input = r#"x.as_u64()"#; + let input = quote! {x.as_u64() }; assert!(lint(input).is_err()); - let input = r#"x.as_u128()"#; + let input = quote! {x.as_u128() }; assert!(lint(input).is_err()); - let input = r#"x.as_usize()"#; + let input = quote! {x.as_usize() }; assert!(lint(input).is_err()); } #[test] fn test_non_as_primitives() { - let input = r#"x.as_ref()"#; + let input = quote! {x.as_ref() }; assert!(lint(input).is_ok()); - let input = r#"x.as_slice()"#; + let input = quote! {x.as_slice() }; assert!(lint(input).is_ok()); } } diff --git a/support/linting/src/forbid_keys_remove.rs b/support/linting/src/forbid_keys_remove.rs index 8f05aa021..e7e5011b1 100644 --- a/support/linting/src/forbid_keys_remove.rs +++ b/support/linting/src/forbid_keys_remove.rs @@ -58,11 +58,12 @@ fn is_keys_remove_call(func: &Expr, args: &Punctuated) -> bool { #[cfg(test)] mod tests { use super::*; + use quote::quote; - fn lint(input: &str) -> Result { + fn lint(input: proc_macro2::TokenStream) -> Result { let mut visitor = KeysRemoveVisitor::default(); - visitor - .visit_expr_call(&syn::parse_str(input).expect("should only use on a function call")); + let expr: syn::ExprCall = syn::parse2(input).expect("should be a valid function call"); + visitor.visit_expr_call(&expr); if visitor.errors.is_empty() { Ok(()) @@ -73,46 +74,46 @@ mod tests { #[test] fn test_keys_remove_forbidden() { - let input = r#"Keys::::remove(netuid, uid_to_replace)"#; + let input = quote! { Keys::::remove(netuid, uid_to_replace) }; assert!(lint(input).is_err()); - let input = r#"Keys::::remove(netuid, uid_to_replace)"#; + let input = quote! { Keys::::remove(netuid, uid_to_replace) }; assert!(lint(input).is_err()); - let input = r#"Keys::::remove(1, "2".parse().unwrap(),)"#; + let input = quote! { Keys::::remove(1, "2".parse().unwrap(),) }; assert!(lint(input).is_err()); } #[test] fn test_non_keys_remove_not_forbidden() { - let input = r#"remove(netuid, uid_to_replace)"#; + let input = quote! { remove(netuid, uid_to_replace) }; assert!(lint(input).is_ok()); - let input = r#"Keys::remove(netuid, uid_to_replace)"#; + let input = quote! { Keys::remove(netuid, uid_to_replace) }; assert!(lint(input).is_ok()); - let input = r#"Keys::::remove::(netuid, uid_to_replace)"#; + let input = quote! { Keys::::remove::(netuid, uid_to_replace) }; assert!(lint(input).is_ok()); - let input = r#"Keys::::remove(netuid, uid_to_replace, third_wheel)"#; + let input = quote! { Keys::::remove(netuid, uid_to_replace, third_wheel) }; assert!(lint(input).is_ok()); - let input = r#"ParentKeys::remove(netuid, uid_to_replace)"#; + let input = quote! { ParentKeys::remove(netuid, uid_to_replace) }; assert!(lint(input).is_ok()); - let input = r#"ChildKeys::::remove(netuid, uid_to_replace)"#; + let input = quote! { ChildKeys::::remove(netuid, uid_to_replace) }; assert!(lint(input).is_ok()); } #[test] fn test_keys_remove_allowed() { - let input = r#" - #[allow(unknown_lints)] - Keys::::remove(netuid, uid_to_replace) - "#; + let input = quote! { + #[allow(unknown_lints)] + Keys::::remove(netuid, uid_to_replace) + }; assert!(lint(input).is_ok()); - let input = r#" - #[allow(unknown_lints)] - Keys::::remove(netuid, uid_to_replace) - "#; + let input = quote! { + #[allow(unknown_lints)] + Keys::::remove(netuid, uid_to_replace) + }; assert!(lint(input).is_ok()); - let input = r#" - #[allow(unknown_lints)] - Keys::::remove(1, "2".parse().unwrap(),) - "#; + let input = quote! { + #[allow(unknown_lints)] + Keys::::remove(1, "2".parse().unwrap(),) + }; assert!(lint(input).is_ok()); } }