improved test cases and implementation

protofire · Feb 7, 2023 · 92d3b79 · 92d3b79
1 parent 1292516
commit 92d3b79
Show file tree

Hide file tree

Showing 5 changed files with 120 additions and 219 deletions.
diff --git a/docs/rules.md b/docs/rules.md
@@ -59,7 +59,7 @@ title:       "Rule Index of Solhint"
 | [avoid-tx-origin](./rules/security/avoid-tx-origin.md)                 | Avoid to use tx.origin.                                                  | ✔️          |
 | [check-send-result](./rules/security/check-send-result.md)             | Check result of "send" call.                                             | ✔️          |
 | [compiler-version](./rules/security/compiler-version.md)               | Compiler version must satisfy a semver requirement.                      | ✔️          |
-| [func-visibility](./rules/security/func-visibility.md)                 | Explicitly mark visibility in function (free functions are skipped).     | ✔️          |
+| [func-visibility](./rules/security/func-visibility.md)                 | Explicitly mark visibility in function.                                  | ✔️          |
 | [mark-callable-contracts](./rules/security/mark-callable-contracts.md) | Explicitly mark all external contracts as trusted or untrusted.          |             |
 | [multiple-sends](./rules/security/multiple-sends.md)                   | Avoid multiple calls of "send" method in single transaction.             | ✔️          |
 | [no-complex-fallback](./rules/security/no-complex-fallback.md)         | Fallback function must be simple.                                        | ✔️          |

diff --git a/docs/rules/security/func-visibility.md b/docs/rules/security/func-visibility.md
@@ -12,7 +12,7 @@ title:       "func-visibility | Solhint"
 
 
 ## Description
-Explicitly mark visibility in function (free functions are skipped).
+Explicitly mark visibility in function.
 
 ## Options
 This rule accepts an array of options:

diff --git a/lib/rules/security/func-visibility.js b/lib/rules/security/func-visibility.js
@@ -10,7 +10,7 @@ const meta = {
   type: 'security',
 
   docs: {
-    description: `Explicitly mark visibility in function (free functions are skipped).`,
+    description: `Explicitly mark visibility in function.`,
     category: 'Security Rules',
     options: [
       {
@@ -53,8 +53,6 @@ const meta = {
   },
 }
 
-let contractsInfo
-
 class FuncVisibilityChecker extends BaseChecker {
   constructor(reporter, config) {
     super(reporter, ruleId, meta)
@@ -63,43 +61,27 @@ class FuncVisibilityChecker extends BaseChecker {
       config && config.getObjectPropertyBoolean(ruleId, 'ignoreConstructors', false)
   }
 
-  SourceUnit(node) {
-    if (!node.children) return
-
-    contractsInfo = []
+  ContractDefinition(node) {
+    this.isContract = node.kind === 'contract'
+  }
 
-    let contractInfo = {}
-    for (const child of node.children) {
-      if (child.type === 'ContractDefinition' && child.kind === 'contract') {
-        contractInfo = {
-          name: child.name,
-          start: child.loc.start.line,
-          end: child.loc.end.line,
-        }
-        contractsInfo.push(contractInfo)
-      }
-    }
+  'ContractDefinition:exit'() {
+    this.isContract = false
   }
 
   FunctionDefinition(node) {
     if (node.isConstructor && this.ignoreConstructors) {
       return
     }
 
-    const functionStart = node.loc.start.line
-    const functionEnd = node.loc.end.line
-
-    for (const contract of contractsInfo) {
-      // this checks if the function is inside any contract, if not is a free function
-      if (functionStart > contract.start && functionEnd < contract.end) {
-        if (node.visibility === 'default') {
-          this.warn(
-            node,
-            node.isConstructor
-              ? 'Explicitly mark visibility in function (Set ignoreConstructors to true if using solidity >=0.7.0)'
-              : 'Explicitly mark visibility in function'
-          )
-        }
+    if (this.isContract) {
+      if (node.visibility === 'default') {
+        this.warn(
+          node,
+          node.isConstructor
+            ? 'Explicitly mark visibility in function (Set ignoreConstructors to true if using solidity >=0.7.0)'
+            : 'Explicitly mark visibility in function'
+        )
       }
     }
   }

diff --git a/test/fixtures/security/contracts-with-free-functions.js b/test/fixtures/security/contracts-with-free-functions.js
@@ -1,176 +1,69 @@
-// all contracts are made with two func-visibility errors
+const CONTRACTS_FREE_FUNCTIONS_ERRORS_2 = `
+function freeAa() returns(bool) {
+    return true;
+}
+contract A {
+    // error here
+    function functionPublicViewA() returns (uint256) {
+        return 1;
+    }
+}
 
-module.exports = [
-  `
-      // SPDX-License-Identifier: MIT
-      pragma solidity 0.4.4;
-      import "hardhat/console.sol";
-  
-      interface fufu {
-          function intFunction() returns(bool);
-          function intFunction() returns(uint256);
-      }
-  
-      function freeAa() returns(bool) {
-          return true;
-      }
-  
-      contract A {
-          uint256 public a;
-  
-          constructor() {
-              a = 2;
-          }
-  
-          function functionPublicViewA() returns (uint256) {
-              if (freeAa()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicPureA() public pure returns (bool) {
-              return freeAa();
-          }
-      }
-  
-      function freeBb() returns(bool) {
-          return true;
-      }
-  
-      contract B {
-          uint256 public a;
-  
-          constructor() {
-              a = 2;
-          }
-  
-          function functionPublicViewB() returns (uint256) {
-              if (freeBb()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicPureB() public pure returns (bool) {
-              return freeCc();
-          }
-      }
-  
-      function freeCc() returns(bool) {
-          return true;
-      }
-      `,
-  `
-      // SPDX-License-Identifier: MIT
-      pragma solidity 0.8.4;
-      import "hardhat/console.sol";
-  
-      function freeAa() returns(bool) {
-          return true;
-      }
-  
-      function freeBb() returns(bool) {
-          return true;
-      }
-  
-      interface fufu {
-          function intFunction() returns(bool);
-          function intFunction() returns(uint256);
-      }
-  
-      contract A {
-          uint256 public a;
-  
-          constructor() {
-              a = 2;
-          }
-  
-          function functionPublicViewA() returns (uint256) {
-              if (freeAa()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicPureA() public pure returns (bool) {
-              return freeBb();
-          }
-      }
-  
-      function freeCc() returns(bool) {
-          return true;
-      }
-  
-      contract B {
-          uint256 public a;
-  
-          constructor() {
-              a = 2;
-          }
-  
-          function functionPublicViewB() returns (uint256) {
-              if (freeAa()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicPureB() public pure returns (bool) {
-              return freeCc();
-          }
-      }
-      `,
-  `
-      // SPDX-License-Identifier: MIT
-      pragma solidity 0.8.4;
-  
-      function freeCc() returns(bool) {
-          return true;
-      }
-  
-      contract A {
-          uint256 public a;
-  
-          constructor() {
-              a = 2;
-          }
-          
-          function functionExternal() returns (uint256) {
-              if (freeCc()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicViewA() returns (uint256) {
-              if (freeCc()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicPureA() public pure returns (bool) {
-              return freeCc();
-          }
-      }
-      `,
-  `
-      // SPDX-License-Identifier: MIT
-      pragma solidity 0.8.4;
-  
-      contract A {
-          uint256 public a;
-  
-          constructor() {
-              a = 2;
-          }
-          
-          function functionExternal() returns (uint256) {
-              if (freeCc()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicViewA() returns (uint256) {
-              if (freeCc()) return a;
-              else return a+1;
-          }
-  
-          function functionPublicPureA() public pure returns (bool) {
-              return freeCc();
-          }
-      }
-  
-      function freeCc() returns(bool) {
-          return true;
-      }
-      `,
-]
+function freeBb() returns(bool) {
+    return true;
+}
+contract B {
+    // error here
+    function functionPublicViewA() returns (uint256) {
+        return 1;
+    }
+
+    function functionPublicPureB() internal pure returns (bool) {
+        return true;
+    }
+}
+`
+
+const CONTRACT_FREE_FUNCTIONS_ERRORS_1 = `
+contract A {
+    constructor() {}
+
+    // error here
+    function functionPublicViewA() returns (uint256) {
+        return 1;
+    }
+}
+
+function freeBb() returns(bool) {
+    return true;
+}
+`
+
+const NOCONTRACT_FREE_FUNCTION_ERRORS_0 = `
+    // NO error here
+    function functionPublicViewA() returns (uint256) {
+        return 1;
+    }
+`
+
+const CONTRACT_FREE_FUNCTIONS_ERRORS_0 = `
+function freeBb() returns(bool) {
+    return true;
+}
+
+contract A {
+    constructor() {}
+
+    // NO error here
+    function functionPublicViewA() external returns (uint256) {
+        return 1;
+    }
+}
+`
+
+module.exports = {
+  CONTRACTS_FREE_FUNCTIONS_ERRORS_2,
+  CONTRACT_FREE_FUNCTIONS_ERRORS_1,
+  NOCONTRACT_FREE_FUNCTION_ERRORS_0,
+  CONTRACT_FREE_FUNCTIONS_ERRORS_0,
+}
diff --git a/test/rules/security/func-visibility.js b/test/rules/security/func-visibility.js
@@ -1,21 +1,47 @@
 const assert = require('assert')
 const linter = require('../../../lib/index')
 const contractWith = require('../../common/contract-builder').contractWith
+const CONTRACTS = require('../../fixtures/security/contracts-with-free-functions')
 
 describe('Linter - func-visibility with free functions', () => {
-  const contractWithFreeFunction = require('../../fixtures/security/contracts-with-free-functions')
+  it('should return two warnings and skip free functions', () => {
+    const code = CONTRACTS.CONTRACTS_FREE_FUNCTIONS_ERRORS_2
+    const report = linter.processStr(code, {
+      rules: { 'func-visibility': ['warn', { ignoreConstructors: true }] },
+    })
 
-  contractWithFreeFunction.forEach((code) =>
-    it('should return required visibility error skiping free functions', () => {
-      const report = linter.processStr(code, {
-        rules: { 'func-visibility': ['warn', { ignoreConstructors: true }] },
-      })
+    assert.equal(report.warningCount, 2)
+    assert.ok(report.reports[0].message.includes('visibility'))
+    assert.ok(report.reports[1].message.includes('visibility'))
+  })
 
-      assert.equal(report.warningCount, 2)
-      assert.ok(report.reports[0].message.includes('visibility'))
-      assert.ok(report.reports[1].message.includes('visibility'))
+  it('should return one warning and skip free functions', () => {
+    const code = CONTRACTS.CONTRACT_FREE_FUNCTIONS_ERRORS_1
+    const report = linter.processStr(code, {
+      rules: { 'func-visibility': ['warn', { ignoreConstructors: true }] },
     })
-  )
+
+    assert.equal(report.warningCount, 1)
+    assert.ok(report.reports[0].message.includes('visibility'))
+  })
+
+  it('should not return any warning for a free function only', () => {
+    const code = CONTRACTS.NOCONTRACT_FREE_FUNCTION_ERRORS_0
+    const report = linter.processStr(code, {
+      rules: { 'func-visibility': ['warn', { ignoreConstructors: true }] },
+    })
+
+    assert.equal(report.warningCount, 0)
+  })
+
+  it('should not return any warning for a correct contract with a free function', () => {
+    const code = CONTRACTS.CONTRACT_FREE_FUNCTIONS_ERRORS_0
+    const report = linter.processStr(code, {
+      rules: { 'func-visibility': ['warn', { ignoreConstructors: true }] },
+    })
+
+    assert.equal(report.warningCount, 0)
+  })
 })
 
 describe('Linter - func-visibility', () => {