Rule to require leading underscores in private variables

[nventuro]

Hi! In OpenZeppelin 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!

[fvictorio]

Hi @nventuro. I started to work on this rule and I could use some details.

I'm curious about the reason for using a leading underscore in private and internal variables, but only in internal functions. Any reason not to use it in private variables too? I will make this configurable in the rule, but I would like to have some sensible default.

Also, do you have any preference for the name of this rule? For now I'm going with leading-underscore-private-vars but it seems like a mouthful. Now that I'm thinking about this, maybe this should be a single rule about names that lets you configure everything? For example: SNAKE_CASE for constants, _camelCase for private and internal variables, camelCase for public stuff, etc.

cc @fernandomg

[fvictorio]

More thoughts on this.

I think having a single naming rule might make sense, but the config will be a mess:

  "rules": {
    "naming": ["warning", {
      "contract": "PascalCase",
      "library": "PascalCase",
      "struct": "PascalCase",
      "event": "PascalCase",
      "function": "camelCase", // or something like { "private": "_camelCase", ... }
      "functionArg": "camelCase",
      "localVar": "camelCase",
      "stateVar": {
        "constant": { // or "SNAKE_CASE" for making all visibilities SNAKE_CASE
          public: "SNAKE_CASE",
          private: "_SNAKE_CASE"
       }
        "variable": {
          public: "camelCase",
          private: "_camelCase"
        }
      }, // or "camelCase" for making everything camelCase
      "modifier": "camelCase"
    }]

On the other hand, having a single rule for each combination would be worse. We would have rules likes state-variable-internal-camel-case etc. In a sense, this is what we're doing now; we're just missing a lot of combinations. And adding configuration would be weird: "var-name-mixedcase": ["warning", "snake_case"]

I think a reasonable middle ground is to have one naming rule for each entity:

  "rules": {
    "contract-name": ["warning", "PascalCase"],
    "state-var-name": ["warning", {
      "public": "camelCase",
      "private": "_camelCAse"
    }],
    "constant-name": {
      "public": "SNAKE_CASE",
      "private": "_SNAKE_CASE"
    },
    ...
  }

Using the style guide for choosing the defaults. For example, in the previous config you could use just "contract-name": "warning" because PascalCase would be the default.

[nventuro]

I'm curious about the reason for using a leading underscore in private and internal variables, but only in internal functions. Any reason not to use it in private variables too? I will make this configurable in the rule, but I would like to have some sensible default.

To clarify, we use it in all private variables, and private and internal functions. public variables, and public and external functions don't start with an underscore.

An important exception I now realized I failed to mention is libraries: inside a library, no internal function starts with an underscore (internal means different things in contract and `library).

Also, do you have any preference for the name of this rule? For now I'm going with leading-underscore-private-vars but it seems like a mouthful.

I don't mind this name at all! I'd rather it be descriptive than cryptic, forcing me to look up the documentation to understand what it means.

Your proposal for the style of each construct is a good idea, in that it'd provide a lot of flexibility, but I'm not sure people actually need that level of granularity: I wouldn't worry too much about catering to all possible cases.

[fvictorio]

Well, I ended up just doing a single rule for this. These are the test cases. @nventuro could you please tell me if they work for you?:

  const SHOULD_WARN_CASES = [
    contractWith('uint private foo;'),
    contractWith('uint internal foo;'),
    contractWith('function foo() private {}'),
    contractWith('function foo() internal {}'),
    libraryWith('function foo() private {}'),

    contractWith('uint _foo;'),
    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 = [
    contractWith('uint private _foo;'),
    contractWith('uint internal _foo;'),
    contractWith('function _foo() private {}'),
    contractWith('function _foo() internal {}'),
    libraryWith('function _foo() private {}'),

    contractWith('uint foo;'),
    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 {}')
  ]

[fvictorio]

I ran this in your dev-v3.0 branch and these are the emitted warnings:

contracts/drafts/SignedSafeMath.sol
  8:29  warning  'INT256_MIN' should start with _  private-vars-leading-underscore

contracts/GSN/GSNRecipient.sol
  22:30  warning  'RELAYED_CALL_ACCEPTED' should start with _      private-vars-leading-underscore
  23:30  warning  'RELAYED_CALL_REJECTED' should start with _      private-vars-leading-underscore
  26:31  warning  'POST_RELAYED_CALL_MAX_GAS' should start with _  private-vars-leading-underscore

contracts/GSN/GSNRecipientERC20Fee.sol
  116:30  warning  'UINT256_MAX' should start with _  private-vars-leading-underscore

contracts/introspection/ERC165Checker.sol
  22:5  warning  '_supportsERC165' should not start with _         private-vars-leading-underscore
  35:5  warning  '_supportsInterface' should not start with _      private-vars-leading-underscore
  50:5  warning  '_supportsAllInterfaces' should not start with _  private-vars-leading-underscore

contracts/introspection/ERC1820Implementer.sol
  14:30  warning  'ERC1820_ACCEPT_MAGIC' should start with _  private-vars-leading-underscore

contracts/mocks/ArraysImpl.sol
  8:23  warning  'array' should start with _  private-vars-leading-underscore

contracts/mocks/ERC777SenderRecipientMock.sol
  40:30  warning  'TOKENS_SENDER_INTERFACE_HASH' should start with _     private-vars-leading-underscore
  41:30  warning  'TOKENS_RECIPIENT_INTERFACE_HASH' should start with _  private-vars-leading-underscore

contracts/mocks/ReentrancyMock.sol
  39:5  warning  'count' should start with _  private-vars-leading-underscore

contracts/mocks/RolesMock.sol
  8:24  warning  'dummyRole' should start with _  private-vars-leading-underscore

contracts/token/ERC20/SafeERC20.sol
  55:5  warning  'callOptionalReturn' should start with _  private-vars-leading-underscore

contracts/token/ERC777/ERC777.sol
  31:39  warning  'ERC1820_REGISTRY' should start with _                 private-vars-leading-underscore
  44:30  warning  'TOKENS_SENDER_INTERFACE_HASH' should start with _     private-vars-leading-underscore

By the way, const-name-snakecase doesn't emit a warning for me when I add a leading underscore to a const.

[fvictorio]

By the way, const-name-snakecase doesn't emit a warning for me when I add a leading underscore to a const.

Oh, you fixed that: e3aa88f 😅

[nventuro]

Those test cases look good! There's only one you might want to change:

contractWith('uint _foo;'),

I think the default is private, so this should be fine (we always make the visibility explicit though).

Thanks for taking a look at the dev3.0 branch! From a quick overview those warnings seem to be correct - that's why we want a linter 😂