Misc clippy lints and clippy CI

Co-authored-by: Michael Krasnitski <[email protected]>

.github/workflows/rust.yml

@@ -10,8 +10,23 @@ on:
       - 'rust/**'
 
 jobs:
-  # TODO: Cargo clippy (just warnings don't fail)
   # TODO: Cargo test (we would need to pull in binary ninja)
+  # Check lints with clippy
+  clippy:
+    name: cargo clippy
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      # Ensure clippy is installed
+      - uses: actions-rust-lang/setup-rust-toolchain@v1
+        with:
+          components: clippy
+      - name: Clippy Check
+      - uses: clechasseur/rs-clippy-check@v4
+        with:
+          working-directory: ./rust
+          args: --all-features
+
   # Check formatting with rustfmt
   formatting:
     name: cargo fmt

rust/build.rs

@@ -5,7 +5,7 @@ fn main() {
     let _ = std::fs::create_dir("target/doc");
     let _ = std::fs::copy("../docs/img/favicon.ico", "target/doc/favicon.ico");
     let _ = std::fs::copy(
-        "under_construction.png",
+        "assets/under_construction.png",
         "target/doc/under_construction.png",
     );
     let _ = std::fs::copy("../docs/img/logo.png", "target/doc/logo.png");

rust/examples/workflow.rs

@@ -2,8 +2,8 @@ use binaryninja::binaryview::BinaryViewExt;
 use binaryninja::llil::{ExprInfo, LiftedNonSSA, NonSSA, VisitorAction};
 use binaryninja::workflow::{Activity, AnalysisContext, Workflow};
 
-const RUST_ACTIVITY_NAME: &'static str = "analysis.plugins.rustexample";
-const RUST_ACTIVITY_CONFIG: &'static str = r#"{
+const RUST_ACTIVITY_NAME: &str = "analysis.plugins.rustexample";
+const RUST_ACTIVITY_CONFIG: &str = r#"{
     "name": "analysis.plugins.rustexample",
     "title" : "Rust Example",
     "description": "This analysis step logs out some information about the function...",
@@ -27,18 +27,15 @@ fn example_activity(analysis_context: &AnalysisContext) {
             for instr in basic_block.iter() {
                 if let Some(llil_instr) = llil.instruction_at(instr) {
                     llil_instr.visit_tree(&mut |expr, info| {
-                        match info {
-                            ExprInfo::Const(_op) => {
-                                // Replace all consts with 0x1337.
-                                println!(
-                                    "Replacing llil expression @ 0x{:x} : {}",
-                                    instr, expr.index
-                                );
-                                unsafe {
-                                    llil.replace_expression(expr.index, llil.const_int(4, 0x1337))
-                                };
-                            }
-                            _ => {}
+                        if let ExprInfo::Const(_op) = info {
+                            // Replace all consts with 0x1337.
+                            println!(
+                                "Replacing llil expression @ 0x{:x} : {}",
+                                instr, expr.index
+                            );
+                            unsafe {
+                                llil.replace_expression(expr.index, llil.const_int(4, 0x1337))
+                            };
                         }
                         VisitorAction::Descend
                     });

rust/src/architecture.rs

@@ -200,6 +200,7 @@ impl From<BNInstructionInfo> for InstructionInfo {
     fn from(value: BNInstructionInfo) -> Self {
         // TODO: This is quite ugly, but we destructure the branch info so this will have to do.
         let mut branch_info = [None; NUM_BRANCH_INFO];
+        #[allow(clippy::needless_range_loop)]
         for i in 0..value.branchCount.min(NUM_BRANCH_INFO) {
             let branch_target = value.branchTarget[i];
             branch_info[i] = Some(BranchInfo {

rust/src/basicblock.rs

@@ -240,7 +240,7 @@ impl<C: BlockContext> BasicBlock<C> {
     // TODO iterated dominance frontier
 }
 
-impl<'a, C: BlockContext> IntoIterator for &'a BasicBlock<C> {
+impl<C: BlockContext> IntoIterator for &BasicBlock<C> {
     type Item = C::Instruction;
     type IntoIter = C::Iter;
 

rust/src/binaryview.rs

@@ -678,7 +678,7 @@ pub trait BinaryViewExt: BinaryViewBase {
         let name_array = unsafe { Array::<QualifiedName>::new(result_names, result_count, ()) };
         id_array
             .into_iter()
-            .zip(name_array.into_iter())
+            .zip(&name_array)
             .map(|(id, name)| (id.to_owned(), name))
             .collect()
     }

rust/src/component.rs

@@ -146,13 +146,10 @@ impl Component {
     }
 
     /// Original name set for this component
-
     /// :note: The `.display_name` property should be used for `bv.get_component_by_path()` lookups.
-
     /// This can differ from the .display_name property if one of its sibling components has the same .original_name; In that
     /// case, .name will be an automatically generated unique name (e.g. "MyComponentName (1)") while .original_name will
     /// remain what was originally set (e.g. "MyComponentName")
-
     /// If this component has a duplicate name and is moved to a component where none of its siblings share its name,
     /// .name will return the original "MyComponentName"
     pub fn name(&self) -> BnString {

rust/src/confidence.rs

@@ -189,7 +189,7 @@ impl<T> From<T> for Conf<T> {
     }
 }
 
-impl<'a> Conf<&'a Type> {
+impl Conf<&'_ Type> {
     pub(crate) fn into_raw(value: Self) -> BNTypeWithConfidence {
         BNTypeWithConfidence {
             type_: value.contents.handle,

rust/src/databuffer.rs

@@ -172,7 +172,7 @@ impl DataBuffer {
 
 impl Default for DataBuffer {
     fn default() -> Self {
-        Self(unsafe { BNCreateDataBuffer([].as_ptr() as *const c_void, 0) })
+        Self(unsafe { BNCreateDataBuffer([].as_ptr(), 0) })
     }
 }
 

rust/src/demangle.rs

@@ -301,10 +301,7 @@ impl Demangler {
             })
         }
 
-        extern "C" fn cb_free_var_name<C>(_ctxt: *mut c_void, name: *mut BNQualifiedName)
-        where
-            C: CustomDemangler,
-        {
+        extern "C" fn cb_free_var_name(_ctxt: *mut c_void, name: *mut BNQualifiedName) {
             ffi_wrap!("CustomDemangler::cb_free_var_name", unsafe {
                 // TODO: What is the point of this free callback?
                 QualifiedName::free_raw(*name)
@@ -319,7 +316,7 @@ impl Demangler {
             context: ctxt as *mut c_void,
             isMangledString: Some(cb_is_mangled_string::<C>),
             demangle: Some(cb_demangle::<C>),
-            freeVarName: Some(cb_free_var_name::<C>),
+            freeVarName: Some(cb_free_var_name),
         };
 
         unsafe {

rust/src/disassembly.rs

@@ -263,7 +263,7 @@ impl InstructionTextToken {
         let kind_value = value.kind.try_value().unwrap_or(0);
         let operand = value.kind.try_operand().unwrap_or(0);
         let size = value.kind.try_size().unwrap_or(0);
-        let type_names = value.kind.try_type_names().unwrap_or(vec![]);
+        let type_names = value.kind.try_type_names().unwrap_or_default();
         BNInstructionTextToken {
             type_: value.kind.into(),
             // NOTE: Expected to be freed with `InstructionTextToken::free_raw`.

rust/src/enterprise.rs

@@ -20,6 +20,7 @@ pub fn checkout_license(duration: Duration) -> Result<(), EnterpriseCheckoutErro
         return Ok(());
     }
 
+    #[allow(clippy::collapsible_if)]
     if !is_server_initialized() {
         if !initialize_server() && is_server_floating_license() {
             return Err(EnterpriseCheckoutError(server_last_error().to_string()));
@@ -31,6 +32,7 @@ pub fn checkout_license(duration: Duration) -> Result<(), EnterpriseCheckoutErro
             return Err(EnterpriseCheckoutError(server_last_error().to_string()));
         }
 
+        #[allow(clippy::collapsible_if)]
         if !is_server_authenticated() {
             if !authenticate_server_with_method("Keychain", false) {
                 let Some(username) = std::env::var("BN_ENTERPRISE_USERNAME").ok() else {
@@ -50,6 +52,7 @@ pub fn checkout_license(duration: Duration) -> Result<(), EnterpriseCheckoutErro
         }
     }
 
+    #[allow(clippy::collapsible_if)]
     if !is_server_license_still_activated()
         || (!is_server_floating_license() && crate::license_expiration_time() < SystemTime::now())
     {

rust/src/flowgraph.rs

@@ -92,7 +92,7 @@ impl<'a> FlowGraphNode<'a> {
     }
 }
 
-unsafe impl<'a> RefCountable for FlowGraphNode<'a> {
+unsafe impl RefCountable for FlowGraphNode<'_> {
     unsafe fn inc_ref(handle: &Self) -> Ref<Self> {
         Ref::new(Self {
             handle: BNNewFlowGraphNodeReference(handle.handle),
@@ -105,7 +105,7 @@ unsafe impl<'a> RefCountable for FlowGraphNode<'a> {
     }
 }
 
-impl<'a> ToOwned for FlowGraphNode<'a> {
+impl ToOwned for FlowGraphNode<'_> {
     type Owned = Ref<Self>;
 
     fn to_owned(&self) -> Self::Owned {

rust/src/hlil/instruction.rs

@@ -754,7 +754,7 @@ impl HighLevelILInstruction {
                         // TODO: Replace with a From<u32> for RegisterValueType.
                         // TODO: We might also want to change the type of `op.constant_data_kind`
                         // TODO: To RegisterValueType and do the conversion when creating instruction.
-                        state: unsafe { std::mem::transmute(op.constant_data_kind) },
+                        state: unsafe { std::mem::transmute::<u32, BNRegisterValueType>(op.constant_data_kind) },
                         value: op.constant_data_value,
                         offset: 0,
                         size: op.size,

rust/src/lib.rs

@@ -16,6 +16,7 @@
 #![allow(clippy::missing_safety_doc)]
 #![allow(clippy::result_unit_err)]
 #![allow(clippy::type_complexity)]
+#![allow(clippy::too_many_arguments)]
 #![doc(html_root_url = "https://dev-rust.binary.ninja/")]
 #![doc(html_favicon_url = "/favicon.ico")]
 #![doc(html_logo_url = "/logo.png")]
@@ -211,7 +212,7 @@ unsafe extern "C" fn cb_progress_func<F: FnMut(usize, usize) -> bool>(
     if ctxt.is_null() {
         return true;
     }
-    let closure: &mut F = std::mem::transmute(ctxt);
+    let closure = &mut *(ctxt as *mut F);
     closure(progress, total)
 }
 

rust/src/llil/block.rs

@@ -12,6 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+use std::fmt::Debug;
 use std::ops::Range;
 
 use crate::architecture::Architecture;
@@ -78,15 +79,16 @@ where
     }
 }
 
-impl<'func, A, M, F> fmt::Debug for LowLevelILBlock<'func, A, M, F>
+impl<'func, A, M, F> Debug for LowLevelILBlock<'func, A, M, F>
 where
-    A: 'func + Architecture,
+    A: 'func + Architecture + Debug,
     M: FunctionMutability,
     F: FunctionForm,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        // TODO: Make this better
-        write!(f, "llil_bb {:?}", self.function)
+        f.debug_struct("LowLevelILBlock")
+            .field("function", &self.function)
+            .finish()
     }
 }
 

rust/src/llil/function.rs

@@ -18,6 +18,7 @@ use binaryninjacore_sys::BNLowLevelILFunction;
 use binaryninjacore_sys::BNNewLowLevelILFunctionReference;
 
 use std::borrow::Borrow;
+use std::fmt::Debug;
 use std::hash::{Hash, Hasher};
 use std::marker::PhantomData;
 
@@ -63,9 +64,9 @@ pub struct LowLevelILFunction<A: Architecture, M: FunctionMutability, F: Functio
     _form: PhantomData<F>,
 }
 
-impl<'func, A, M, F> LowLevelILFunction<A, M, F>
+impl<A, M, F> LowLevelILFunction<A, M, F>
 where
-    A: 'func + Architecture,
+    A: Architecture,
     M: FunctionMutability,
     F: FunctionForm,
 {
@@ -137,9 +138,9 @@ where
 // LLIL basic blocks are not available until the function object
 // is finalized, so ensure we can't try requesting basic blocks
 // during lifting
-impl<'func, A, F> LowLevelILFunction<A, Finalized, F>
+impl<A, F> LowLevelILFunction<A, Finalized, F>
 where
-    A: 'func + Architecture,
+    A: Architecture,
     F: FunctionForm,
 {
     pub fn basic_blocks(&self) -> Array<BasicBlock<LowLevelBlock<A, Finalized, F>>> {
@@ -176,9 +177,9 @@ impl LowLevelILFunction<CoreArchitecture, Mutable, NonSSA<LiftedNonSSA>> {
     }
 }
 
-impl<'func, A, M, F> ToOwned for LowLevelILFunction<A, M, F>
+impl<A, M, F> ToOwned for LowLevelILFunction<A, M, F>
 where
-    A: 'func + Architecture,
+    A: Architecture,
     M: FunctionMutability,
     F: FunctionForm,
 {
@@ -189,9 +190,9 @@ where
     }
 }
 
-unsafe impl<'func, A, M, F> RefCountable for LowLevelILFunction<A, M, F>
+unsafe impl<A, M, F> RefCountable for LowLevelILFunction<A, M, F>
 where
-    A: 'func + Architecture,
+    A: Architecture,
     M: FunctionMutability,
     F: FunctionForm,
 {
@@ -210,15 +211,17 @@ where
     }
 }
 
-impl<'func, A, M, F> fmt::Debug for LowLevelILFunction<A, M, F>
+impl<A, M, F> Debug for LowLevelILFunction<A, M, F>
 where
-    A: 'func + Architecture,
+    A: Architecture + Debug,
     M: FunctionMutability,
     F: FunctionForm,
 {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        // TODO: Make this better
-        write!(f, "<llil func handle {:p}>", self.handle)
+        f.debug_struct("LowLevelILFunction")
+            .field("arch", &self.arch())
+            .field("instruction_count", &self.instruction_count())
+            .finish()
     }
 }
 

rust/src/mlil/instruction.rs

@@ -753,7 +753,7 @@ impl MediumLevelILInstruction {
                         // TODO: Replace with a From<u32> for RegisterValueType.
                         // TODO: We might also want to change the type of `op.constant_data_kind`
                         // TODO: To RegisterValueType and do the conversion when creating instruction.
-                        state: unsafe { std::mem::transmute(op.constant_data_kind) },
+                        state: unsafe { std::mem::transmute::<u32, BNRegisterValueType>(op.constant_data_kind) },
                         value: op.constant_data_value,
                         offset: 0,
                         size: op.size,

rust/src/project.rs

@@ -1103,7 +1103,7 @@ unsafe extern "C" fn cb_progress_func<F: FnMut(usize, usize) -> bool>(
     if ctxt.is_null() {
         return true;
     }
-    let closure: &mut F = mem::transmute(ctxt);
+    let closure = &mut *(ctxt as *mut F);
     closure(progress, total)
 }
 

rust/src/section.rs

@@ -273,7 +273,7 @@ impl<S: BnStrCompatible> SectionBuilder<S> {
         let len = self.range.end.wrapping_sub(start);
 
         unsafe {
-            let nul_str = std::ffi::CStr::from_bytes_with_nul_unchecked(b"\x00").as_ptr();
+            let nul_str = c"".as_ptr();
             let name_ptr = name.as_ref().as_ptr() as *mut _;
             let ty_ptr = ty.map_or(nul_str, |s| s.as_ref().as_ptr() as *mut _);
             let linked_section_ptr =

rust/src/settings.rs

@@ -50,10 +50,6 @@ impl Settings {
         }
     }
 
-    pub fn default() -> Ref<Self> {
-        Self::new("default")
-    }
-
     pub fn set_resource_id<S: BnStrCompatible>(&self, resource_id: S) {
         let resource_id = resource_id.into_bytes_with_nul();
         unsafe { BNSettingsSetResourceId(self.handle, resource_id.as_ref().as_ptr() as *mut _) };

rust/src/typearchive.rs

@@ -376,7 +376,7 @@ impl TypeArchive {
     }
 
     /// Get a list of all types' names and ids in the archive at a current snapshot
-
+    ///
     /// * `snapshot` - Snapshot id to search for types
     pub fn get_type_names_and_ids<S: BnStrCompatible>(
         &self,

rust/src/typecontainer.rs

@@ -351,7 +351,7 @@ impl TypeContainer {
 
     /// Parse an entire block of source into types, variables, and functions, with
     /// knowledge of the types in the Type Container.
-
+    /// 
     /// * `source` - Source code to parse
     /// * `file_name` - Name of the file containing the source (optional: exists on disk)
     /// * `options` - String arguments to pass as options, e.g. command line arguments