removing parentheses from modifiers with an empty list of parameters. (#555)

Author: Janther

README.md

@@ -277,6 +277,34 @@ As a final check, make sure that VSCode is configured to format files on save.
 
 Note: By design, Prettier prioritizes a local over a global configuration. If you have a `.prettierrc` file in your porject, your VSCode's default settings or rules in `settings.json` are ignored ([prettier/prettier-vscode#1079](https://github.com/prettier/prettier-vscode/issues/1079)).
 
+## Edge cases
+
+Prettier Solidity does its best to be pretty and consistent, but in some cases it falls back to doing things that are less than ideal.
+
+### Modifiers in constructors
+
+Modifiers with no arguments are formatted with their parentheses removed, except for constructors. The reason for this is that Prettier Solidity cannot always tell apart a modifier from a base constructor. So modifiers in constructors are not modified. For example, this:
+
+```solidity
+contract Foo is Bar {
+  constructor() Bar() modifier1 modifier2() modifier3(42) {}
+
+  function f() modifier1 modifier2() modifier3(42) {}
+}
+```
+
+will be formatted as
+
+```solidity
+contract Foo is Bar {
+  constructor() Bar() modifier1 modifier2() modifier3(42) {}
+
+  function f() modifier1 modifier2 modifier3(42) {}
+}
+```
+
+Notice that the unnecessary parentheses in `modifier2` were removed in the function but not in the constructor.
+
 ## Contributing
 
 1. [Fork it](https://github.com/prettier-solidity/prettier-plugin-solidity/fork)

src/nodes/ModifierDefinition.js

@@ -7,25 +7,23 @@ const {
 const printSeparatedList = require('./print-separated-list');
 
 const modifierParameters = (node, path, print) => {
-  if (node.parameters) {
-    return node.parameters.length > 0
-      ? [
-          '(',
-          printSeparatedList(path.map(print, 'parameters'), {
-            separator: [
-              ',',
-              // To keep consistency any list of parameters will split if it's longer than 2.
-              // For more information see:
-              // https://github.com/prettier-solidity/prettier-plugin-solidity/issues/256
-              node.parameters.length > 2 ? hardline : line
-            ]
-          }),
-          ')'
+  if (node.parameters && node.parameters.length > 0) {
+    return [
+      '(',
+      printSeparatedList(path.map(print, 'parameters'), {
+        separator: [
+          ',',
+          // To keep consistency any list of parameters will split if it's longer than 2.
+          // For more information see:
+          // https://github.com/prettier-solidity/prettier-plugin-solidity/issues/256
+          node.parameters.length > 2 ? hardline : line
         ]
-      : '()';
+      }),
+      ')'
+    ];
   }
 
-  return '';
+  return '()';
 };
 
 const virtual = (node) => (node.isVirtual ? [line, 'virtual'] : '');

src/nodes/ModifierInvocation.js

@@ -2,6 +2,8 @@ const printSeparatedList = require('./print-separated-list');
 
 const modifierArguments = (node, path, print) => {
   if (node.arguments) {
+    // We always print parentheses at this stage because the parser already
+    // stripped them in FunctionDefinitions that are not a constructor.
     return node.arguments.length > 0
       ? ['(', printSeparatedList(path.map(print, 'arguments')), ')']
       : '()';

src/parser.js

@@ -43,6 +43,21 @@ function parse(text, _parsers, options) {
         );
       }
     },
+    ModifierDefinition(ctx) {
+      if (!ctx.parameters) {
+        ctx.parameters = [];
+      }
+    },
+    FunctionDefinition(ctx) {
+      if (!ctx.isConstructor) {
+        ctx.modifiers.forEach((modifier) => {
+          if (modifier.arguments && modifier.arguments.length === 0) {
+            // eslint-disable-next-line no-param-reassign
+            modifier.arguments = null;
+          }
+        });
+      }
+    },
     ForStatement(ctx) {
       if (ctx.initExpression) {
         ctx.initExpression.omitSemicolon = true;

tests/config/format-test.js

@@ -61,6 +61,7 @@ const testsWithAstChanges = new Map(
     "Parentheses/BitXorNoParentheses.sol",
     "Parentheses/LogicNoParentheses.sol",
     "HexLiteral/HexLiteral.sol",
+    "ModifierInvocations/ModifierInvocations.sol",
   ].map((fixture) => {
     const [file, compareBytecode = () => true] = Array.isArray(fixture)
       ? fixture

tests/format/AllSolidityFeatures/__snapshots__/jsfmt.spec.js.snap

@@ -750,7 +750,7 @@ contract DualIndex {
     mapping(uint256 => mapping(uint256 => uint256)) data;
     address public admin;
 
-    modifier restricted {
+    modifier restricted() {
         if (msg.sender == admin) _;
     }
 

tests/format/FunctionDefinitions/FunctionDefinitions.sol

@@ -9,11 +9,11 @@ interface FunctionInterfaces {
 
   function manyParams(uint x1, uint x2, uint x3, uint x4, uint x5, uint x6, uint x7, uint x8, uint x9, uint x10);
 
-  function manyModifiers() modifier1 modifier2 modifier3 modifier4 modifier5 modifier6 modifier7 modifier8 modifier9 modifier10;
+  function manyModifiers() modifier1() modifier2() modifier3 modifier4 modifier5 modifier6 modifier7 modifier8 modifier9 modifier10;
 
   function manyReturns() returns(uint y1, uint y2, uint y3, uint y4, uint y5, uint y6, uint y7, uint y8, uint y9, uint y10);
 
-  function someParamsSomeModifiers(uint x1, uint x2, uint x3) modifier1 modifier2 modifier3;
+  function someParamsSomeModifiers(uint x1, uint x2, uint x3) modifier1() modifier2 modifier3;
 
   function someParamsSomeReturns(uint x1, uint x2, uint x3) returns(uint y1, uint y2, uint y3);
 

tests/format/FunctionDefinitions/__snapshots__/jsfmt.spec.js.snap

@@ -17,11 +17,11 @@ interface FunctionInterfaces {
 
   function manyParams(uint x1, uint x2, uint x3, uint x4, uint x5, uint x6, uint x7, uint x8, uint x9, uint x10);
 
-  function manyModifiers() modifier1 modifier2 modifier3 modifier4 modifier5 modifier6 modifier7 modifier8 modifier9 modifier10;
+  function manyModifiers() modifier1() modifier2() modifier3 modifier4 modifier5 modifier6 modifier7 modifier8 modifier9 modifier10;
 
   function manyReturns() returns(uint y1, uint y2, uint y3, uint y4, uint y5, uint y6, uint y7, uint y8, uint y9, uint y10);
 
-  function someParamsSomeModifiers(uint x1, uint x2, uint x3) modifier1 modifier2 modifier3;
+  function someParamsSomeModifiers(uint x1, uint x2, uint x3) modifier1() modifier2 modifier3;
 
   function someParamsSomeReturns(uint x1, uint x2, uint x3) returns(uint y1, uint y2, uint y3);
 

tests/format/ModifierInvocations/ModifierInvocations.sol

@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.6;
+
+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.
+  constructor () emptyParams noParams() ModifierDefinitions() {}
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}

tests/format/ModifierInvocations/__snapshots__/jsfmt.spec.js.snap

@@ -0,0 +1,51 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`ModifierInvocations.sol format 1`] = `
+====================================options=====================================
+parsers: ["solidity-parse"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.6;
+
+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.
+  constructor () emptyParams noParams() ModifierDefinitions() {}
+
+  // We remove parentheses in modifiers without arguments.
+  function test() public emptyParams noParams() {}
+}
+
+=====================================output=====================================
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.6;
+
+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.
+    constructor() emptyParams noParams() ModifierDefinitions() {}
+
+    // We remove parentheses in modifiers without arguments.
+    function test() public emptyParams noParams {}
+}
+
+================================================================================
+`;