Merge pull request #691 from protofire/fix-gas-incrementBy1-rule

fix rule to support backward expression

Author: dbale-altoros

docs/rules/gas-consumption/gas-increment-by-one.md

@@ -24,7 +24,6 @@ This rule accepts a string option for rule severity. Must be one of "error", "wa
 ```
 
 ### Notes
-- This rule only works for expressions like this: [ j = j + 1 ] but will fail if the code is written like this: [ j = 1 + j ]
 - [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (Incrementing/Decrementing By 1)
 - [source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-8rekj) of the rule initiative
 

lib/rules/gas-consumption/gas-increment-by-one.js

@@ -9,9 +9,6 @@ const meta = {
     description: 'Suggest increments by one, like this ++i instead of other type',
     category: 'Gas Consumption Rules',
     notes: [
-      {
-        note: 'This rule only works for expressions like this: [ j = j + 1 ] but will fail if the code is written like this: [ j = 1 + j ]',
-      },
       {
         note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiative (Incrementing/Decrementing By 1)',
       },
@@ -57,25 +54,43 @@ class GasIncrementByOne extends BaseChecker {
   }
 
   isVariableIncrementOrDecrement(left, right, operator) {
-    const leftVar = this.extractVariableName(left)
-    let rightValue
+  const leftVar = this.extractVariableName(left)
 
-    // assignment and operation
-    if (operator === '+=' || operator === '-=') {
-      rightValue = right.type === 'NumberLiteral' && parseInt(right.number) === 1
-      return { result: leftVar && rightValue, varName: leftVar }
-    }
-    // regular assignment
-    else if (operator === '=') {
-      const rightVar = this.extractVariableName(right.left)
-      rightValue =
-        right.right && right.right.type === 'NumberLiteral' && parseInt(right.right.number) === 1
-      return { result: leftVar === rightVar && rightValue, varName: leftVar }
-    }
+  // assignment and operation
+  if (operator === '+=' || operator === '-=') {
+    const rightIsOne = right.type === 'NumberLiteral' && parseInt(right.number) === 1
+    return { result: leftVar && rightIsOne, varName: leftVar }
+  }
+
+  // regular assignment
+  if (operator === '=') {
+    if (right.type !== 'BinaryOperation') return { result: false }
+
+    const rightLeftVar = this.extractVariableName(right.left)
+    const rightRightVar = this.extractVariableName(right.right)
 
-    return false
+    const isLeftOne = right.left.type === 'NumberLiteral' && parseInt(right.left.number) === 1
+    const isRightOne = right.right.type === 'NumberLiteral' && parseInt(right.right.number) === 1
+
+    const isIncrement =
+      right.operator === '+' &&
+      (
+        (leftVar === rightLeftVar && isRightOne) || 
+        (leftVar === rightRightVar && isLeftOne)
+      )
+
+    const isDecrement =
+      right.operator === '-' &&
+      leftVar === rightLeftVar &&
+      isRightOne
+
+    return { result: isIncrement || isDecrement, varName: leftVar }
   }
 
+  return { result: false }
+}
+
+
   extractVariableName(node) {
     if (node.type === 'Identifier') {
       return node.name
@@ -96,53 +111,4 @@ class GasIncrementByOne extends BaseChecker {
   }
 }
 
-module.exports = GasIncrementByOne
-
-// SourceUnit(node) {
-//   this.findVariableUpdates(node)
-// }
-
-// findVariableUpdates(node) {
-//   if (node === null || typeof node !== 'object') {
-//     return
-//   }
-
-//   // Check for assignment in a function body
-//   if (node.type === 'FunctionDefinition') {
-//     this.findVariableUpdates(node.body)
-//     return
-//   }
-
-//   if (node.type === 'ExpressionStatement' && node.expression.type === 'BinaryOperation') {
-//     const expr = node.expression
-//     if (expr.operator === '=' || expr.operator === '+=' || expr.operator === '-=') {
-//       if (this.isVariableIncrementOrDecrement(expr.left, expr.right, expr.operator)) {
-//         this.reportError(node, this.extractVariableName(expr.left))
-//       }
-//     }
-//   }
-
-//   Object.values(node).forEach((value) => {
-//     if (typeof value === 'object') {
-//       this.findVariableUpdates(value)
-//     }
-//   })
-// }
-
-// isVariableIncrementOrDecrement(left, right, operator) {
-//   const leftVar = this.extractVariableName(left)
-//   let rightVar, rightValue
-
-//   if (operator === '+=' || operator === '-=') {
-//     // For compound assignment, the operation is directly on the variable
-//     rightValue = right.type === 'NumberLiteral' && parseInt(right.number) === 1
-//     return leftVar && rightValue
-//   } else if (operator === '=') {
-//     rightVar = this.extractVariableName(right.left)
-//     rightValue =
-//       right.right && right.right.type === 'NumberLiteral' && parseInt(right.right.number) === 1
-//     return leftVar === rightVar && rightValue
-//   }
-
-//   return false
-// }
+module.exports = GasIncrementByOne

test/rules/gas-consumption/gas-increment-by-one.js

@@ -40,15 +40,19 @@ const RAISE_ERRORS = [
     statement: 'anotherVar -= 1;',
     varName: 'anotherVar',
   },
-  // {
-  //   statement:
-  //     'complexMapping[user].balances[asset] = complexMapping[user].balances[asset] + 1 + someFunctionCall();',
-  //   varName: 'complexMapping',
-  // },
-  // {
-  //   statement: 'anotherMapping[user][token].count = anotherMapping[user][token].count - 1 + 5;',
-  //   varName: 'anotherMapping',
-  // },
+  {
+    statement: 'complexMapping[user].balances[asset] = complexMapping[user].balances[asset] + 1;',
+    varName: 'complexMapping',
+  },
+  {
+    statement: 'amount = 1 + amount;',
+    varName: 'amount',
+  },
+  {
+    statement:
+      'complexMappingBackward[user].balances[asset] = 1 + complexMappingBackward[user].balances[asset];',
+    varName: 'complexMappingBackward',
+  },
 ]
 
 const NOT_RAISE_ERRORS = [