Reviewing how and when we call cleanModifierInvocationArguments

src/slang-nodes/ContractDefinition.ts

@@ -1,6 +1,6 @@
+import { NonterminalKind } from '@nomicfoundation/slang/cst';
 import { doc } from 'prettier';
 import { satisfies } from 'semver';
-import { NonterminalKind } from '@nomicfoundation/slang/cst';
 import { SlangNode } from './SlangNode.js';
 import { TerminalNode } from './TerminalNode.js';
 import { ContractSpecifiers } from './ContractSpecifiers.js';
@@ -42,12 +42,10 @@ export class ContractDefinition extends SlangNode {
 
     this.updateMetadata(this.specifiers, this.members);
 
-    this.cleanModifierInvocationArguments(options);
-  }
-
-  cleanModifierInvocationArguments(options: ParserOptions<AstNode>): void {
     // Older versions of Solidity defined a constructor as a function having
     // the same name as the contract.
+    // So we delegate to the parents the responsibility of cleaning the
+    // arguments of modifier invocations.
     if (!satisfies(options.compiler, '>=0.5.0')) {
       for (const member of this.members.items) {
         if (

src/slang-nodes/FallbackFunctionDefinition.ts

@@ -52,15 +52,8 @@ export class FallbackFunctionDefinition extends SlangNode {
       this.body
     );
 
-    this.cleanModifierInvocationArguments();
-  }
-
-  cleanModifierInvocationArguments(): void {
     for (const attribute of this.attributes.items) {
-      if (
-        typeof attribute !== 'string' &&
-        attribute.kind === NonterminalKind.ModifierInvocation
-      ) {
+      if (attribute.kind === NonterminalKind.ModifierInvocation) {
         attribute.cleanModifierInvocationArguments();
       }
     }

src/slang-nodes/FunctionDefinition.ts

@@ -1,5 +1,5 @@
-import { satisfies } from 'semver';
 import { NonterminalKind } from '@nomicfoundation/slang/cst';
+import { satisfies } from 'semver';
 import { printFunctionWithBody } from '../slang-printers/print-function.js';
 import { extractVariant } from '../slang-utils/extract-variant.js';
 import { SlangNode } from './SlangNode.js';
@@ -60,17 +60,16 @@ export class FunctionDefinition extends SlangNode {
 
     // Older versions of Solidity defined a constructor as a function having
     // the same name as the contract.
+    // So we delegate to the parents the responsibility of cleaning the
+    // arguments of modifier invocations.
     if (satisfies(options.compiler, '>=0.5.0')) {
       this.cleanModifierInvocationArguments();
     }
   }
 
   cleanModifierInvocationArguments(): void {
     for (const attribute of this.attributes.items) {
-      if (
-        typeof attribute !== 'string' &&
-        attribute.kind === NonterminalKind.ModifierInvocation
-      ) {
+      if (attribute.kind === NonterminalKind.ModifierInvocation) {
         attribute.cleanModifierInvocationArguments();
       }
     }

src/slang-nodes/ImportDeconstructionSymbols.ts

@@ -1,6 +1,6 @@
+import { NonterminalKind } from '@nomicfoundation/slang/cst';
 import { doc } from 'prettier';
 import { satisfies } from 'semver';
-import { NonterminalKind } from '@nomicfoundation/slang/cst';
 import { printSeparatedList } from '../slang-printers/print-separated-list.js';
 import { SlangNode } from './SlangNode.js';
 import { ImportDeconstructionSymbol } from './ImportDeconstructionSymbol.js';

src/slang-nodes/LibraryDefinition.ts

@@ -1,5 +1,6 @@
 import { NonterminalKind } from '@nomicfoundation/slang/cst';
 import { doc } from 'prettier';
+import { satisfies } from 'semver';
 import { SlangNode } from './SlangNode.js';
 import { TerminalNode } from './TerminalNode.js';
 import { LibraryMembers } from './LibraryMembers.js';
@@ -29,6 +30,18 @@ export class LibraryDefinition extends SlangNode {
     this.members = new LibraryMembers(ast.members, collected, options);
 
     this.updateMetadata(this.members);
+
+    // Older versions of Solidity defined a constructor as a function having
+    // the same name as the contract.
+    // So we delegate to the parents the responsibility of cleaning the
+    // arguments of modifier invocations.
+    if (!satisfies(options.compiler, '>=0.5.0')) {
+      for (const member of this.members.items) {
+        if (member.kind === NonterminalKind.FunctionDefinition) {
+          member.cleanModifierInvocationArguments();
+        }
+      }
+    }
   }
 
   print(path: AstPath<LibraryDefinition>, print: PrintFunction): Doc {

src/slang-nodes/ReceiveFunctionDefinition.ts

@@ -41,15 +41,8 @@ export class ReceiveFunctionDefinition extends SlangNode {
 
     this.updateMetadata(this.parameters, this.attributes, this.body);
 
-    this.cleanModifierInvocationArguments();
-  }
-
-  cleanModifierInvocationArguments(): void {
     for (const attribute of this.attributes.items) {
-      if (
-        typeof attribute !== 'string' &&
-        attribute.kind === NonterminalKind.ModifierInvocation
-      ) {
+      if (attribute.kind === NonterminalKind.ModifierInvocation) {
         attribute.cleanModifierInvocationArguments();
       }
     }

src/slang-nodes/UnnamedFunctionDefinition.ts

@@ -41,10 +41,6 @@ export class UnnamedFunctionDefinition extends SlangNode {
 
     this.updateMetadata(this.parameters, this.attributes, this.body);
 
-    this.cleanModifierInvocationArguments();
-  }
-
-  cleanModifierInvocationArguments(): void {
     for (const attribute of this.attributes.items) {
       if (attribute.kind === NonterminalKind.ModifierInvocation) {
         attribute.cleanModifierInvocationArguments();

tests/format/ModifierInvocations/ModifierInvocations.sol

@@ -15,3 +15,21 @@ contract ModifierInvocations is ModifierDefinitions {
   // We remove parentheses in modifiers without arguments.
   function test() public emptyParams noParams() {}
 }
+
+library ModifierInvocationsLibrary {
+  // We enforce the use of parentheses in modifiers without parameters.
+  modifier emptyParams {_;}
+  modifier noParams() {_;}
+
+  modifier nonZero (uint x) {
+    require (x != 0);
+    _;
+  }
+
+  function isPrime (uint x) public nonZero (x) returns (bool) {
+    // Complicated logic here
+  }
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}

tests/format/ModifierInvocations/__snapshots__/format.test.js.snap

@@ -24,6 +24,24 @@ contract ModifierInvocations is ModifierDefinitions {
   function test() public emptyParams noParams() {}
 }
 
+library ModifierInvocationsLibrary {
+  // We enforce the use of parentheses in modifiers without parameters.
+  modifier emptyParams {_;}
+  modifier noParams() {_;}
+
+  modifier nonZero (uint x) {
+    require (x != 0);
+    _;
+  }
+
+  function isPrime (uint x) public nonZero (x) returns (bool) {
+    // Complicated logic here
+  }
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}
+
 =====================================output=====================================
 // SPDX-License-Identifier: MIT
 pragma solidity 0.8.34;
@@ -47,5 +65,27 @@ contract ModifierInvocations is ModifierDefinitions {
     function test() public emptyParams noParams {}
 }
 
+library ModifierInvocationsLibrary {
+    // We enforce the use of parentheses in modifiers without parameters.
+    modifier emptyParams() {
+        _;
+    }
+    modifier noParams() {
+        _;
+    }
+
+    modifier nonZero(uint x) {
+        require(x != 0);
+        _;
+    }
+
+    function isPrime(uint x) public nonZero(x) returns (bool) {
+        // Complicated logic here
+    }
+
+    // We remove parentheses in modifiers without arguments.
+    function test() public emptyParams noParams {}
+}
+
 ================================================================================
 `;

tests/format/ModifierInvocationsV0.4.26/ModifierInvocations.sol

@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.4.26;
+
+contract ModifierDefinitions {
+  // We enforce the use of parentheses in modifiers without parameters.
+  modifier emptyParams {_;}
+  modifier noParams() {_;}
+}
+
+contract ModifierInvocations is ModifierDefinitions {
+  // We can't differentiate between constructor calls or modifiers, so we keep
+  // parentheses untouched in constructors.
+  function ModifierInvocations () emptyParams noParams() ModifierDefinitions() {}
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}
+
+library ModifierInvocationsLibrary {
+  // We enforce the use of parentheses in modifiers without parameters.
+  modifier emptyParams {_;}
+  modifier noParams() {_;}
+
+  modifier nonZero (uint x) {
+    require (x != 0);
+    _;
+  }
+
+  function isPrime (uint x) public nonZero (x) returns (bool) {
+    // Complicated logic here
+  }
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}

tests/format/ModifierInvocationsV0.4.26/__snapshots__/format.test.js.snap

@@ -0,0 +1,96 @@
+// Jest Snapshot v1, https://jestjs.io/docs/snapshot-testing
+
+exports[`ModifierInvocations.sol - {"compiler":"0.4.26"} format 1`] = `
+====================================options=====================================
+compiler: "0.4.26"
+parsers: ["slang"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+// SPDX-License-Identifier: MIT
+pragma solidity 0.4.26;
+
+contract ModifierDefinitions {
+  // We enforce the use of parentheses in modifiers without parameters.
+  modifier emptyParams {_;}
+  modifier noParams() {_;}
+}
+
+contract ModifierInvocations is ModifierDefinitions {
+  // We can't differentiate between constructor calls or modifiers, so we keep
+  // parentheses untouched in constructors.
+  function ModifierInvocations () emptyParams noParams() ModifierDefinitions() {}
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}
+
+library ModifierInvocationsLibrary {
+  // We enforce the use of parentheses in modifiers without parameters.
+  modifier emptyParams {_;}
+  modifier noParams() {_;}
+
+  modifier nonZero (uint x) {
+    require (x != 0);
+    _;
+  }
+
+  function isPrime (uint x) public nonZero (x) returns (bool) {
+    // Complicated logic here
+  }
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}
+
+=====================================output=====================================
+// SPDX-License-Identifier: MIT
+pragma solidity 0.4.26;
+
+contract ModifierDefinitions {
+    // We enforce the use of parentheses in modifiers without parameters.
+    modifier emptyParams() {
+        _;
+    }
+    modifier noParams() {
+        _;
+    }
+}
+
+contract ModifierInvocations is ModifierDefinitions {
+    // We can't differentiate between constructor calls or modifiers, so we keep
+    // parentheses untouched in constructors.
+    function ModifierInvocations()
+        emptyParams
+        noParams()
+        ModifierDefinitions()
+    {}
+
+    // We remove parentheses in modifiers without arguments.
+    function test() public emptyParams noParams {}
+}
+
+library ModifierInvocationsLibrary {
+    // We enforce the use of parentheses in modifiers without parameters.
+    modifier emptyParams() {
+        _;
+    }
+    modifier noParams() {
+        _;
+    }
+
+    modifier nonZero(uint x) {
+        require(x != 0);
+        _;
+    }
+
+    function isPrime(uint x) public nonZero(x) returns (bool) {
+        // Complicated logic here
+    }
+
+    // We remove parentheses in modifiers without arguments.
+    function test() public emptyParams noParams {}
+}
+
+================================================================================
+`;

tests/format/ModifierInvocationsV0.4.26/format.test.js

@@ -0,0 +1 @@
+runFormatTest(import.meta, ['slang'], { compiler: '0.4.26' });