Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: ordering-rule support for top level statements #393

Merged
merged 3 commits into from
Feb 22, 2023
Merged

fix: ordering-rule support for top level statements #393

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
97a4fdf
ordering-rule custom error support
dbale-altoros Jan 27, 2023
0b4b278
ordering rule: added support for file level statements
dbale-altoros Feb 14, 2023
e57a680
fix: changed weight order
dbale-altoros Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion lib/rules/order/ordering.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class OrderingChecker extends BaseChecker {

SourceUnit(node) {
const children = node.children

this.checkOrder(children, sourceUnitPartOrder)
}

Expand Down Expand Up @@ -90,10 +89,22 @@ function sourceUnitPartOrder(node) {
return [10, 'import directive']
}

if (node.type === 'CustomErrorDefinition') {
return [15, 'custom error definition']
}

if (node.type === 'EnumDefinition' || node.type === 'StructDefinition') {
return [20, 'type definition']
}

if (node.type === 'FileLevelConstant') {
return [23, 'file level constant']
}

if (node.type === 'FunctionDefinition') {
return [26, 'free function definition']
}

if (node.type === 'ContractDefinition') {
if (node.kind === 'interface') {
return [30, 'interface']
Expand Down Expand Up @@ -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']
}
Expand Down
244 changes: 243 additions & 1 deletion test/rules/order/ordering.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {}
Expand All @@ -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 {}
Expand Down Expand Up @@ -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";
error Unauthorized();
enum MyEnum { A, B }
struct OneNiceStruct { uint256 a; uint256 b; }
uint256 constant oneNiceConstant = 1;
function freeFunction() pure returns (uint256) {
return 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frangio sorry for the ping, but do you have an opinion on this order for file-level nodes? The new supported elements are custom errors, constants and functions.

I feel like I would swap constants and errors here, so that the order is:

  • Constants
  • Enum/Structs
  • Custom errors
  • Functions
  • Contracts

Bikeshedding though.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No opinion here to be honest.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fvictorio I feel the order you proposed is a better one
I changed the weight so it can match what you suggested
Please approve when you have time

contract OneNiceContract {}
`
const report = linter.processStr(code, {
rules: { ordering: 'error' },
})

assert.equal(report.errorCount, 0)
})
})