gc: struct packing

docs/rules/gas-consumption/gas-struct-packing.md

@@ -24,7 +24,7 @@ This rule accepts a string option of rule severity. Must be one of "error", "war
 ```
 
 ### Notes
-- This rule 
+- This rule assumes all UserDefinedTypeName take a new slot. (beware of Enums inside Structs) 
 - [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (Variable Packing)
 - [source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-f8m1r) of the rule initiative
 
@@ -38,4 +38,3 @@ This rule is introduced in the latest version.
 - [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/gas-consumption/gas-struct-packing.js)
 - [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-struct-packing.md)
 - [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-struct-packing.js)
-s)

lib/rules/gas-consumption/gas-struct-packing.js

@@ -10,7 +10,7 @@ const meta = {
     category: 'Gas Consumption Rules',
     notes: [
       {
-        note: 'This rule ',
+        note: 'This rule assumes all UserDefinedTypeName take a new slot. (beware of Enums inside Structs) ',
       },
       {
         note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (Variable Packing)',
@@ -34,133 +34,97 @@ class GasStructPacking extends BaseChecker {
   }
 
   StructDefinition(node) {
-    if (this.isInefficientlyPacked(node)) {
+    const reportError = this.isInefficientlyPacked(node)
+    if (reportError) {
       this.reportError(node)
     }
   }
 
   isInefficientlyPacked(structNode) {
     if (!structNode || !Array.isArray(structNode.members) || structNode.members.length <= 1) {
+    // if (!structNode || !Array.isArray(structNode.members)) {
       return false // Early return for structs with 1 or no members
     }
 
     let members = structNode.members.map((member) => ({
       name: member.name,
       size: this.getVariableSize(member.typeName),
       type: member.typeName.type,
-      sum: this.canBeAdded(member.typeName),
     }))
 
-    console.log('members :>> ', members)
-
-    return
-/*
-    let members = structNode.members.map((member) => ({
-      name: member.name,
-      size: this.getVariableSize(member.typeName),
-      type: member.typeName.type,
-      newSlot: this.isNewSlot(member.typeName),
-    }))
-
-    let currentSlotSize = 0
-    let overflow = false
-
-    console.log('members :>> ', members)
-
-    // Iterate over struct members to assess packing efficiency
-    for (const member of members) {
-      // Special handling for large values that occupy an entire slot or more
-      if (member.size == 32) {
-        // If the current slot has been partially used, this large value starts a new slot
-        if (currentSlotSize > 0) {
-          currentSlotSize = 0 // Reset for the next slot
-          overflow = true // Mark that we've overflowed into a new slot
-        }
-        // No need to add size to currentSlotSize since it occupies the whole new slot
-        continue
-      }
-
-      if (currentSlotSize + member.size > 32) {
-        // If adding this member exceeds the slot size, move to the next slot
-        overflow = true // Mark that we've overflowed
-        currentSlotSize = member.size // This member starts the new slot
-      } else {
-        // Accumulate member size in the current slot
-        currentSlotSize += member.size
-      }
-    }
-
-    // If we've never overflowed, all members are efficiently packed
-    return overflow
-    */
+    const canBeImproved = this.analyzePacking(members)
+    return canBeImproved
   }
 
-  /*
-  isInefficientlyPacked(structNode) {
-    if (!structNode || !Array.isArray(structNode.members) || structNode.members.length <= 1) {
-      return false
-    }
-
-    let members = structNode.members.map((member) => ({
-      name: member.name,
-      size: this.getVariableSize(member.typeName),
-      type: member.typeName.type,
-    }))
-
-    console.log('members :>> ', members)
+  // Function to calculate the optimal slots needed for given members
+  calculateOptimalSlots(arr) {
+    let totalSize = 0
+    arr.forEach(({ size }) => {
+      totalSize += size
+    })
+    return Math.ceil(totalSize / 32)
+  }
 
-    let currentSlotSize = 0;
-    let isPotentialInefficiencyDetected = false;
+  calculateCurrentSlotsUsed(members) {
+    let slotsUsed = 0;
+    let currentSlotSpace = 0;
 
-    // Iterate over struct members to assess packing efficiency
     members.forEach(member => {
-      // If adding this member exceeds the current slot size, it starts in a new slot
-      if (currentSlotSize + member.size > 32) {
-        // If there was room left in the slot, it's a potential inefficiency
-        if (currentSlotSize < 32) {
-          isPotentialInefficiencyDetected = true;
+      if (member.size === 32) {
+        if (currentSlotSpace > 0) {
+          slotsUsed += 1; // Finish the current slot if it was partially filled
+          currentSlotSpace = 0;
         }
-        currentSlotSize = member.size % 32;
+        slotsUsed += 1; // This member occupies a full slot
       } else {
-        currentSlotSize += member.size;
-        // Adjust current slot size to modulo 32 for cases where exact fit or overflow
-        currentSlotSize %= 32;
+        if (currentSlotSpace + member.size > 32) {
+          slotsUsed += 1; // Move to the next slot if adding this member exceeds the current slot
+          currentSlotSpace = member.size; // Start filling the next slot
+        } else {
+          currentSlotSpace += member.size; // Add to the current slot space
+        }
       }
     });
 
-    // If there's unused space in the last slot, it might not indicate inefficiency by itself
-    // unless combined with earlier detected potential inefficiency.
-    if (currentSlotSize > 0 && currentSlotSize < 32 && isPotentialInefficiencyDetected) {
-      isPotentialInefficiencyDetected = true;
-    } else if (currentSlotSize === 0 && !isPotentialInefficiencyDetected) {
-      // If we perfectly fill slots without earlier inefficiency, it's efficient
-      isPotentialInefficiencyDetected = false;
+    // If there's any space used in the current slot after looping, it means another slot is partially filled
+    if (currentSlotSpace > 0) {
+      slotsUsed += 1;
     }
 
-    return isPotentialInefficiencyDetected;
-
+    return slotsUsed;
   }
-  */
-
-  isSlotFull(member) {
-    console.log('member :>> ', member)
-    if (!member) {
-      return false
-    }
 
-    switch (member.type) {
-      case 'Mapping':
-        return true
+
+  analyzePacking(members) {
+    // Separate members into large and small for analysis
+    // const largeMembers = members.filter((member) => member.size === 32)
+    const smallMembers = members.filter((member) => member.size < 32)
+
+    // Sort small members by size, descending, to optimize packing
+    smallMembers.sort((a, b) => b.size - a.size)
+
+    // Initial slots count: one slot per large member
+    let currentSlots = this.calculateCurrentSlotsUsed(members)
+
+    // Track remaining space in the current slot
+    let remainingSpace = 32
+    smallMembers.forEach((member) => {
+      if (member.size <= remainingSpace) {
+        // If the member fits in the current slot, subtract its size from the remaining space
+        remainingSpace -= member.size
+      } else {
+        // If not, start a new slot and adjust remaining space
+        remainingSpace = 32 - member.size
+      }
+    })
 
-      case 'ArrayTypeName':
-        return member.size === 32
+    // Calculate optimal slots needed if perfectly packed
+    const optimalSlots = this.calculateOptimalSlots(members)
 
-      case 'ElementaryTypeName':
-        return member.name.startsWith('string') || member.name.startsWith('bytes')
+    // console.log(`\n\nCurrent slot usage: ${currentSlots}`)
+    // console.log(`Optimal (minimum possible) slot usage: ${optimalSlots}`)
 
-      default:
-        return false
-    }
+    return currentSlots > optimalSlots
   }
 
   getVariableSize(typeNode) {
@@ -172,14 +136,17 @@ class GasStructPacking extends BaseChecker {
       case 'ElementaryTypeName':
         return this.getSizeForElementaryType(typeNode.name)
       case 'UserDefinedTypeName':
+        // this can be improved for enums
         return 32
       case 'ArrayTypeName':
         if (typeNode.length) {
           // if array is fixed, get the length * var size
           const varSize = this.getSizeForElementaryType(typeNode.baseTypeName.name)
-          return parseInt(typeNode.length.number) * varSize
+          const size = parseInt(typeNode.length.number) * varSize
+          return size > 32 ? 32 : size
         }
         return 32 // Dynamic arrays occupy a full slot
+
       default:
         return 32
     }
@@ -206,30 +173,10 @@ class GasStructPacking extends BaseChecker {
     return Math.ceil(bits / 8)
   }
 
-  canBeAdded(typeName) {
-    console.log('typeName :>> ', typeName);
-
-    if (typeName.name.includes('uint') || typeName.name.includes('int')) {
-      const aa = this.getSizeForIntType(typeName.name)
-      console.log('aa :>> ', aa);
-
-      return !(this.getSizeForIntType(typeName.name) >= 32)
-    }
-    console.log("20");
-
-    switch (typeName.name) {
-      case 'address':
-      case 'bool':
-        return true
-      default:
-        return false
-    }
-  }
-
   reportError(node) {
     this.error(
       node,
-      `GC: For [ ${node.name} ] struct, packing seems inefficient. Try rearranging variables to achieve 32bytes slots`
+      `GC: For [ ${node.name} ] struct, packing seems inefficient. Try rearranging to achieve 32bytes slots`
     )
   }
 }