Merge pull request #393 from protofire/fix/ordering-rule-for-custom-e…

…rrors

fix: ordering-rule custom error support

lib/rules/order/ordering.js

@@ -28,7 +28,6 @@ class OrderingChecker extends BaseChecker {
 
   SourceUnit(node) {
     const children = node.children
-
     this.checkOrder(children, sourceUnitPartOrder)
   }
 
@@ -90,21 +89,33 @@ function sourceUnitPartOrder(node) {
     return [10, 'import directive']
   }
 
+  if (node.type === 'FileLevelConstant') {
+    return [20, 'file level constant']
+  }
+
   if (node.type === 'EnumDefinition' || node.type === 'StructDefinition') {
-    return [20, 'type definition']
+    return [30, 'type definition']
+  }
+
+  if (node.type === 'CustomErrorDefinition') {
+    return [40, 'custom error definition']
+  }
+
+  if (node.type === 'FunctionDefinition') {
+    return [50, 'free function definition']
   }
 
   if (node.type === 'ContractDefinition') {
     if (node.kind === 'interface') {
-      return [30, 'interface']
+      return [60, 'interface']
     }
 
     if (node.kind === 'library') {
-      return [40, 'library definition']
+      return [70, 'library definition']
     }
 
     if (node.kind === 'contract') {
-      return [50, 'contract definition']
+      return [80, 'contract definition']
     }
   }
 
@@ -135,6 +146,10 @@ function contractPartOrder(node) {
     return [30, 'event definition']
   }
 
+  if (node.type === 'CustomErrorDefinition') {
+    return [35, 'custom error definition']
+  }
+
   if (node.type === 'ModifierDefinition') {
     return [40, 'modifier definition']
   }

test/rules/order/ordering.js

@@ -145,7 +145,7 @@ describe('Linter - ordering', () => {
     assert.ok(report.messages[0].message.includes('Function order is incorrect'))
   })
 
-  it('should not raise error when external const goes before public ', () => {
+  it('should not raise error when external function goes before public ', () => {
     const code = contractWith(`
                 function a() external view {}
                 function b() public {}
@@ -158,6 +158,110 @@ describe('Linter - ordering', () => {
     assert.equal(report.errorCount, 0)
   })
 
+  it('should raise an error when custom error is after external function', () => {
+    const code = contractWith(`
+                function a() external view {}
+                error Unauthorized();
+                function b() public {}
+            `)
+
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, custom error definition can not go after external view function'
+      )
+    )
+  })
+
+  it('should raise an error when custom error is after public function', () => {
+    const code = contractWith(`
+                function b() public {}
+                error Unauthorized();
+                function a() external view {}
+            `)
+
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, custom error definition can not go after public  function'
+      )
+    )
+  })
+
+  it('should raise an error when custom error is after constructor', () => {
+    const code = contractWith(`
+                constructor() {}
+                error Unauthorized();
+            `)
+
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, custom error definition can not go after constructor'
+      )
+    )
+  })
+
+  it('should raise an error when custom error is before event definition', () => {
+    const code = contractWith(`
+                error Unauthorized();
+                event WithdrawRegistered(uint256 receiver);
+                constructor() {}
+            `)
+
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, event definition can not go after custom error'
+      )
+    )
+  })
+
+  it('should not raise an error when custom error is well placed after event and before modifier', () => {
+    const code = contractWith(`
+                event WithdrawRegistered(uint256 receiver);
+                error Unauthorized();
+                modifier onlyOwner() {}
+                constructor() {}
+            `)
+
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
+
+  it('should not raise an error when custom error is well placed after state variables and before constructor', () => {
+    const code = contractWith(`
+                uint256 public stateVariable;
+                error Unauthorized();
+                constructor() {}
+            `)
+
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
+
   it('should raise error for enum after contract', () => {
     const code = `
       contract Foo {}
@@ -185,4 +289,142 @@ describe('Linter - ordering', () => {
 
     assert.equal(report.errorCount, 1)
   })
+
+  it('should raise error when custom error is before import', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      error Unauthorized();
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, import directive can not go after custom error definition'
+      )
+    )
+  })
+
+  it('should not raise error when custom error is defined in correct order', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      error Unauthorized();
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
+
+  it('should raise error when free function is before custom error', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      function freeFunction() pure returns (uint256) {
+        return 1;
+      }     
+      error Unauthorized();
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, custom error definition can not go after free function definition'
+      )
+    )
+  })
+
+  it('should not raise error when free function is defined in correct order', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      error Unauthorized();
+      function freeFunction() pure returns (uint256) {
+        return 1;
+      }     
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
+
+  it('should raise error when file level constant is defined after free function', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      function freeFunction() pure returns (uint256) {
+        return 1;
+      }     
+      uint256 constant oneNiceConstant = 1;
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 1)
+    assert.ok(
+      report.messages[0].message.includes(
+        'Function order is incorrect, file level constant can not go after free function definition'
+      )
+    )
+  })
+
+  it('should not raise error when file level constant is defined in correct order', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      uint256 constant oneNiceConstant = 1;
+      function freeFunction() pure returns (uint256) {
+        return 1;
+      }     
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
+
+  it('should not raise error when all top level code is well placed', () => {
+    const code = `
+      // SPDX-License-Identifier: MIT
+      pragma solidity ^0.8.0;
+      import "@openzeppelin/contracts/ownership/Ownable.sol";
+      uint256 constant oneNiceConstant = 1;
+      enum MyEnum { A, B }
+      struct OneNiceStruct { uint256 a; uint256 b; }            
+      error Unauthorized();
+      function freeFunction() pure returns (uint256) {
+        return 1;
+      }     
+      contract OneNiceContract {}
+    `
+    const report = linter.processStr(code, {
+      rules: { ordering: 'error' },
+    })
+
+    assert.equal(report.errorCount, 0)
+  })
 })