Merge pull request #544 from protofire/gc-calldata-parameters

GC: calldata parameters

conf/rulesets/solhint-all.js

@@ -25,6 +25,7 @@ module.exports = Object.freeze({
       },
     ],
     'constructor-syntax': 'warn',
+    'gas-calldata-parameters': 'warn',
     'gas-indexed-events': 'warn',
     'gas-multitoken1155': 'warn',
     'gas-small-strings': 'warn',

docs/rules.md

@@ -27,11 +27,12 @@ title:       "Rule Index of Solhint"
 
 ## Gas Consumption Rules
 
-| Rule Id                                                             | Error                                                          | Recommended | Deprecated |
-| ------------------------------------------------------------------- | -------------------------------------------------------------- | ----------- | ---------- |
-| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md) | Suggest indexed arguments on events for uint, bool and address |             |            |
-| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md) | ERC1155 is a cheaper non-fungible token than ERC721            |             |            |
-| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md)   | Keep strings smaller than 32 bytes                             |             |            |
+| Rule Id                                                                       | Error                                                          | Recommended | Deprecated |
+| ----------------------------------------------------------------------------- | -------------------------------------------------------------- | ----------- | ---------- |
+| [gas-calldata-parameters](./rules/gas-consumption/gas-calldata-parameters.md) | Suggest calldata keyword on function arguments when read only  |             |            |
+| [gas-indexed-events](./rules/gas-consumption/gas-indexed-events.md)           | Suggest indexed arguments on events for uint, bool and address |             |            |
+| [gas-multitoken1155](./rules/gas-consumption/gas-multitoken1155.md)           | ERC1155 is a cheaper non-fungible token than ERC721            |             |            |
+| [gas-small-strings](./rules/gas-consumption/gas-small-strings.md)             | Keep strings smaller than 32 bytes                             |             |            |
         
 
 ## Miscellaneous

docs/rules/gas-consumption/gas-calldata-parameters.md

@@ -0,0 +1,41 @@
+---
+warning:     "This is a dynamically generated file. Do not edit manually."
+layout:      "default"
+title:       "gas-calldata-parameters | Solhint"
+---
+
+# gas-calldata-parameters
+![Category Badge](https://img.shields.io/badge/-Gas%20Consumption%20Rules-informational)
+![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
+
+## Description
+Suggest calldata keyword on function arguments when read only
+
+## Options
+This rule accepts a string option of rule severity. Must be one of "error", "warn", "off". Default to warn.
+
+### Example Config
+```json
+{
+  "rules": {
+    "gas-calldata-parameters": "warn"
+  }
+}
+```
+
+### Notes
+- Only applies for external functions when receiving arguments with [memory] keyword
+- This rule makes a soft check to see if argument is readOnly to make the suggestion. Check it manually before changing it.
+- [source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiciative (see Calldata vs Memory)
+- [source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-6acr7) of the rule initiciative
+
+## Examples
+This rule does not have examples.
+
+## Version
+This rule is introduced in the latest version.
+
+## Resources
+- [Rule source](https://github.com/protofire/solhint/tree/master/lib/rules/gas-consumption/gas-calldata-parameters.js)
+- [Document source](https://github.com/protofire/solhint/tree/master/docs/rules/gas-consumption/gas-calldata-parameters.md)
+- [Test cases](https://github.com/protofire/solhint/tree/master/test/rules/gas-consumption/gas-calldata-parameters.js)

lib/rules/gas-consumption/gas-calldata-parameters.js

@@ -0,0 +1,121 @@
+const BaseChecker = require('../base-checker')
+
+const solidityOperators = [
+  '=',
+  '++',
+  '--',
+  '+=',
+  '-=',
+  '*=',
+  '/=',
+  '%=',
+  '&=',
+  '|=',
+  '^=',
+  '<<=',
+  '>>=',
+]
+const ruleId = 'gas-calldata-parameters'
+const meta = {
+  type: 'gas-consumption',
+
+  docs: {
+    description: 'Suggest calldata keyword on function arguments when read only',
+    category: 'Gas Consumption Rules',
+    notes: [
+      {
+        note: 'Only applies for external functions when receiving arguments with [memory] keyword',
+      },
+      {
+        note: 'This rule makes a soft check to see if argument is readOnly to make the suggestion. Check it manually before changing it.',
+      },
+      {
+        note: '[source 1](https://coinsbench.com/comprehensive-guide-tips-and-tricks-for-gas-optimization-in-solidity-5380db734404) of the rule initiciative (see Calldata vs Memory)',
+      },
+      {
+        note: '[source 2](https://www.rareskills.io/post/gas-optimization?postId=c9db474a-ff97-4fa3-a51d-fe13ccb8fe3b#viewer-6acr7) of the rule initiciative',
+      },
+    ],
+  },
+
+  isDefault: false,
+  recommended: false,
+  defaultSetup: 'warn',
+
+  schema: null,
+}
+
+class GasCalldataParameters extends BaseChecker {
+  constructor(reporter) {
+    super(reporter, ruleId, meta)
+  }
+
+  // checkIfReadOnly(statements, functionArguments) {
+  checkIfReadOnly(node, index) {
+    let isUpdated = false
+    let i = 0
+    const statements = node.body.statements
+
+    while (i < statements.length) {
+      if (statements[i].type === 'ExpressionStatement') {
+        // variable manipulation
+        if (solidityOperators.includes(statements[i].expression.operator)) {
+          // binary operation
+          if (statements[i].expression.type === 'BinaryOperation') {
+            // array
+            if (statements[i].expression.left.base && statements[i].expression.left.base.name) {
+              isUpdated = statements[i].expression.left.base.name === node.parameters[index].name
+            }
+            // regular expression
+            else if (statements[i].expression.left.name) {
+              isUpdated = statements[i].expression.left.name === node.parameters[index].name
+            }
+            // struct
+            else if (statements[i].expression.left.type === 'MemberAccess') {
+              isUpdated =
+                statements[i].expression.left.expression.name === node.parameters[index].name
+            }
+          }
+          // unary operations
+          else if (statements[i].expression.type === 'UnaryOperation') {
+            isUpdated =
+              // regular unary expression
+              statements[i].expression.subExpression.name === node.parameters[index].name ||
+              // on array expression
+              statements[i].expression.subExpression.base.name === node.parameters[index].name
+          }
+        }
+      }
+      // if is updated stop looping
+      if (isUpdated) i = statements.length
+      // if not, keep looping statements
+      else i++
+    }
+
+    return isUpdated
+  }
+
+  FunctionDefinition(node) {
+    const qtyArguments = node.parameters.length
+    let isUpdated = false
+
+    // check for external functions the memory keyword
+    if (node.visibility === 'external' && qtyArguments > 0) {
+      for (let i = 0; i < qtyArguments; i++) {
+        if (node.parameters[i].storageLocation === 'memory') {
+          isUpdated = this.checkIfReadOnly(node, i)
+          if (!isUpdated) this.reportError(node, node.parameters[i].name)
+        }
+      }
+    }
+  }
+
+  reportError(node, parameterName) {
+    this.error(
+      node,
+      `GC: [${parameterName}] argument on Function [${node.name}] could be [calldata] if it's not being updated`
+    )
+  }
+}
+
+module.exports = GasCalldataParameters

lib/rules/gas-consumption/index.js

@@ -1,12 +1,14 @@
 const GasMultitoken1155 = require('./gas-multitoken1155')
 const GasSmallStrings = require('./gas-small-strings')
 const GasIndexedEvents = require('./gas-indexed-events')
+const GasCalldataParameters = require('./gas-calldata-parameters')
 
 // module.exports = function checkers(reporter, config, tokens) {
 module.exports = function checkers(reporter, config) {
   return [
     new GasMultitoken1155(reporter, config),
     new GasSmallStrings(reporter, config),
     new GasIndexedEvents(reporter, config),
+    new GasCalldataParameters(reporter, config),
   ]
 }

test/rules/gas-consumption/gas-calldata-parameters.js

@@ -0,0 +1,155 @@
+const assert = require('assert')
+const linter = require('../../../lib/index')
+const { contractWith } = require('../../common/contract-builder')
+
+const replaceErrorMsg = (parameterName, functionName) => {
+  const errMsg = `GC: [${parameterName}] argument on Function [${functionName}] could be [calldata] if it's not being updated`
+  return errMsg
+}
+
+describe('Linter - gas-calldata-parameters', () => {
+  it('should raise error on strVar', () => {
+    const code = contractWith(`
+      function function1(
+          uint256[] memory arrayUint,
+          bool active,
+          string memory strVar
+      ) external pure {
+          active = true;
+          arrayUint[0] = 1;          
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.equal(report.messages[0].message, replaceErrorMsg('strVar', 'function1'))
+  })
+
+  it('should raise error on strVar and arrayUint', () => {
+    const code = contractWith(`
+      function function1(
+          uint256[] memory arrayUint,
+          bool active,
+          string memory strVar
+      ) external pure {
+          active = true;          
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 2)
+    assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'function1'))
+    assert.equal(report.messages[1].message, replaceErrorMsg('strVar', 'function1'))
+  })
+
+  it('should raise error on arrayUint', () => {
+    const code = contractWith(`
+      function function1(
+          uint256[] memory arrayUint,
+          bool active,
+          CustomStruct memory customStruct
+      ) external pure {
+        customStruct.field1 = true;          
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'function1'))
+  })
+
+  it('should raise error on arrayUint, customStruct and strVar', () => {
+    const code = contractWith(`
+      function functionTest1(
+          uint256[] memory arrayUint,
+          bool active,
+          CustomStruct memory customStruct,
+          string memory strVar
+      ) external pure {
+        active = true;          
+        string strVar2 = strVar;
+        active = customStruct.active;
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 3)
+    assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'functionTest1'))
+    assert.equal(report.messages[1].message, replaceErrorMsg('customStruct', 'functionTest1'))
+    assert.equal(report.messages[2].message, replaceErrorMsg('strVar', 'functionTest1'))
+  })
+
+  it('should raise error on customStruct and strVar', () => {
+    const code = contractWith(`
+      function functionTest1(
+          uint256[] memory arrayUint,
+          bool active,
+          CustomStruct memory customStruct,
+          string memory strVar
+      ) external pure {
+        arrayUint[i]++;
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 2)
+    assert.equal(report.messages[0].message, replaceErrorMsg('customStruct', 'functionTest1'))
+    assert.equal(report.messages[1].message, replaceErrorMsg('strVar', 'functionTest1'))
+  })
+
+  it('should raise error on arrayUint', () => {
+    const code = contractWith(`
+      function functionTest1(
+          uint256[] memory arrayUint,
+          bool active,
+          CustomStruct memory customStruct,
+          string memory strVar
+      ) external pure {
+        customStruct.field1 = "something";
+        strVar <<= active;
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.equal(report.messages[0].message, replaceErrorMsg('arrayUint', 'functionTest1'))
+  })
+
+  it('should NOT raise error', () => {
+    const code = contractWith(`
+      function function1(
+          uint256[] memory arrayUint,
+          bool active,
+          string memory strVar
+      ) external pure {
+          active = true;          
+          arrayUint[0] = 1;
+          strVar = '';
+      }      
+      `)
+
+    const report = linter.processStr(code, {
+      rules: { 'gas-calldata-parameters': 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
+})