Merge pull request #656 from protofire/feat/improve-ordering

feat: improve ordering rule

Author: dbale-altoros

docs/rules.md

@@ -44,7 +44,7 @@ title:       "Rule Index of Solhint"
 | [var-name-mixedcase](./rules/naming/var-name-mixedcase.md)                           | Variable names must be in mixedCase. (Does not check IMMUTABLES, use immutable-vars-naming)                   | $~~~~~~~~$✔️ |            |
 | [imports-on-top](./rules/order/imports-on-top.md)                                    | Import statements must be on top.                                                                             | $~~~~~~~~$✔️ |            |
 | [imports-order](./rules/naming/imports-order.md)                                     | Order the imports of the contract to follow a certain hierarchy (read "Notes section")                        |              |            |
-| [ordering](./rules/order/ordering.md)                                                | Check order of elements in file and inside each contract, according to the style guide.                       |              |            |
+| [ordering](./rules/order/ordering.md)                                                | Check order of elements in file and inside each contract, according to the style guide                        |              |            |
 | [visibility-modifier-order](./rules/order/visibility-modifier-order.md)              | Visibility modifier must be first in list of modifiers.                                                       | $~~~~~~~~$✔️ |            |
         
 

docs/rules/order/ordering.md

@@ -9,7 +9,7 @@ title:       "ordering | Solhint"
 ![Default Severity Badge warn](https://img.shields.io/badge/Default%20Severity-warn-yellow)
 
 ## Description
-Check order of elements in file and inside each contract, according to the style guide.
+Check order of elements in file and inside each contract, according to the style guide
 
 ## Options
 This rule accepts a string option for rule severity. Must be one of "error", "warn", "off". Defaults to warn.
@@ -58,14 +58,14 @@ library MyLibrary {
 }
 
 contract MyContract {
-  struct InnerStruct {
-    bool flag;
-  }
-
   enum InnerEnum {
     A, B, C
   }
 
+  struct InnerStruct {
+    bool flag;
+  }
+
   uint public x;
   uint public y;
 
@@ -120,14 +120,14 @@ library MyLibrary {
 contract MyContract {
   using MyLibrary for uint;
 
-  struct InnerStruct {
-    bool flag;
-  }
-
   enum InnerEnum {
     A, B, C
   }
 
+  struct InnerStruct {
+    bool flag;
+  }
+
   address payable owner;
   uint public x;
   uint public y;
@@ -198,14 +198,98 @@ library MyLibrary {
 contract MyContract {
   using MyLibrary for uint;
 
+  enum InnerEnum {
+    A, B, C
+  }
+
   struct InnerStruct {
     bool flag;
   }
 
+  address payable owner;
+  uint public x;
+  uint public y;
+
+  event MyEvent(address a);
+
+  modifier onlyOwner {
+    require(
+      msg.sender == owner,
+      "Only owner can call this function."
+    );
+    _;
+  }
+
+  constructor () public {}
+
+  receive() external payable {}
+
+  fallback () external {}
+
+  function myExternalFunction() external {}
+  function myExternalViewFunction() external view {}
+  function myExternalPureFunction() external pure {}
+
+  function myPublicFunction() public {}
+  function myPublicViewFunction() public view {}
+  function myPublicPureFunction() public pure {}
+
+  function myInternalFunction() internal {}
+  function myInternalViewFunction() internal view {}
+  function myInternalPureFunction() internal pure {}
+
+  function myPrivateFunction() private {}
+  function myPrivateViewFunction() private view {}
+  function myPrivatePureFunction() private pure {}
+}
+
+```
+
+#### All units are in order - ^0.7.0
+
+```solidity
+
+pragma solidity ^0.7.0;
+
+import "./some/library.sol";
+import "./some/other-library.sol";
+
+uint constant FOO_BAR = 42;
+
+enum MyEnum {
+  Foo,
+  Bar
+}
+
+struct MyStruct {
+  uint x;
+  uint y;
+}
+
+interface IBox {
+  function getValue() public;
+  function setValue(uint) public;
+}
+
+library MyLibrary {
+  function add(uint a, uint b, uint c) public returns (uint) {
+    return a + b + c;
+  }
+}
+
+contract MyContract {
+  using MyLibrary for uint;
+
   enum InnerEnum {
     A, B, C
   }
 
+  struct InnerStruct {
+    bool flag;
+  }
+
+  uint constant v;
+  uint immutable w;
   address payable owner;
   uint public x;
   uint public y;
@@ -339,6 +423,89 @@ contract MyContract {
 
 ```
 
+#### file-level constant before import
+
+```solidity
+
+uint256 constant FOO_BAR = 42;
+import {A} from "./A.sol";
+
+```
+
+#### file-level constant after type definition
+
+```solidity
+
+struct foo { uint x; }
+uint256 constant FOO_BAR = 42;
+
+```
+
+#### immutable after mutable
+
+```solidity
+
+contract Foo {
+uint bar;
+uint immutable foo;
+}
+
+```
+
+#### constant after immutable
+
+```solidity
+
+contract Foo {
+uint immutable bar;
+uint constant foo;
+}
+
+```
+
+#### constant after mutable
+
+```solidity
+
+contract Foo {
+uint bar;
+uint constant foo;
+}
+
+```
+
+#### file-scoped struct before enum
+
+```solidity
+
+  struct MyStruct {
+    uint x;
+    uint y;
+  }
+  enum MyEnum {
+    Foo,
+    Bar
+  }
+
+```
+
+#### contract-scoped struct before enum
+
+```solidity
+
+contract MyContract {
+  struct MyStruct {
+    uint x;
+    uint y;
+  }
+  enum MyEnum {
+    Foo,
+    Bar
+  }
+}
+
+```
+
 ## Version
 This rule was introduced in [Solhint 3.2.0](https://github.com/protofire/solhint/blob/v3.2.0)
 

lib/rules/order/ordering.js

@@ -6,7 +6,7 @@ const meta = {
   type: 'order',
 
   docs: {
-    description: `Check order of elements in file and inside each contract, according to the style guide.`,
+    description: `Check order of elements in file and inside each contract, according to the style guide`,
     category: 'Style Guide Rules',
     examples: {
       good: require('../../../test/fixtures/order/ordering-correct'),
@@ -75,10 +75,6 @@ function getMutabilityWeight({ baseWeight, stateMutability }) {
   }
 }
 
-function isTypeDeclaration(node) {
-  return ['StructDefinition', 'EnumDefinition'].includes(node.type)
-}
-
 function sourceUnitPartOrder(node) {
   if (node.type === 'PragmaDirective') {
     return [0, 'pragma directive']
@@ -92,8 +88,12 @@ function sourceUnitPartOrder(node) {
     return [20, 'file level constant']
   }
 
-  if (node.type === 'EnumDefinition' || node.type === 'StructDefinition') {
-    return [30, 'type definition']
+  if (node.type === 'EnumDefinition') {
+    return [30, 'enum definition']
+  }
+
+  if (node.type === 'StructDefinition') {
+    return [35, 'struct definition']
   }
 
   if (node.type === 'CustomErrorDefinition') {
@@ -121,31 +121,39 @@ function sourceUnitPartOrder(node) {
   throw new Error('Unrecognized source unit part, please report this issue')
 }
 
-function isInitializerModifier(modifiers, targetName, targetArguments) {
-  // search the modifiers array with the name === 'initializer'
-  return modifiers.some(
-    (modifier) => modifier.name === targetName && modifier.arguments === targetArguments
-  )
-}
-
 function contractPartOrder(node) {
   if (node.type === 'UsingForDeclaration') {
     return [0, 'using for declaration']
   }
 
-  if (isTypeDeclaration(node)) {
-    let label
-    if (node.type === 'StructDefinition') {
-      label = 'struct definition'
-    } else {
-      label = 'enum definition'
-    }
+  if (node.type === 'EnumDefinition') {
+    return [10, 'enum definition']
+  }
 
-    return [10, label]
+  if (node.type === 'StructDefinition') {
+    return [15, 'struct definition']
   }
 
   if (node.type === 'StateVariableDeclaration') {
-    return [20, 'state variable declaration']
+    // the grammar: https://docs.soliditylang.org/en/latest/grammar.html
+    // forbids declaration of multiple state variables in the same
+    // StateVariableDeclaration, however in the AST they show up in an array,
+    // similar to regular VariableDeclarationStatements, which allow
+    // VariableDefinitionTuples inside them and therefore can declare many
+    // variables at once
+    if (node.variables.length !== 1) {
+      throw new Error(
+        'state variable definition with more than one variable. Please report this issue'
+      )
+    }
+    const variable = node.variables[0]
+    if (variable.isDeclaredConst) {
+      return [20, 'contract constant declaration']
+    } else if (variable.isImmutable) {
+      return [22, 'contract immutable declaration']
+    } else {
+      return [25, 'state variable declaration']
+    }
   }
 
   if (node.type === 'EventDefinition') {
@@ -160,13 +168,8 @@ function contractPartOrder(node) {
     return [40, 'modifier definition']
   }
 
-  if (
-    node.isConstructor ||
-    (node.type === 'FunctionDefinition' &&
-      node.name === 'initialize' &&
-      isInitializerModifier(node.modifiers, 'initializer', null))
-  ) {
-    return [50, 'constructor/initializer']
+  if (node.isConstructor) {
+    return [50, 'constructor']
   }
 
   if (isReceiveFunction(node)) {