Use workspace to set clippy linters

use them in yjit and zjit
fix clippy issues

Author: jesse-shopify

Cargo.toml

@@ -49,3 +49,19 @@ overflow-checks = true
 debug = true
 # Use ThinLTO. Much smaller output for a small amount of build time increase.
 lto = "thin"
+
+[workspace.lints.clippy]
+correctness = { level = "forbid", priority = 1 }
+suspicious = { level = "forbid", priority = 1 }
+perf = { level = "forbid", priority = 1 }
+
+complexity = { level = "deny", priority = -1 }
+
+style = { level = "allow", priority = -1 }
+pedantic = { level = "allow", priority = -1 }
+
+too_many_arguments = "allow"
+identity_op = "allow"
+
+unwrap_or_default = "forbid"
+ptr_arg = "forbid"

yjit/Cargo.toml

@@ -24,3 +24,6 @@ disasm = ["capstone"]
 # from cfg!(debug_assertions) so that we can see disasm of the code
 # that would run in the release mode.
 runtime_checks = []
+
+[lints]
+workspace = true

yjit/src/asm/mod.rs

@@ -224,16 +224,16 @@ impl CodeBlock {
     }
 
     /// Free the memory pages of given code page indexes
-    fn free_pages(&mut self, page_idxs: &Vec<usize>) {
-        let mut page_idxs = page_idxs.clone();
-        page_idxs.reverse(); // to loop with pop()
+    /// In Rust >= 1.77 this could probably be simplified with chunk_by.
+    fn free_pages(&mut self, page_idxs: &[usize]) {
+        let mut page_idxs = page_idxs.iter().rev().collect::<Vec<_>>();
 
         // Group adjacent page indexes and free them in batches to reduce the # of syscalls.
-        while let Some(page_idx) = page_idxs.pop() {
+        while let Some(&page_idx) = page_idxs.pop() {
             // Group first adjacent page indexes
             let mut batch_idxs = vec![page_idx];
-            while page_idxs.last() == Some(&(batch_idxs.last().unwrap() + 1)) {
-                batch_idxs.push(page_idxs.pop().unwrap());
+            while page_idxs.last() == Some(&&(*batch_idxs.last().unwrap() + 1)) {
+                batch_idxs.push(*page_idxs.pop().unwrap());
             }
 
             // Free the grouped pages at once
@@ -441,12 +441,12 @@ impl CodeBlock {
 
         // Ignore empty code ranges
         if start_addr == end_addr {
-            return (0..0).into_iter();
+            return 0..0;
         }
 
         let start_page = (start_addr.raw_addr(self) - mem_start) / self.page_size;
         let end_page = (end_addr.raw_addr(self) - mem_start - 1) / self.page_size;
-        (start_page..end_page + 1).into_iter()
+        start_page..end_page + 1
     }
 
     /// Get a (possibly dangling) direct pointer to the current write position

yjit/src/backend/ir.rs

@@ -186,7 +186,7 @@ impl Opnd
 
     /// Maps the indices from a previous list of instructions to a new list of
     /// instructions.
-    pub fn map_index(self, indices: &Vec<usize>) -> Opnd {
+    pub fn map_index(self, indices: &[usize]) -> Opnd {
         match self {
             Opnd::InsnOut { idx, num_bits } => {
                 Opnd::InsnOut { idx: indices[idx], num_bits }
@@ -1266,11 +1266,11 @@ impl Assembler
 
     /// Spill all live registers to the stack
     pub fn spill_regs(&mut self) {
-        self.spill_regs_except(&vec![]);
+        self.spill_regs_except(&[]);
     }
 
     /// Spill all live registers except `ignored_temps` to the stack
-    pub fn spill_regs_except(&mut self, ignored_temps: &Vec<RegOpnd>) {
+    pub fn spill_regs_except(&mut self, ignored_temps: &[RegOpnd]) {
         // Forget registers above the stack top
         let mut reg_mapping = self.ctx.get_reg_mapping();
         for stack_idx in self.ctx.get_stack_size()..MAX_CTX_TEMPS as u8 {
@@ -1348,17 +1348,17 @@ impl Assembler
 
     // Shuffle register moves, sometimes adding extra moves using SCRATCH_REG,
     // so that they will not rewrite each other before they are used.
-    pub fn reorder_reg_moves(old_moves: &Vec<(Reg, Opnd)>) -> Vec<(Reg, Opnd)> {
+    pub fn reorder_reg_moves(old_moves: &[(Reg, Opnd)]) -> Vec<(Reg, Opnd)> {
         // Return the index of a move whose destination is not used as a source if any.
-        fn find_safe_move(moves: &Vec<(Reg, Opnd)>) -> Option<usize> {
+        fn find_safe_move(moves: &[(Reg, Opnd)]) -> Option<usize> {
             moves.iter().enumerate().find(|(_, &(dest_reg, _))| {
                 moves.iter().all(|&(_, src_opnd)| src_opnd != Opnd::Reg(dest_reg))
             }).map(|(index, _)| index)
         }
 
         // Remove moves whose source and destination are the same
-        let mut old_moves: Vec<(Reg, Opnd)> = old_moves.clone().into_iter()
-            .filter(|&(reg, opnd)| Opnd::Reg(reg) != opnd).collect();
+        let mut old_moves: Vec<(Reg, Opnd)> = old_moves.iter()
+            .filter(|&(reg, opnd)| Opnd::Reg(*reg) != *opnd).copied().collect();
 
         let mut new_moves = vec![];
         while old_moves.len() > 0 {
@@ -1394,7 +1394,7 @@ impl Assembler
 
         // Mutate the pool bitmap to indicate that the register at that index
         // has been allocated and is live.
-        fn alloc_reg(pool: &mut u32, regs: &Vec<Reg>) -> Option<Reg> {
+        fn alloc_reg(pool: &mut u32, regs: &[Reg]) -> Option<Reg> {
             for (index, reg) in regs.iter().enumerate() {
                 if (*pool & (1 << index)) == 0 {
                     *pool |= 1 << index;
@@ -1405,7 +1405,7 @@ impl Assembler
         }
 
         // Allocate a specific register
-        fn take_reg(pool: &mut u32, regs: &Vec<Reg>, reg: &Reg) -> Reg {
+        fn take_reg(pool: &mut u32, regs: &[Reg], reg: &Reg) -> Reg {
             let reg_index = regs.iter().position(|elem| elem.reg_no == reg.reg_no);
 
             if let Some(reg_index) = reg_index {
@@ -1419,7 +1419,7 @@ impl Assembler
         // Mutate the pool bitmap to indicate that the given register is being
         // returned as it is no longer used by the instruction that previously
         // held it.
-        fn dealloc_reg(pool: &mut u32, regs: &Vec<Reg>, reg: &Reg) {
+        fn dealloc_reg(pool: &mut u32, regs: &[Reg], reg: &Reg) {
             let reg_index = regs.iter().position(|elem| elem.reg_no == reg.reg_no);
 
             if let Some(reg_index) = reg_index {
@@ -1602,7 +1602,7 @@ impl Assembler
                 if c_args.len() > 0 {
                     // Resolve C argument dependencies
                     let c_args_len = c_args.len() as isize;
-                    let moves = Self::reorder_reg_moves(&c_args.drain(..).into_iter().collect());
+                    let moves = Self::reorder_reg_moves(&c_args.to_vec());
                     shift_live_ranges(&mut shifted_live_ranges, asm.insns.len(), moves.len() as isize - c_args_len);
 
                     // Push batched C arguments
@@ -1791,7 +1791,7 @@ impl Assembler {
     }
 
     /// Let vm_check_canary() assert the leafness of this ccall if leaf_ccall is set
-    fn set_stack_canary(&mut self, opnds: &Vec<Opnd>) -> Option<Opnd> {
+    fn set_stack_canary(&mut self, opnds: &[Opnd]) -> Option<Opnd> {
         // Use the slot right above the stack top for verifying leafness.
         let canary_opnd = self.stack_opnd(-1);
 

yjit/src/codegen.rs

@@ -43,6 +43,7 @@ type InsnGenFn = fn(
 
 /// Ephemeral code generation state.
 /// Represents a [crate::core::Block] while we build it.
+#[allow(clippy::type_complexity)]
 pub struct JITState<'a> {
     /// Instruction sequence for the compiling block
     pub iseq: IseqPtr,
@@ -1178,14 +1179,14 @@ pub fn gen_entry_reg_mapping(asm: &mut Assembler, blockid: BlockId, stack_size:
     // Find an existing callee block. If it's not found or uses no register, skip loading registers.
     let mut ctx = Context::default();
     ctx.set_stack_size(stack_size);
-    let reg_mapping = find_most_compatible_reg_mapping(blockid, &ctx).unwrap_or(RegMapping::default());
+    let reg_mapping = find_most_compatible_reg_mapping(blockid, &ctx).unwrap_or_default();
     if reg_mapping == RegMapping::default() {
         return reg_mapping;
     }
 
     // If found, load the same registers to reuse the block.
     asm_comment!(asm, "reuse maps: {:?}", reg_mapping);
-    let local_table_size: u32 = unsafe { get_iseq_body_local_table_size(blockid.iseq) }.try_into().unwrap();
+    let local_table_size: u32 = unsafe { get_iseq_body_local_table_size(blockid.iseq) };
     for &reg_opnd in reg_mapping.get_reg_opnds().iter() {
         match reg_opnd {
             RegOpnd::Local(local_idx) => {
@@ -7708,7 +7709,7 @@ fn gen_send_iseq(
         // runtime guards later in copy_splat_args_for_rest_callee()
         if !iseq_has_rest {
             let supplying = argc - 1 - i32::from(kw_splat) + array_length as i32;
-            if (required_num..=required_num + opt_num).contains(&supplying) == false {
+            if !(required_num..=required_num + opt_num).contains(&supplying) {
                 gen_counter_incr(jit, asm, Counter::send_iseq_splat_arity_error);
                 return None;
             }

yjit/src/core.rs

@@ -1099,7 +1099,7 @@ impl Context {
                 MapToLocal(local_idx) => {
                     bits.push_op(CtxOp::MapTempLocal);
                     bits.push_u3(stack_idx as u8);
-                    bits.push_u3(local_idx as u8);
+                    bits.push_u3(local_idx);
                 }
 
                 MapToSelf => {
@@ -1170,19 +1170,19 @@ impl Context {
 
             match op {
                 CtxOp::SetSelfType => {
-                    ctx.self_type = unsafe { transmute(bits.read_u4(&mut idx)) };
+                    ctx.self_type = unsafe { transmute::<u8, Type>(bits.read_u4(&mut idx)) };
                 }
 
                 CtxOp::SetLocalType => {
                     let local_idx = bits.read_u3(&mut idx) as usize;
-                    let t = unsafe { transmute(bits.read_u4(&mut idx)) };
+                    let t = unsafe { transmute::<u8, Type>(bits.read_u4(&mut idx)) };
                     ctx.set_local_type(local_idx, t);
                 }
 
                 // Map temp to stack (known type)
                 CtxOp::SetTempType => {
                     let temp_idx = bits.read_u3(&mut idx) as usize;
-                    let temp_type = unsafe { transmute(bits.read_u4(&mut idx)) };
+                    let temp_type = unsafe { transmute::<u8, Type>(bits.read_u4(&mut idx)) };
                     ctx.set_temp_mapping(temp_idx, TempMapping::MapToStack(temp_type));
                 }
 
@@ -2294,7 +2294,7 @@ pub fn limit_block_versions(blockid: BlockId, ctx: &Context) -> Context {
         let generic_ctx = ctx.get_generic_ctx();
 
         if cfg!(debug_assertions) {
-            let mut ctx = ctx.clone();
+            let mut ctx = *ctx;
             if ctx.inline() {
                 // Suppress TypeDiff::Incompatible from ctx.diff(). We return TypeDiff::Incompatible
                 // to keep inlining blocks until we hit the limit, but it's safe to give up inlining.
@@ -2920,7 +2920,7 @@ impl Context {
         }
 
         // Prepare a Context with the same registers
-        let mut dst_with_same_regs = dst.clone();
+        let mut dst_with_same_regs = *dst;
         dst_with_same_regs.set_reg_mapping(self.get_reg_mapping());
 
         // Diff registers and other stuff separately, and merge them
@@ -3823,7 +3823,7 @@ pub fn gen_branch_stub_hit_trampoline(ocb: &mut OutlinedCb) -> Option<CodePtr> {
 }
 
 /// Return registers to be pushed and popped on branch_stub_hit.
-pub fn caller_saved_temp_regs() -> impl Iterator<Item = &'static Reg> + DoubleEndedIterator {
+pub fn caller_saved_temp_regs() -> impl DoubleEndedIterator<Item = &'static Reg> {
     let temp_regs = Assembler::get_temp_regs().iter();
     let len = temp_regs.len();
     // The return value gen_leave() leaves in C_RET_REG

yjit/src/cruby.rs

@@ -97,7 +97,7 @@ pub type size_t = u64;
 pub type RedefinitionFlag = u32;
 
 #[allow(dead_code)]
-#[allow(clippy::all)]
+#[allow(clippy::complexity)]
 mod autogened {
     use super::*;
     // Textually include output from rust-bindgen as suggested by its user guide.

yjit/src/lib.rs

@@ -1,8 +1,3 @@
-// Clippy disagreements
-#![allow(clippy::style)] // We are laid back about style
-#![allow(clippy::too_many_arguments)] // :shrug:
-#![allow(clippy::identity_op)] // Sometimes we do it for style
-
 // TODO(alan): This lint is right -- the way we use `static mut` is UB happy. We have many globals
 // and take `&mut` frequently, sometimes with a method that easily allows calling it twice.
 //

yjit/src/utils.rs

@@ -92,10 +92,7 @@ pub fn ruby_str_to_rust(v: VALUE) -> String {
     let str_ptr = unsafe { rb_RSTRING_PTR(v) } as *mut u8;
     let str_len: usize = unsafe { rb_RSTRING_LEN(v) }.try_into().unwrap();
     let str_slice: &[u8] = unsafe { slice::from_raw_parts(str_ptr, str_len) };
-    match String::from_utf8(str_slice.to_vec()) {
-        Ok(utf8) => utf8,
-        Err(_) => String::new(),
-    }
+    String::from_utf8(str_slice.to_vec()).unwrap_or_default()
 }
 
 // Location is the file defining the method, colon, method name.

zjit/Cargo.toml

@@ -19,3 +19,6 @@ expect-test = "1.5.1"
 # Support --yjit-dump-disasm and RubyVM::YJIT.disasm using libcapstone.
 disasm = ["capstone"]
 runtime_checks = []
+
+[lints]
+workspace = true