Merge pull request #525 from protofire/fix-private-leading-underscore…

…-library Fix private vars leading underscore on libraries
protofire · Dec 5, 2023 · 80bb793 · 80bb793
2 parents ebe2ec4 + 5313f07
commit 80bb793
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 44 deletions.
diff --git a/docs/rules/naming/private-vars-leading-underscore.md b/docs/rules/naming/private-vars-leading-underscore.md
@@ -30,7 +30,7 @@ This rule accepts an array of options:
 ```
 
 ### Notes
-- This rule considers functions and variables in Libraries as well
+- This rule DO NOT considers functions and variables in Libraries
 - This rule skips external and public functions
 - This rule skips external and public state variables
 - See [here](https://docs.soliditylang.org/en/latest/style-guide.html#underscore-prefix-for-non-external-functions-and-variables) for further information

diff --git a/e2e/08-autofix/private-vars-underscore/Foo1AfterFix.sol b/e2e/08-autofix/private-vars-underscore/Foo1AfterFix.sol
@@ -1,21 +1,21 @@
 pragma solidity 0.8.0;
 
 library libraryName {
-    uint256 internal _lzarasa1;
-    uint256 internal _lzarasa2 = 2;
+    uint256 internal lzarasa1;
+    uint256 internal lzarasa2 = 2;
     uint256 internal _lzarasa3;
-    uint256 private _lzarasa4;
-    uint256 private _lzarasa5 = 5;
+    uint256 private lzarasa4;
+    uint256 private lzarasa5 = 5;
     uint256 private _lzarasa6;
 
-    uint256 public lzarasa7;
-    uint256 public lzarasa8 = 8;
+    uint256 public _lzarasa7;
+    uint256 public _lzarasa8 = 8;
     uint256 public lzarasa9;
 
     function fofo1() public {}
-    function fofo2() public {}
+    function _fofo2() public {}
     function _fofo3() internal {}
-    function _fofo4() internal {}
+    function fofo4() internal {}
 }
 
 contract Foo1 {

diff --git a/e2e/autofix-test.js b/e2e/autofix-test.js
@@ -257,7 +257,7 @@ describe('e2e', function () {
           expect(code).to.equal(1)
 
           const reportLines = stdout.split('\n')
-          const finalLine = '27 problems (27 errors, 0 warnings)'
+          const finalLine = '19 problems (19 errors, 0 warnings)'
           expect(reportLines[reportLines.length - 3]).to.contain(finalLine)
 
           result = compareTextFiles(currentFile, afterFixFile)

diff --git a/lib/rules/naming/private-vars-leading-underscore.js b/lib/rules/naming/private-vars-leading-underscore.js
@@ -67,7 +67,7 @@ const meta = {
     },
     notes: [
       {
-        note: 'This rule considers functions and variables in Libraries as well',
+        note: 'This rule DO NOT considers functions and variables in Libraries',
       },
       {
         note: 'This rule skips external and public functions',
@@ -103,26 +103,27 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker {
     this.isStrict = config && config.getObjectPropertyBoolean(ruleId, 'strict', DEFAULT_STRICTNESS)
   }
 
-  // ContractDefinition(node) {
-  //   if (node.kind === 'library') {
-  //     this.inLibrary = true
-  //   }
-  // }
+  ContractDefinition(node) {
+    if (node.kind === 'library') {
+      this.inLibrary = true
+    }
+  }
 
-  // 'ContractDefinition:exit'() {
-  //   this.inLibrary = false
-  // }
+  'ContractDefinition:exit'() {
+    this.inLibrary = false
+  }
 
   FunctionDefinition(node) {
-    if (!node.name) {
-      return
-    }
+    if (!this.inLibrary) {
+      if (!node.name) {
+        return
+      }
 
-    const isPrivate = node.visibility === 'private'
-    const isInternal = node.visibility === 'internal' || node.visibility === 'default'
-    // const shouldHaveLeadingUnderscore = isPrivate || (!this.inLibrary && isInternal)
-    const shouldHaveLeadingUnderscore = isPrivate || isInternal
-    this.validateName(node, shouldHaveLeadingUnderscore, 'function')
+      const isPrivate = node.visibility === 'private'
+      const isInternal = node.visibility === 'internal' || node.visibility === 'default'
+      const shouldHaveLeadingUnderscore = isPrivate || isInternal
+      this.validateName(node, shouldHaveLeadingUnderscore, 'function')
+    }
   }
 
   StateVariableDeclaration() {
@@ -134,18 +135,20 @@ class PrivateVarsLeadingUnderscoreChecker extends BaseChecker {
   }
 
   VariableDeclaration(node) {
-    if (!this.inStateVariableDeclaration) {
-      // if strict is enabled, non-state vars should not start with leading underscore
-      if (this.isStrict) {
-        this.validateName(node, false, 'variable')
+    if (!this.inLibrary) {
+      if (!this.inStateVariableDeclaration) {
+        // if strict is enabled, non-state vars should not start with leading underscore
+        if (this.isStrict) {
+          this.validateName(node, false, 'variable')
+        }
+        return
       }
-      return
-    }
 
-    const isPrivate = node.visibility === 'private'
-    const isInternal = node.visibility === 'internal' || node.visibility === 'default'
-    const shouldHaveLeadingUnderscore = isPrivate || isInternal
-    this.validateName(node, shouldHaveLeadingUnderscore, 'variable')
+      const isPrivate = node.visibility === 'private'
+      const isInternal = node.visibility === 'internal' || node.visibility === 'default'
+      const shouldHaveLeadingUnderscore = isPrivate || isInternal
+      this.validateName(node, shouldHaveLeadingUnderscore, 'variable')
+    }
   }
 
   validateName(node, shouldHaveLeadingUnderscore, type) {

diff --git a/test/rules/naming/private-vars-leading-underscore.js b/test/rules/naming/private-vars-leading-underscore.js
@@ -11,25 +11,18 @@ describe('Linter - private-vars-leading-underscore', () => {
     contractWith('function foo() {}'),
     contractWith('function foo() private {}'),
     contractWith('function foo() internal {}'),
-    libraryWith('function foo() {}'),
-    libraryWith('function foo() private {}'),
-    libraryWith('function foo() internal {}'),
 
     // warn when public/external names start with _
     contractWith('uint public _foo;'),
     contractWith('uint external _foo;'),
     contractWith('function _foo() public {}'),
     contractWith('function _foo() external {}'),
-    libraryWith('function _foo() public {}'),
-    libraryWith('function _foo() external {}'),
   ]
 
   const SHOULD_WARN_STRICT_CASES = [
     contractWith('function _foo() internal { uint _bar; }'),
     contractWith('function foo(uint _bar) external {}'),
     contractWith('function foo() public returns (uint256 _bar) {}'),
-    libraryWith('function _foo() returns (uint256 _bar) {}'),
-    libraryWith('function _foo(uint _bar) private {}'),
   ]
 
   const SHOULD_NOT_WARN_CASES = [