Repository: protofire/solhint (js)

Issue:
Rule to require leading underscores in private variables
Hi! In [OpenZeppelin](https://github.com/OpenZeppelin/openzeppelin-solidity) we use leading underscores for `private` variables and functions, and `internal` functions (to both emphasize this attribute, and avoid clashing with `public` names).

Would it be possible to add a rule to enforce this sort of thing? I currently need to disable some rules, since e.g. I have `private` variables that are also `constant` (`bytes4 private constant _INTERFACE_ID_ERC165 = 0x01ffc9a7`), and the snake case rule doesn't support leading underscores.

Thanks!

Pull request description:
Add 'private-vars-leading-underscore' rule
Closes #90.

Private and internal state variables should have a leading underscore in their names. Public state variables should not. The default visibility is internal.

Private and internal functions should have a leading underscore. Public and external functions should not. The default visibility is public.

For functions in libraries, the rules are the same except internal functions shouldn't have a leading underscore (only private functions).

Patch:
diff --git a/lib/common/identifier-naming.js b/lib/common/identifier-naming.js
index fef5180..8545464 100644
--- a/lib/common/identifier-naming.js
+++ b/lib/common/identifier-naming.js
@@ -25,5 +25,9 @@ module.exports = {
 
   isNotUpperSnakeCase(text) {
     return !this.isUpperSnakeCase(text)
+  },
+
+  hasLeadingUnderscore(text) {
+    return text && text[0] === '_'
   }
 }
diff --git a/lib/rules/naming/index.js b/lib/rules/naming/index.js
index a4eab05..939b4e6 100644
--- a/lib/rules/naming/index.js
+++ b/lib/rules/naming/index.js
@@ -4,6 +4,7 @@ const EventNameCamelcaseChecker = require('./event-name-camelcase')
 const FuncNameMixedcaseChecker = require('./func-name-mixedcase')
 const FuncParamNameMixedcaseChecker = require('./func-param-name-mixedcase')
 const ModifierNameMixedcaseChecker = require('./modifier-name-mixedcase')
+const PrivateVarsLeadingUnderscore = require('./private-vars-leading-underscore')
 const UseForbiddenNameChecker = require('./use-forbidden-name')
 const VarNameMixedcaseChecker = require('./var-name-mixedcase')
 
@@ -15,6 +16,7 @@ module.exports = function checkers(reporter) {
     new FuncNameMixedcaseChecker(reporter),
     new FuncParamNameMixedcaseChecker(reporter),
     new ModifierNameMixedcaseChecker(reporter),
+    new PrivateVarsLeadingUnderscore(reporter),
     new UseForbiddenNameChecker(reporter),
     new VarNameMixedcaseChecker(reporter)
   ]
diff --git a/lib/rules/naming/private-vars-leading-underscore.js b/lib/rules/naming/private-vars-leading-underscore.js
new file mode 100644
index 0000000..3abb788
--- /dev/null
+++ b/lib/rules/naming/private-vars-leading-underscore.js
@@ -0,0 +1,79 @@
+const BaseChecker = require('./../base-checker')
+const naming = require('./../../common/identifier-naming')
+
+const ruleId = 'private-vars-leading-underscore'
+const meta = {
+  type: 'naming',
+
+  docs: {
+    description: 'Private and internal names must start with a single underscore.',
+    category: 'Style Guide Rules'
+  },
+
+  isDefault: false,
+  recommended: false,
+  defaultSetup: 'warn',
+
+  schema: null
+}
+
+class PrivateVarsLeadingUnderscoreChecker extends BaseChecker {
+  constructor(reporter) {
+    super(reporter, ruleId, meta)
+  }
+
+  ContractDefinition(node) {
+    if (node.kind === 'library') {
+      this.inLibrary = true
+    }
+  }
+
+  'ContractDefinition:exit'() {
+    this.inLibrary = false
+  }
+
+  FunctionDefinition(node) {
+    if (!node.name) {
+      return
+    }
+
+    const isPrivate = node.visibility === 'private'
+    const isInternal = node.visibility === 'internal'
+    const shouldHaveLeadingUnderscore = isPrivate || (!this.inLibrary && isInternal)
+    this.validateName(node, shouldHaveLeadingUnderscore)
+  }
+
+  StateVariableDeclaration() {
+    this.inStateVariableDeclaration = true
+  }
+
+  'StateVariableDeclaration:exit'() {
+    this.inStateVariableDeclaration = false
+  }
+
+  VariableDeclaration(node) {
+    if (!this.inStateVariableDeclaration) {
+      return
+    }
+
+    const isPrivate = node.visibility === 'private'
+    const isInternal = node.visibility === 'internal' || node.visibility === 'default'
+    const shouldHaveLeadingUnderscore = isPrivate || isInternal
+    this.validateName(node, shouldHaveLeadingUnderscore)
+  }
+
+  validateName(node, shouldHaveLeadingUnderscore) {
+    if (naming.hasLeadingUnderscore(node.name) !== shouldHaveLeadingUnderscore) {
+      this._error(node, node.name, shouldHaveLeadingUnderscore)
+    }
+  }
+
+  _error(node, name, shouldHaveLeadingUnderscore) {
+    this.error(
+      node,
+      `'${name}' ${shouldHaveLeadingUnderscore ? 'should' : 'should not'} start with _`
+    )
+  }
+}
+
+module.exports = PrivateVarsLeadingUnderscoreChecker


Test patch:
diff --git a/test/common/contract-builder.js b/test/common/contract-builder.js
index 317b022..2a99450 100644
--- a/test/common/contract-builder.js
+++ b/test/common/contract-builder.js
@@ -11,6 +11,17 @@ function contractWith(code) {
     `
 }
 
+function libraryWith(code) {
+  return `
+      pragma solidity 0.4.4;
+        
+        
+      library A {
+        ${code}
+      }
+    `
+}
+
 function funcWith(statements) {
   return contractWith(`
         function b() public {
@@ -56,6 +67,7 @@ function repeatLines(line, count) {
 
 module.exports = {
   contractWith,
+  libraryWith,
   funcWith,
   modifierWith,
   multiLine,
diff --git a/test/rules/naming/private-vars-leading-underscore.js b/test/rules/naming/private-vars-leading-underscore.js
new file mode 100644
index 0000000..6982a0f
--- /dev/null
+++ b/test/rules/naming/private-vars-leading-underscore.js
@@ -0,0 +1,81 @@
+const assert = require('assert')
+const linter = require('./../../../lib/index')
+const { contractWith, libraryWith } = require('./../../common/contract-builder')
+
+describe('Linter - private-vars-leading-underscore', () => {
+  const SHOULD_WARN_CASES = [
+    // warn when private/internal names don't start with _
+    contractWith('uint foo;'),
+    contractWith('uint private foo;'),
+    contractWith('uint internal foo;'),
+    contractWith('function foo() private {}'),
+    contractWith('function foo() internal {}'),
+    libraryWith('function foo() private {}'),
+
+    // warn when public/external/default names start with _
+    contractWith('uint public _foo;'),
+    contractWith('function _foo() {}'),
+    contractWith('function _foo() public {}'),
+    contractWith('function _foo() external {}'),
+    libraryWith('function _foo() {}'),
+    libraryWith('function _foo() public {}'),
+    libraryWith('function _foo() internal {}')
+  ]
+  const SHOULD_NOT_WARN_CASES = [
+    // don't warn when private/internal names start with _
+    contractWith('uint _foo;'),
+    contractWith('uint private _foo;'),
+    contractWith('uint internal _foo;'),
+    contractWith('function _foo() private {}'),
+    contractWith('function _foo() internal {}'),
+    libraryWith('function _foo() private {}'),
+
+    // don't warn when public/external/default names don't start with _
+    contractWith('uint public foo;'),
+    contractWith('function foo() {}'),
+    contractWith('function foo() public {}'),
+    contractWith('function foo() external {}'),
+    libraryWith('function foo() {}'),
+    libraryWith('function foo() public {}'),
+    libraryWith('function foo() internal {}'),
+
+    // don't warn for constructors
+    contractWith('constructor() public {}'),
+
+    // other names (variables, parameters, returns) shouldn't be affected by this rule
+    contractWith('function foo(uint bar) {}'),
+    contractWith('function foo(uint _bar) {}'),
+    contractWith('function foo() { uint bar; }'),
+    contractWith('function foo() { uint _bar; }'),
+    contractWith('function foo() returns (uint256) {}'),
+    contractWith('function foo() returns (uint256 bar) {}'),
+    contractWith('function foo() returns (uint256 _bar) {}'),
+    libraryWith('function foo(uint bar) {}'),
+    libraryWith('function foo(uint _bar) {}'),
+    libraryWith('function foo() { uint bar; }'),
+    libraryWith('function foo() { uint _bar; }'),
+    libraryWith('function foo() returns (uint256) {}'),
+    libraryWith('function foo() returns (uint256 bar) {}'),
+    libraryWith('function foo() returns (uint256 _bar) {}')
+  ]
+
+  SHOULD_WARN_CASES.forEach((code, index) => {
+    it(`should emit a warning (${index})`, () => {
+      const report = linter.processStr(code, {
+        rules: { 'private-vars-leading-underscore': 'error' }
+      })
+
+      assert.equal(report.errorCount, 1)
+    })
+  })
+
+  SHOULD_NOT_WARN_CASES.forEach((code, index) => {
+    it(`should not emit a warning (${index})`, () => {
+      const report = linter.processStr(code, {
+        rules: { 'private-vars-leading-underscore': 'error' }
+      })
+
+      assert.equal(report.errorCount, 0)
+    })
+  })
+})