support `ignoreConstructors` option for `no-empty-blocks` #418

hackartists · 2023-03-01T08:30:16Z

A new example

You can set ignoreConstructors simlar with func-visibility rule.

{
  "rules": {
    "no-empty-blocks": ["warn", {ignoreConstructors: true}]
  }
}

Related Issue

#242

…lhint#418 gets merged

…commit (#93) * chore: remove global import warnings * chore: fix 'Explicit mark state visibility' warnings * chore: use capslock for constant names * chore: Use camelCase for Contract name * chore: use mixedCase for function name * chore: fix unused variables * chore: fix avoid low level calls * chore: fix solhint no-inline-assembly * chore: fix solhint no empty blocks. Could be better once protofire/solhint#418 gets merged * chore: fix solhint max-states-count * chore: fix compilation warnings * chore: add forge build to pre-commit * chore: add --deny-warnings to CI * fix: remove --deny-warnings from husky --------- Co-authored-by: Orland0x <37511817+Orland0x@users.noreply.github.com>

juanpcapurro

I like this implementation ✨

However there's a thing I'd like to discuss:

the following is valid with the ignoreConstructors option

constuctor() {}

however the code doesn't do anything useful, and IMO it'd be good practice to
get rid of it.

the case in which this rule (without the option herein defined) is annoying is
where you have a constructor with a member initialization list:

constructor(uint a) Base(a) {}

since the linter complains about the empty code block but you can't get rid of
the constructor definition, since it's necessary to call the base constructor.

Therefore I propose to change the default behaviour of the rule so it always
ignores constructors with both a member initialization list and an empty body
and also always raises an error on an empty constructor without said
initialization

quick patch on top of 584479629 to show what I mean:

diff --git a/lib/rules/best-practises/no-empty-blocks.js b/lib/rules/best-practises/no-empty-blocks.js
index ba14fa5..1cd88dc 100644
--- a/lib/rules/best-practises/no-empty-blocks.js
+++ b/lib/rules/best-practises/no-empty-blocks.js
@@ -54,0 +55 @@ class NoEmptyBlocksChecker extends BaseChecker {
+    node.body.modifiers = node.modifiers
@@ -58 +59 @@ class NoEmptyBlocksChecker extends BaseChecker {
-    if (node.isConstructor && this.ignoreConstructors) {
+    if (node.isConstructor && node.modifiers.length > 0) {
diff --git a/test/rules/best-practises/no-empty-blocks.js b/test/rules/best-practises/no-empty-blocks.js
index 83ae325..5727461 100644
--- a/test/rules/best-practises/no-empty-blocks.js
+++ b/test/rules/best-practises/no-empty-blocks.js
@@ -29,0 +30 @@ describe('Linter - no-empty-blocks', () => {
+    contractWith('constructor (uint256 a) Base(a) {  }'),
@@ -117,10 +117,0 @@ describe('Linter - no-empty-blocks', () => {
-  it('should not raise error for constructor when ignoreConstructors is set to true', () => {
-    const code = contractWith(`constructor () {}`)
-
-    const report = linter.processStr(code, {
-      rules: { 'no-empty-blocks': ['warn', { ignoreConstructors: true }] },
-    })
-
-    assertNoWarnings(report)
-  })
-

what do y'all think?

hackartists · 2023-03-03T06:46:26Z

@juanpcapurro Thank you for your comments. I agree your comments because I mostly use empty constructor only for inherited contract.
However, it might have inteded empty block of constructor. It is because the solidity doesn't allow to skip constructor except for abstract contract. For example, you can often see as below code in javascript if the class don't need to do something when instantiation.

const instance = new Class();

In this case, javascript allow us to skip writing constructor, but the solidity.
As the above reason, it is the reason why I declare ignoreConstructors.

But, your comments can work for some, so how about adding an optional setting as belows.

Firstly, it will keep the current behavior as below.

{
      rules: { 'no-empty-blocks': ['warn', { ignoreConstructors: true }] },
}

Additinally, it allows to configure more detail as below.

{
      rules: { 'no-empty-blocks': ['warn', { ignoreConstructors: { mustHaveModifiers: true } }] },
}

How do you think about the above optional setting?
(Or we might consider about ignoreConstructors for all constructor and ignoreInheritedConstructors for contructors having modifiers.)

juanpcapurro · 2023-03-07T19:22:44Z

However, it might have inteded empty block of constructor. It is because the solidity doesn't allow to skip constructor except for abstract contract

I'm not sure I follow you. I just did a small test with the default contract for a forge init and it doesn't have a constructor and compiles just fine:

pragma solidity ^0.8.13;

contract Counter {
    uint256 public number;

    function setNumber(uint256 newNumber) public {
        number = newNumber;
    }

    function increment() public {
        number++;
    }
}

Could you provide an example of a contract that doesn't compile because of not having a constructor (including solidity version)?

or do you mean something else?

hackartists · 2023-03-08T04:27:11Z

@juanpcapurro oh, thank you, good to me. And I will reflect your feedback.

hackartists · 2023-03-08T07:54:25Z

@juanpcapurro I've pushed the commit reflecting your feedback.
By the commit, the configuration is reverted to same as origin, namely simple string.
Therefore, for all no-empty-block, it will disappear warning for inherited constructor having modifiers.

juanpcapurro · 2023-03-08T13:38:30Z

@dbale-altoros Could you enable CI for this PR?

dbale-altoros · 2023-03-08T15:00:05Z

@dbale-altoros Could you enable CI for this PR?

@juanpcapurro done

juanpcapurro

the blocking issue currently is wether to modify the AST or not, and the redundant testcases are secondary

juanpcapurro · 2023-03-08T13:16:47Z

test/rules/best-practises/no-empty-blocks.js

@@ -113,6 +115,26 @@ describe('Linter - no-empty-blocks', () => {
    assertNoWarnings(report)
  })

+  it('should raise error for empty block of [constructor]', () => {


isn't this whole hunk redundant with the two above?

You are right. They were redundant so I've removed them.

juanpcapurro · 2023-03-08T20:02:56Z

test/rules/best-practises/no-empty-blocks.js

+    assertWarnsCount(report, 1)
+  })
+
+  it('should not raise error for empty block of inherited [constructor] having modifiers', () => {


what do the brackets mean?

Basically, it was removed. But I denote the syntax keywards by brackets as the way others do

juanpcapurro · 2023-03-08T21:56:22Z

lib/rules/best-practises/no-empty-blocks.js

@@ -27,7 +27,15 @@ class NoEmptyBlocksChecker extends BaseChecker {
    this._validateContractPartsCount(node)
  }

+  FunctionDefinition(node) {
+    node.body.shouldIgnore = node.isConstructor && node.modifiers.length > 0


Keep in mind the AST is shared for all visitors, so this property will exist in the Blocks that are bodies of a function in all rule definitions that are executed after this one.

Therefore, an effort should be made not to pollute the AST with information not globally relevant.

My personally preferred approach would be to add the isConstructor and modifiers fields of the FunctionDefinition node to its body, and then assert isConstructors && modifiers.length > 0 in the listener for Block nodes.

Having those two names in the Block node makes it more clear what they are if they show up in the execution of another rule.

However, I'm not sure that it's a good practise to modify the AST that's gonna be used by other rules, and I'd like the opinion of @fvictorio on this topic. (I grepped a bit and couldn't find many usages of the node variable as an lvalue)

If it's problematic to modify the AST in this way, then you'd have to store the information of what blocks to ignore in the ComplexityScore class. One such way could be to have an object whose keys are a string that looks like ${range[0]-range[1]}, and it's value is wether to ignore the block or not.

The way this is done in other rules is by adding a field to the checker instance. So:

this.shouldIgnore = ...

Agree that modifying the node is not a good idea.

@fvictorio @juanpcapurro I agree what you've mentioned. Howeve whether to be constructor could be validated in FunctionDefinition. Therefore, I needed to recognize if its block is passed by constructor or not. Do you have any other good idea to pass it to the block.

What about using node.parent in the Block callback to check if the block corresponds to a constructor, and then checking if there are modifiers?

Something similar to that is done in a following line to check if the block corresponds to a fallback function.

@fvictorio Thank for good to me. I've change it to use node.parent and checked it working properly.

dbale-altoros · 2023-06-28T20:22:21Z

@hackartists thanks for this contribution!!
It is a great addition
Well done!

support ignoreConstructors option for no-empty-blocks

5844796

pscott added a commit to snapshot-labs/sx-evm that referenced this pull request Mar 2, 2023

chore: fix solhint no empty blocks. Could be better once protofire/so…

6e18aa3

…lhint#418 gets merged

pscott mentioned this pull request Mar 2, 2023

chores: fix compiler warnings; fix solhint warnings; add build to precommit snapshot-labs/sx-evm#93

Merged

juanpcapurro reviewed Mar 2, 2023

View reviewed changes

hackartists force-pushed the feat/ignore-empty-block-for-constructor branch from b117715 to 9e95930 Compare March 8, 2023 07:49

add modifier length checker for constructor

c81ffe6

hackartists force-pushed the feat/ignore-empty-block-for-constructor branch from 9e95930 to c81ffe6 Compare March 8, 2023 07:53

hackartists requested a review from juanpcapurro March 8, 2023 07:53

juanpcapurro reviewed Mar 8, 2023

View reviewed changes

hackartists added 2 commits March 10, 2023 13:42

remove redundant test cases

caedb35

eliminate modifying AST

651fee8

juanpcapurro mentioned this pull request May 24, 2023

Ignore emtpy block for constructor solhint-community/solhint-community#8

Merged

dbale-altoros approved these changes Jun 28, 2023

View reviewed changes

dbale-altoros mentioned this pull request Jun 28, 2023

no-empty-blocks false positive in inherited constructor #242

Closed

dbale-altoros merged commit 7b12625 into protofire:master Jun 28, 2023
10 checks passed

dbale-altoros mentioned this pull request Sep 13, 2023

"no-empty-blocks": ["error", { "ignoreConstructors": true }] not working #496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support `ignoreConstructors` option for `no-empty-blocks` #418

support `ignoreConstructors` option for `no-empty-blocks` #418

hackartists commented Mar 1, 2023

juanpcapurro left a comment

hackartists commented Mar 3, 2023 •

edited

juanpcapurro commented Mar 7, 2023

hackartists commented Mar 8, 2023

hackartists commented Mar 8, 2023 •

edited

juanpcapurro commented Mar 8, 2023

dbale-altoros commented Mar 8, 2023

juanpcapurro left a comment

juanpcapurro Mar 8, 2023

hackartists Mar 10, 2023

juanpcapurro Mar 8, 2023

hackartists Mar 10, 2023

juanpcapurro Mar 8, 2023

fvictorio Mar 9, 2023

hackartists Mar 10, 2023

fvictorio Mar 10, 2023

hackartists Mar 10, 2023

dbale-altoros commented Jun 28, 2023

support ignoreConstructors option for no-empty-blocks #418

support ignoreConstructors option for no-empty-blocks #418

Conversation

hackartists commented Mar 1, 2023

A new example

Related Issue

juanpcapurro left a comment

Choose a reason for hiding this comment

hackartists commented Mar 3, 2023 • edited

juanpcapurro commented Mar 7, 2023

hackartists commented Mar 8, 2023

hackartists commented Mar 8, 2023 • edited

juanpcapurro commented Mar 8, 2023

dbale-altoros commented Mar 8, 2023

juanpcapurro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbale-altoros commented Jun 28, 2023

support `ignoreConstructors` option for `no-empty-blocks` #418

support `ignoreConstructors` option for `no-empty-blocks` #418

hackartists commented Mar 3, 2023 •

edited

hackartists commented Mar 8, 2023 •

edited