Proposal to avoid separating function definitions without an implemen…

…tation (#941)

* Proposal for avoiding separation between function definitions without an implementation

* compatibility with Prettier 2

* refactor backward compatibility utils

* moving isPrettier2 to the backwards compatibility module

* reorganising the logic for clarity and to avoid having to check for extra lines added by mistake at the end

* to avoid conflicts with #912 we changed isPrettier2 from a function to a constant checked at the loading of the module

* Adding documentation

* only check for leading comments

.c8rc

@@ -5,7 +5,11 @@
   "functions": 100,
   "statements": 100,
   "exclude": ["/node_modules/", "/src/prettier-comments/"],
-  "include": ["src/**/*.js", "!src/prettier-comments/**/*.js"],
+  "include": [
+    "src/**/*.js",
+    "!src/prettier-comments/**/*.js",
+    "!src/common/backward-compatibility.js"
+  ],
   "reporter": ["lcov", "text"],
   "temp-dir": "./coverage/"
 }

src/comments/handlers/handleContractDefinitionComments.js

@@ -1,5 +1,5 @@
 import { util } from 'prettier';
-import { getNextNonSpaceNonCommentCharacter } from '../../common/util.js';
+import { getNextNonSpaceNonCommentCharacter } from '../../common/backward-compatibility.js';
 
 const { addLeadingComment, addTrailingComment, addDanglingComment } = util;
 

src/comments/handlers/handleModifierInvocationComments.js

@@ -1,5 +1,5 @@
 import { util } from 'prettier';
-import { getNextNonSpaceNonCommentCharacter } from '../../common/util.js';
+import { getNextNonSpaceNonCommentCharacter } from '../../common/backward-compatibility.js';
 
 const { addLeadingComment, addTrailingComment, addDanglingComment } = util;
 

src/common/backward-compatibility.js

@@ -0,0 +1,46 @@
+import { util } from 'prettier';
+import { prettierVersionSatisfies } from './util.js';
+
+export const isPrettier2 = prettierVersionSatisfies('^2.3.0');
+
+// The functions in this file will never be 100% covered in a single run
+// since it depends on the version of Prettier being used.
+// Mocking the behaviour will introduce a lot of maintenance in the tests.
+
+export function isNextLineEmpty(text, startIndex) {
+  return isPrettier2
+    ? util.isNextLineEmptyAfterIndex(text, startIndex)
+    : util.isNextLineEmpty(text, startIndex); // V3 deprecated `isNextLineEmptyAfterIndex`
+}
+
+export function getNextNonSpaceNonCommentCharacterIndex(text, node, locEnd) {
+  return isPrettier2
+    ? util.getNextNonSpaceNonCommentCharacterIndex(text, node, locEnd)
+    : util.getNextNonSpaceNonCommentCharacterIndex(text, locEnd(node)); // V3 signature changed
+}
+
+export function getNextNonSpaceNonCommentCharacter(text, node, locEnd) {
+  return isPrettier2
+    ? text.charAt(
+        util.getNextNonSpaceNonCommentCharacterIndex(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,5 +1,12 @@
 import { doc } from 'prettier';
-import { isNextLineEmpty, isPrettier2 } from './util.js';
+import {
+  isFirst,
+  isLast,
+  isNextLineEmpty,
+  isPrettier2,
+  next,
+  previous
+} from './backward-compatibility.js';
 
 const { group, indent, join, line, softline, hardline } = doc.builders;
 
@@ -26,12 +33,28 @@ export const printComments = (node, path, options, filter = () => true) => {
   // since it depends on the version of Prettier being used.
   // Mocking the behaviour will introduce a lot of maintenance in the tests.
   /* c8 ignore start */
-  return isPrettier2()
+  return isPrettier2
     ? document.parts // Prettier V2
     : document; // Prettier V3
   /* 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) => {
@@ -48,29 +71,41 @@ export function printPreservingEmptyLines(path, key, options, print) {
       parts.push(hardline);
     }
 
-    if (index > 0) {
-      if (
-        ['ContractDefinition', 'FunctionDefinition'].includes(nodeType) &&
-        parts[parts.length - 2] !== 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));
 
-    if (
-      isNextLineEmpty(options.originalText, options.locEnd(node) + 1) ||
-      nodeType === 'FunctionDefinition'
-    ) {
-      parts.push(hardline);
+    // 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);
+      }
     }
   }, key);
 
-  if (parts.length > 1 && parts[parts.length - 1] === hardline) {
-    parts.pop();
-  }
-
   return parts;
 }
 

src/common/util.js

@@ -2,32 +2,6 @@ import { util, version } from 'prettier';
 import satisfies from 'semver/functions/satisfies.js';
 
 export const prettierVersionSatisfies = (range) => satisfies(version, range);
-export const isPrettier2 = () => prettierVersionSatisfies('^2.3.0');
-
-// The following functions will never be 100% covered in a single run
-// since it depends on the version of Prettier being used.
-// Mocking the behaviour will introduce a lot of maintenance in the tests.
-/* c8 ignore start */
-export function isNextLineEmpty(text, startIndex) {
-  return isPrettier2()
-    ? util.isNextLineEmptyAfterIndex(text, startIndex)
-    : util.isNextLineEmpty(text, startIndex); // V3 deprecated `isNextLineEmptyAfterIndex`
-}
-
-export function getNextNonSpaceNonCommentCharacterIndex(text, node, locEnd) {
-  return isPrettier2()
-    ? util.getNextNonSpaceNonCommentCharacterIndex(text, node, locEnd)
-    : util.getNextNonSpaceNonCommentCharacterIndex(text, locEnd(node)); // V3 signature changed
-}
-
-export function getNextNonSpaceNonCommentCharacter(text, node, locEnd) {
-  return isPrettier2()
-    ? text.charAt(
-        util.getNextNonSpaceNonCommentCharacterIndex(text, node, locEnd)
-      )
-    : util.getNextNonSpaceNonCommentCharacter(text, locEnd(node)); // V3 exposes this function directly
-}
-/* c8 ignore stop */
 
 export function printString(rawContent, options) {
   const double = { quote: '"', regex: /"/g };

src/nodes/FunctionDefinition.js

@@ -1,5 +1,5 @@
 import { doc } from 'prettier';
-import { getNextNonSpaceNonCommentCharacter } from '../common/util.js';
+import { getNextNonSpaceNonCommentCharacter } from '../common/backward-compatibility.js';
 import {
   printComments,
   printSeparatedItem,

src/prettier-comments/language-js/comments.js

@@ -1,5 +1,5 @@
 import { util } from "prettier";
-import { getNextNonSpaceNonCommentCharacter } from "../../common/util.js";
+import { getNextNonSpaceNonCommentCharacter } from "../../common/backward-compatibility.js";
 
 const {
   addLeadingComment,