Removing forced separation of functions and contracts (#960)

Author: Janther

src/common/backward-compatibility.js

@@ -27,20 +27,8 @@ export function getNextNonSpaceNonCommentCharacter(text, node, locEnd) {
     : util.getNextNonSpaceNonCommentCharacter(text, locEnd(node)); // V3 exposes this function directly
 }
 
-export function isFirst(path, _, index) {
-  return isPrettier2 ? index === 0 : path.isFirst;
-}
-
 export function isLast(path, key, index) {
   return isPrettier2
     ? index === path.getParentNode()[key].length - 1
     : path.isLast;
 }
-
-export function previous(path, key, index) {
-  return isPrettier2 ? path.getParentNode()[key][index - 1] : path.previous;
-}
-
-export function next(path, key, index) {
-  return isPrettier2 ? path.getParentNode()[key][index + 1] : path.next;
-}

src/common/printer-helpers.js

@@ -1,11 +1,8 @@
 import { doc } from 'prettier';
 import {
-  isFirst,
   isLast,
   isNextLineEmpty,
-  isPrettier2,
-  next,
-  previous
+  isPrettier2
 } from './backward-compatibility.js';
 
 const { group, indent, join, line, softline, hardline } = doc.builders;
@@ -24,7 +21,7 @@ export const printComments = (node, path, options, filter = () => true) => {
           return null;
         }
         comment.printed = true;
-        return options.printer.printComment(commentPath);
+        return options.printer.printComment(commentPath, options);
       }, 'comments')
       .filter(Boolean)
   );
@@ -39,22 +36,6 @@ export const printComments = (node, path, options, filter = () => true) => {
   /* c8 ignore stop */
 };
 
-const shouldHaveEmptyLine = (node, checkForLeading) =>
-  Boolean(
-    // if node is not FunctionDefinition, it should have an empty line
-    node.type !== 'FunctionDefinition' ||
-      // if FunctionDefinition is not abstract, it should have an empty line
-      node.body ||
-      // if FunctionDefinition has the comment we are looking for (trailing or
-      // leading), it should have an empty line
-      node.comments?.some((comment) => checkForLeading && comment.leading)
-  );
-
-const separatingLine = (firstNode, secondNode) =>
-  shouldHaveEmptyLine(firstNode, false) || shouldHaveEmptyLine(secondNode, true)
-    ? hardline
-    : '';
-
 export function printPreservingEmptyLines(path, key, options, print) {
   const parts = [];
   path.each((childPath, index) => {
@@ -71,38 +52,16 @@ export function printPreservingEmptyLines(path, key, options, print) {
       parts.push(hardline);
     }
 
-    // Only attempt to prepend an empty line if `node` is not the first item
-    // and an empty line hasn't already been appended after the previous `node`
-    if (
-      !isFirst(childPath, key, index) &&
-      parts[parts.length - 2] !== hardline
-    ) {
-      if (nodeType === 'FunctionDefinition') {
-        // Prepend FunctionDefinition with an empty line if there should be a
-        // separation with the previous `node`
-        parts.push(separatingLine(previous(childPath, key, index), node));
-      } else if (nodeType === 'ContractDefinition') {
-        // Prepend ContractDefinition with an empty line
-        parts.push(hardline);
-      }
-    }
-
     parts.push(print(childPath));
 
     // Only attempt to append an empty line if `node` is not the last item
-    if (!isLast(childPath, key, index)) {
-      if (isNextLineEmpty(options.originalText, options.locEnd(node) + 1)) {
-        // Append an empty line if the original text already had an one after
-        // the current `node`
-        parts.push(hardline);
-      } else if (nodeType === 'FunctionDefinition') {
-        // Append FunctionDefinition with an empty line if there should be a
-        // separation with the next `node`
-        parts.push(separatingLine(node, next(childPath, key, index)));
-      } else if (nodeType === 'ContractDefinition') {
-        // Append ContractDefinition with an empty line
-        parts.push(hardline);
-      }
+    if (
+      !isLast(childPath, key, index) &&
+      isNextLineEmpty(options.originalText, options.locEnd(node) + 1)
+    ) {
+      // Append an empty line if the original text already had an one after
+      // the current `node`
+      parts.push(hardline);
     }
   }, key);
 

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

@@ -23,7 +23,6 @@ pragma solidity ^0.5.2;
 contract AddressPayable {
     using Address for address payable;
     address payable[] hello;
-
     function sendSomeEth(
         address payable to,
         address payable[] memory world

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

@@ -568,7 +568,6 @@ import {symbol1 as alias, symbol2} from "File.sol";
 
 interface i {
     event ForeignEvent();
-
     function f();
 }
 
@@ -582,7 +581,6 @@ contract c {
         val3 = 1 finney; // 1 * 10 ** 15
         val4 = 1 ether; // 1 * 10 ** 18
     }
-
     uint256 val1;
     uint256 val2;
     uint256 val3;
@@ -600,22 +598,18 @@ contract test {
     function test() {
         choices = ActionChoices.GoStraight;
     }
-
     function getChoice() returns (uint d) {
         d = uint256(choices);
     }
-
     ActionChoices choices;
 }
 
 contract Base {
     function Base(uint i) {
         m_i = i;
     }
-
     uint public m_i;
 }
-
 contract Derived is Base(0) {
     function Derived(uint i) Base(i) {}
 }
@@ -653,20 +647,16 @@ contract c {
     function() returns (uint) {
         return g(8);
     }
-
     function g(uint pos) internal returns (uint) {
         setData(pos, 8);
         return getData(pos);
     }
-
     function setData(uint pos, uint value) internal {
         data[pos] = value;
     }
-
     function getData(uint pos) internal {
         return data[pos];
     }
-
     mapping(uint => uint) data;
     mapping(uint id => uint) data;
     mapping(address owner => mapping(address spender => uint amount)) allowances;
@@ -689,7 +679,6 @@ library IntegerSet {
         /// Number of stored items.
         uint size;
     }
-
     function insert(
         data storage self,
         uint value
@@ -705,27 +694,22 @@ library IntegerSet {
             return false;
         }
     }
-
     function remove(data storage self, uint value) returns (bool success) {
         uint index = self.index[value];
         if (index == 0) return false;
         delete self.index[value];
         delete self.items[index];
         self.size--;
     }
-
     function contains(data storage self, uint value) returns (bool) {
         return self.index[value] > 0;
     }
-
     function iterate_start(data storage self) returns (uint index) {
         return iterate_advance(self, 0);
     }
-
     function iterate_valid(data storage self, uint index) returns (bool) {
         return index < self.items.length;
     }
-
     function iterate_advance(
         data storage self,
         uint index
@@ -736,7 +720,6 @@ library IntegerSet {
         ) index++;
         return index;
     }
-
     function iterate_get(data storage self, uint index) returns (uint value) {
         return self.items[index];
     }
@@ -746,15 +729,13 @@ library IntegerSet {
 contract User {
     /// Just a struct holding our data.
     IntegerSet.data data;
-
     /// Insert something
     function insert(uint v) returns (uint size) {
         /// Sends \`data\` via reference, so IntegerSet can modify it.
         IntegerSet.insert(data, v);
         /// We can access members of the struct - but we should take care not to mess with them.
         return data.size;
     }
-
     /// Computes the sum of all stored data.
     function sum() returns (uint s) {
         for (
@@ -974,7 +955,6 @@ contract Ballot {
         owner(myPrice)
         returns (uint[], address myAdd, string[] names)
     {}
-
     function foobar()
         payable
         owner(myPrice)
@@ -1071,13 +1051,9 @@ contract Overrides {
         require(msg.sender == owner(), "Ownable: caller is not the owner");
         _;
     }
-
     function foo() public override {}
-
     function bar() public override(Foo) {}
-
     function baz() public override(Foo, Bar) {}
-
     function long()
         public
         override(

tests/format/Comments/Comments.sol

@@ -2,6 +2,14 @@ pragma solidity ^0.4.24;
 
 
 contract Comments1 {
+  /* solhint-disable var-name-mixedcase */
+IEIP712DomainSeparator private EIP712domainSeparator;
+bytes32 private _CACHED_DOMAIN_SEPARATOR;
+
+
+/* solhint-enable var-name-mixedcase */
+
+
     function() {
         // solhint-disable-previous-line no-empty-blocks
     }
@@ -39,6 +47,18 @@ contract Comments6 /*why the name `Comments6`*/ is Interface1/*why we used Inter
 }
 
 contract Comments7 {
+
+
+  // 1 comment before first function
+
+
+
+  // 2 comment before first function
+
+
+
+  // 3 comment before first function
+
   function someFunction(
     uint a, // the first value
     uint b, // the second value

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

@@ -10,6 +10,14 @@ pragma solidity ^0.4.24;
 
 
 contract Comments1 {
+  /* solhint-disable var-name-mixedcase */
+IEIP712DomainSeparator private EIP712domainSeparator;
+bytes32 private _CACHED_DOMAIN_SEPARATOR;
+
+
+/* solhint-enable var-name-mixedcase */
+
+
     function() {
         // solhint-disable-previous-line no-empty-blocks
     }
@@ -47,6 +55,18 @@ contract Comments6 /*why the name \`Comments6\`*/ is Interface1/*why we used Int
 }
 
 contract Comments7 {
+
+
+  // 1 comment before first function
+
+
+
+  // 2 comment before first function
+
+
+
+  // 3 comment before first function
+
   function someFunction(
     uint a, // the first value
     uint b, // the second value
@@ -149,6 +169,12 @@ contract Comments13 {
 pragma solidity ^0.4.24;
 
 contract Comments1 {
+    /* solhint-disable var-name-mixedcase */
+    IEIP712DomainSeparator private EIP712domainSeparator;
+    bytes32 private _CACHED_DOMAIN_SEPARATOR;
+
+    /* solhint-enable var-name-mixedcase */
+
     function() {
         // solhint-disable-previous-line no-empty-blocks
     }
@@ -212,6 +238,12 @@ contract Comments4 is
 }
 
 contract Comments7 {
+    // 1 comment before first function
+
+    // 2 comment before first function
+
+    // 3 comment before first function
+
     function someFunction(
         uint a, // the first value
         uint b, // the second value

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

@@ -62,7 +62,6 @@ contract FunctionCalls {
 
         Voter airbnb = Voter({ weight: 2, voted: true });
     }
-
     function verify(
         ForwardRequest calldata req,
         bytes calldata signature
@@ -215,7 +214,6 @@ contract FunctionCalls {
 
         Voter airbnb = Voter({weight: 2, voted: true});
     }
-
     function verify(
         ForwardRequest calldata req,
         bytes calldata signature