From dfff3697c20347e44c860c411c2c000e4fb34c53 Mon Sep 17 00:00:00 2001 From: Bryce Osterhaus Date: Mon, 20 May 2024 11:44:04 +0400 Subject: [PATCH] fix(prettier-plugin): add rules for if/else new line and fix issues when formatting in portal --- projects/prettier-plugin/options.mjs | 19 +- projects/prettier-plugin/parsers.mjs | 64 ++-- .../rules/lines-around-comments.mjs | 199 ++++++------ .../rules/newline-before-block-statements.mjs | 130 ++++++++ ...est.mjs => lines-around-comments.test.mjs} | 53 +++- .../newline-before-block-statements.test.mjs | 293 ++++++++++++++++++ projects/prettier-plugin/utils/visitNode.mjs | 44 +++ 7 files changed, 683 insertions(+), 119 deletions(-) create mode 100644 projects/prettier-plugin/rules/newline-before-block-statements.mjs rename projects/prettier-plugin/test/{index.test.mjs => lines-around-comments.test.mjs} (82%) create mode 100644 projects/prettier-plugin/test/newline-before-block-statements.test.mjs create mode 100644 projects/prettier-plugin/utils/visitNode.mjs diff --git a/projects/prettier-plugin/options.mjs b/projects/prettier-plugin/options.mjs index 6ad28b8a3..975df7f61 100644 --- a/projects/prettier-plugin/options.mjs +++ b/projects/prettier-plugin/options.mjs @@ -4,10 +4,27 @@ */ export const options = { + commentIgnoreAfterPatterns: { + array: true, + category: 'Format', + description: + 'Ignore line after comment node that contains a given pattern', + since: '1.0.0', + type: 'string', + }, + commentIgnoreBeforePatterns: { + array: true, + category: 'Format', + description: + 'Ignore new line before comment node that contains a given pattern', + since: '1.0.0', + type: 'string', + }, commentIgnorePatterns: { array: true, category: 'Format', - description: 'Ignore comment nodes that contain a given pattern', + description: + 'Ignore new lines line before and after comment nodes that contain a given pattern', since: '1.0.0', type: 'string', }, diff --git a/projects/prettier-plugin/parsers.mjs b/projects/prettier-plugin/parsers.mjs index c7508772f..eb95b0e51 100644 --- a/projects/prettier-plugin/parsers.mjs +++ b/projects/prettier-plugin/parsers.mjs @@ -8,37 +8,51 @@ import {parsers as babelParsers} from 'prettier/plugins/babel'; import {parsers as typescriptParsers} from 'prettier/plugins/typescript'; import {linesAroundComments} from './rules/lines-around-comments.mjs'; +import {newlineBeforeBlockStatement} from './rules/newline-before-block-statements.mjs'; function transformParser(parserName, defaultParser) { return { ...defaultParser, astFormat: 'liferay-style-ast', parse: async (text, options) => { - /* - * We need to filter out our own plugin before calling default prettier - */ - const plugins = options.plugins.filter( - (plugin) => !plugin.printers['liferay-style-ast'] - ); - - let formattedText = await format(text, { - ...options, - plugins, - }); - - const ast = defaultParser.parse(formattedText, options); - - formattedText = linesAroundComments( - formattedText, - ast, - parserName, - options - ); - - return { - body: formattedText, - type: 'FormattedText', - }; + try { + /* + * We need to filter out our own plugin before calling default prettier + */ + const plugins = options.plugins.filter( + (plugin) => !plugin.printers['liferay-style-ast'] + ); + + let formattedText = await format(text, { + ...options, + plugins, + }); + + let ast = defaultParser.parse(formattedText, options); + + formattedText = linesAroundComments( + formattedText, + ast, + parserName, + options + ); + + ast = defaultParser.parse(formattedText, options); + + formattedText = newlineBeforeBlockStatement( + formattedText, + ast, + parserName, + options + ); + + return { + body: formattedText, + type: 'FormattedText', + }; + } catch (err) { + console.log('ERROR', options.filepath, err); + } }, }; } diff --git a/projects/prettier-plugin/rules/lines-around-comments.mjs b/projects/prettier-plugin/rules/lines-around-comments.mjs index 5fb08532e..d18d6a5d5 100644 --- a/projects/prettier-plugin/rules/lines-around-comments.mjs +++ b/projects/prettier-plugin/rules/lines-around-comments.mjs @@ -4,12 +4,15 @@ */ export function linesAroundComments(formattedText, ast, parserName, options) { - const {commentIgnorePatterns = []} = options; + const { + commentIgnorePatterns = [], + commentIgnoreAfterPatterns = [], + commentIgnoreBeforePatterns = [], + } = options; + const totalLines = ast.loc.end.line; let hasDirective = false; - let linesAdded = 0; - const ignoredLines = []; /* * Track where each inline comment is so that we can group them @@ -18,12 +21,13 @@ export function linesAroundComments(formattedText, ast, parserName, options) { (inlineComments, commentNode) => { if ( isInlineComment(commentNode) && - !isEndofLineComment(commentNode, formattedText, parserName) + !isEndofLineComment(commentNode, formattedText) ) { - /* - * Subtract '1' to make it zero based counting - */ - inlineComments.push(commentNode.loc.start.line - 1); + inlineComments.push(commentNode.loc.start.line); + } + + if (isDirective(commentNode, formattedText, parserName)) { + hasDirective = true; } return inlineComments; @@ -31,63 +35,78 @@ export function linesAroundComments(formattedText, ast, parserName, options) { [] ); + let modifiedText = formattedText; + + const ignorePatterns = [ + ...[...commentIgnorePatterns, '/ { + const regex = new RegExp(item); + + return { + afterPattern: regex, + beforePattern: regex, + }; + }), + ...[ + ...commentIgnoreAfterPatterns, + 'eslint-disable', + 'prettier-ignore', + '@ts-ignore', + 'webpackIgnore: true', + ].map((item) => ({ + afterPattern: new RegExp(item), + })), + ...commentIgnoreBeforePatterns.map((item) => ({ + beforePattern: new RegExp(item), + })), + ]; + /* - * Normalizes our content into an array of strings. Each item represents a single - * line of the source file. An empty item in the array would signify a new line. + * We are reading these comments from a "bottom to top" approach. */ - let formattedTextByLines = formattedText.split('\n'); + for (let i = ast.comments.length - 1; i >= 0; i--) { + const commentNode = ast.comments[i]; + + let skipAfter = false; + let skipBefore = false; - ast.comments.forEach((commentNode) => { /* - * Ignore if comment node value matches option + * Ignore new line above comment node value matches option */ - if ( - commentIgnorePatterns.find((pattern) => { - const regex = new RegExp(pattern); + ignorePatterns.forEach(({afterPattern, beforePattern}) => { + if (afterPattern?.exec(commentNode.value)) { + skipAfter = true; + } - return regex.exec(commentNode.value); - }) - ) { - return; + if (beforePattern?.exec(commentNode.value)) { + skipBefore = true; + } + }); + + if (skipAfter && skipBefore) { + continue; } /* * Ignore comments that are at the end of a line */ - if (isEndofLineComment(commentNode, formattedText, parserName)) { - return; + if (isEndofLineComment(commentNode, modifiedText)) { + continue; } /* * Skip directives */ - if (isDirective(commentNode, formattedText, parserName)) { - hasDirective = true; - - return; + if (isDirective(commentNode, modifiedText, parserName)) { + continue; } - /* - * Subtract '1' to make it zero based counting - */ - const startingLine = commentNode.loc.start.line - 1; - const endingLine = commentNode.loc.end.line - 1; - - let skipAfter = false; - let skipBefore = false; - - /* - * Don't add a line after if the comment is for eslint - */ - if (commentNode.value.includes('eslint-disable')) { - ignoredLines.push(endingLine + 1); - skipAfter = true; - } + const startingLine = commentNode.loc.start.line; + const endingLine = commentNode.loc.end.line; /* * Don't add a line after if the comment is at the end of the file */ - if (endingLine === totalLines) { + if (endingLine === totalLines - 1) { skipAfter = true; } @@ -95,14 +114,8 @@ export function linesAroundComments(formattedText, ast, parserName, options) { * Don't add a line before if its the first line in the file * or * Don't add a line before if the line above is a directive - * or - * Don't add a line before if the line was ignored by a previous comment */ - if ( - startingLine === 0 || - (hasDirective && startingLine === 1) || - ignoredLines.includes(startingLine) - ) { + if (startingLine === 1 || (hasDirective && startingLine === 2)) { skipBefore = true; } @@ -114,39 +127,29 @@ export function linesAroundComments(formattedText, ast, parserName, options) { skipBefore = true; } - if (linesWithInlineComment.includes(startingLine + 1)) { + if (linesWithInlineComment.includes(endingLine + 1)) { skipAfter = true; } } - const startLine = startingLine + linesAdded; - - if (!isNewLineBefore(formattedTextByLines, startLine) && !skipBefore) { - formattedTextByLines = insertNewLine( - formattedTextByLines, - startLine - ); - - linesAdded += 1; - } - - const endLine = endingLine + linesAdded; - + /* + * Since we are traversing from "bottom to top" of the file, we need + * want to check the 'after' line first. + */ if ( + !skipAfter && !isBlockComment(commentNode) && - !isNewLineAfter(formattedTextByLines, endLine) && - !skipAfter + !isNewLineAfter(commentNode, modifiedText) ) { - formattedTextByLines = insertNewLine( - formattedTextByLines, - endLine + 1 - ); + modifiedText = insertNewLineAfter(commentNode, modifiedText); + } - linesAdded += 1; + if (!skipBefore && !isNewLineBefore(commentNode, modifiedText)) { + modifiedText = insertNewLineBefore(commentNode, modifiedText); } - }); + } - return formattedTextByLines.join('\n'); + return modifiedText; } function isDirective(node, text, parserName) { @@ -171,15 +174,10 @@ function isInlineComment(node) { /* * Returns The contents on the same line before the comment starts */ -function getContentsBeforeColumn(node, source, parserName) { +function getContentsBeforeColumn(node, source) { const {column} = node.loc.start; - let index; - if (parserName === 'typescript') { - index = node.range[0]; - } else { - index = node.loc.start.index; - } + const index = node.start ?? node.range[0]; return source.slice(index - column, index - 1); } @@ -187,21 +185,46 @@ function getContentsBeforeColumn(node, source, parserName) { /* * A comment like `var test = 'foo'; // this is my variable` */ -function isEndofLineComment(node, source, parserName) { +function isEndofLineComment(node, source) { return ( node.loc.start.column !== 0 && - /\S/.test(getContentsBeforeColumn(node, source, parserName)) + /\S/.test(getContentsBeforeColumn(node, source)) ); } -function isNewLineBefore(textArray, index) { - return textArray[index - 1] === ''; +function isNewLineBefore(commentNode, content) { + const startingIndex = (commentNode.start ?? commentNode.range[0]) - 1; + const indentSize = commentNode.loc.start.column; + + return ( + content.charAt(startingIndex - indentSize) === '\n' && + content.charAt(startingIndex - indentSize - 1) === '\n' + ); } -function isNewLineAfter(textArray, index) { - return textArray[index + 1] === ''; +function isNewLineAfter(commentNode, content) { + const endindex = commentNode.end ?? commentNode.range[1]; + + return ( + content.charAt(endindex) === '\n' && + content.charAt(endindex + 1) === '\n' + ); } -function insertNewLine(textArray, index) { - return [...textArray.slice(0, index), '', ...textArray.slice(index)]; +function insertNewLineAfter(commentNode, content) { + const endingIndex = commentNode.end ?? commentNode.range[1]; + + const insertIndex = endingIndex + 1; + + return content.slice(0, insertIndex) + '\n' + content.slice(insertIndex); +} + +function insertNewLineBefore(commentNode, content) { + const startingIndex = commentNode.start ?? commentNode.range[0]; + + const indentSize = commentNode.loc.start.column; + + const insertIndex = startingIndex - indentSize; + + return content.slice(0, insertIndex) + '\n' + content.slice(insertIndex); } diff --git a/projects/prettier-plugin/rules/newline-before-block-statements.mjs b/projects/prettier-plugin/rules/newline-before-block-statements.mjs new file mode 100644 index 000000000..4bf543c1b --- /dev/null +++ b/projects/prettier-plugin/rules/newline-before-block-statements.mjs @@ -0,0 +1,130 @@ +/** + * SPDX-FileCopyrightText: © 2020 Liferay, Inc. + * SPDX-License-Identifier: MIT + */ + +import visitNode from '../utils/visitNode.mjs'; + +export function newlineBeforeBlockStatement(formattedText, ast) { + let modifiedText = formattedText; + + const linesAdded = {}; + + visitNode(ast, (node) => { + if (node.type === 'IfStatement') { + /* + * } else if { + * consequent ^ ^ alternate + * + * } else { + * consequent ^ ^ alternate + */ + + const {alternate, consequent} = node; + + if (alternate) { + const alternateStartingLine = alternate.loc.start.line; + const consequentEndingLine = consequent.loc.end.line; + + if (alternateStartingLine === consequentEndingLine) { + modifiedText = insertNewLineAfter( + consequent, + modifiedText, + linesAdded + ); + } + } + } + + if (node.type === 'TryStatement') { + /* + * } catch (error) { + * block--^ ^--handler + * + * or: + * + * } catch { + * block--^ ^--handler + * + * or: + * + * } finally { + * block-or-handler ^ ^ finalizer + */ + const {block, finalizer, handler} = node; + + /* + * We want to make sure and replace the finalizer before the + * handler since we are working in a "bottom-up" traversal. + */ + if (finalizer) { + const nodeAbove = handler || block; + const nodeAboveEndingLine = nodeAbove.loc.end.line; + const finalizerStartLine = finalizer.loc.start.line; + + if (nodeAboveEndingLine === finalizerStartLine) { + modifiedText = insertNewLineAfter( + nodeAbove, + modifiedText, + linesAdded + ); + } + } + + if (handler) { + const blockEndingLine = block.loc.end.line; + const handlerStartLine = handler.loc.start.line; + + if (blockEndingLine === handlerStartLine) { + modifiedText = insertNewLineAfter( + block, + modifiedText, + linesAdded + ); + } + } + } + }); + + return modifiedText; +} + +function insertNewLineAfter(nodeAbove, content, linesAdded) { + const aboveEndingIndex = nodeAbove.range[1]; + const endingLine = nodeAbove.loc.end.line; + + /* + * Determine the index offset from the number of characters added above. + */ + const offset = Object.keys(linesAdded).reduce((acc, lineNumber) => { + if (lineNumber < endingLine) { + acc += linesAdded[lineNumber]; + } + + return acc; + }, 0); + + const indentSize = '\t'.repeat(nodeAbove.loc.end.column - 1); + + /* + * Keep track of the number of characters added at each line. + */ + linesAdded[endingLine] = indentSize.length; + + /* + * We know we are working with prettier's standard output, which means + * we can assume the literal space(' ') between the closing bracket and + * the keyword. '} else' + * ^--- this space + */ + const spaceOffset = 1; + + const index = aboveEndingIndex + offset; + + return ( + content.slice(0, index) + + '\n' + + indentSize + + content.slice(index + spaceOffset) + ); +} diff --git a/projects/prettier-plugin/test/index.test.mjs b/projects/prettier-plugin/test/lines-around-comments.test.mjs similarity index 82% rename from projects/prettier-plugin/test/index.test.mjs rename to projects/prettier-plugin/test/lines-around-comments.test.mjs index 0dbd21fa5..66d429673 100644 --- a/projects/prettier-plugin/test/index.test.mjs +++ b/projects/prettier-plugin/test/lines-around-comments.test.mjs @@ -66,6 +66,22 @@ const bar = 'bar';`, // foo // bar +const bar = 'bar'; +`, + }, + { + _name: 'doesnt format already correct code', + code: `const foo = 'foo'; + +// foo +// bar + +const bar = 'bar';`, + expected: `const foo = 'foo'; + +// foo +// bar + const bar = 'bar'; `, }, @@ -124,6 +140,7 @@ return false; { _name: 'disable line after if it is an "eslint-disable" comment', code: `if (true) { + // eslint-disable-next-line var foo = 'test'; // foo return false; @@ -139,6 +156,7 @@ return false; { _name: 'disable line after if it is an "eslint-disable" block comment', code: `if (true) { + /* eslint-disable */ var foo = 'test'; // foo return false; @@ -153,11 +171,10 @@ return false; }, { _name: 'ignore triple slash references in TS', - code: `/* eslint-disable */ -/// -/// `, - expected: `/* eslint-disable */ -/// + code: `/// +/// +`, + expected: `/// /// `, }, @@ -208,6 +225,32 @@ var foo = 'bar'; // mylint-disable var bar = 'foo'; +`, + }, + { + _config: { + commentIgnoreAfterPatterns: ['disable-line-after'], + commentIgnoreBeforePatterns: ['disable-line-before'], + commentIgnorePatterns: ['disable-both-lines'], + }, + _name: 'respects commentIgnorePatterns option with regex 3', + code: `var foo = 'bar'; +// disable-both-lines +var bar = 'foo'; +// disable-line-after +var test = 'test'; +// disable-line-before +var baz = 'baz'; +`, + expected: `var foo = 'bar'; +// disable-both-lines +var bar = 'foo'; + +// disable-line-after +var test = 'test'; +// disable-line-before + +var baz = 'baz'; `, }, ]; diff --git a/projects/prettier-plugin/test/newline-before-block-statements.test.mjs b/projects/prettier-plugin/test/newline-before-block-statements.test.mjs new file mode 100644 index 000000000..8e7414a99 --- /dev/null +++ b/projects/prettier-plugin/test/newline-before-block-statements.test.mjs @@ -0,0 +1,293 @@ +/** + * SPDX-FileCopyrightText: © 2020 Liferay, Inc. + * SPDX-License-Identifier: MIT + */ + +import assert from 'node:assert'; +import {describe, test} from 'node:test'; +import {format} from 'prettier'; + +import * as liferayPrettierPlugin from '../index.mjs'; + +const baseConfig = { + bracketSpacing: false, + plugins: [liferayPrettierPlugin], + singleQuote: true, + tabWidth: 4, + useTabs: true, +}; + +const babelConfig = { + ...baseConfig, + parser: 'babel', +}; + +const tsConfig = { + ...baseConfig, + parser: 'typescript', +}; + +const fixtures = [ + { + _name: 'if/else newline', + code: `if (true) { +} else { + +}`, + expected: `if (true) { +} +else { +} +`, + }, + { + _name: 'if/elseif/else newline', + code: `if (foo) { + if (foo) { + } else if (bar) { + } else if (baz) { + } else { + } +} else {} +`, + expected: `if (foo) { + if (foo) { + } + else if (bar) { + } + else if (baz) { + } + else { + } +} +else { +} +`, + }, + { + _name: 'multiple nested if/else', + code: `if (1) { + if (2) { + if (3) { + } else {} + } else if (4) { + if (5) { + } else {} + } + if (6) { + } else {} +} +`, + expected: `if (1) { + if (2) { + if (3) { + } + else { + } + } + else if (4) { + if (5) { + } + else { + } + } + if (6) { + } + else { + } +} +`, + }, + { + _name: 'try/catch newline', + code: `if (true) { + try { + } catch { + } +} +`, + expected: `if (true) { + try { + } + catch {} +} +`, + }, + { + _name: 'try/finally newline', + code: `try { +} finally { +}`, + expected: `try { +} +finally { +} +`, + }, + { + _name: 'try/catch/finally newline1', + code: `try { +} catch {} finally { +}`, + expected: `try { +} +catch { +} +finally { +} +`, + }, + { + _name: 'try/catch/finally newline2', + code: `try { + } catch {} + +try { +} catch {} finally { +}`, + expected: `try { +} +catch {} + +try { +} +catch { +} +finally { +} +`, + }, + { + _name: 'kitchen sink', + code: `if (true) { + try { + } catch { + } + + if (true) { + } else { + } + + try { + } catch {} + + try { + } catch {} + finally {} +} +`, + expected: `if (true) { + try { + } + catch {} + + if (true) { + } + else { + } + + try { + } + catch {} + + try { + } + catch { + } + finally { + } +} +`, + }, + { + _name: 'long conditional for if/else', + code: `if ( + AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA && BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB +) { +} else { +} +`, + expected: `if ( + AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA && + BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB +) { +} +else { +} +`, + }, + + { + _name: 'Works when the file contains a literal newline character', + code: `if (true) {var foo = '\\n'}`, + expected: `if (true) { + var foo = '\\n'; +} +`, + }, + + { + _name: 'Works with "else if"', + code: `if (true) { +} else if (false) { +} +`, + expected: `if (true) { +} +else if (false) { +} +`, + }, + + { + _name: 'Works with "else if"', + code: `if (1) {} else if (2) {} else if (3) {}`, + expected: `if (1) { +} +else if (2) { +} +else if (3) { +} +`, + }, + { + _name: 'Works with "else if"', + code: `if (1) {} else if (2) {} else if (3) {}`, + expected: `if (1) { +} +else if (2) { +} +else if (3) { +} +`, + }, +]; + +describe('babel', () => { + fixtures.forEach((fixture) => { + const {_config = {}, _name, code, expected, ...testConfig} = fixture; + + test(_name, testConfig, async () => { + assert.equal( + await format(code, { + ...babelConfig, + ..._config, + }), + expected + ); + }); + }); +}); + +describe('typescript', () => { + fixtures.forEach((fixture) => { + const {_config = {}, _name, code, expected, ...testConfig} = fixture; + + test(_name, testConfig, async () => { + assert.equal( + await format(code, {...tsConfig, ..._config}), + expected + ); + }); + }); +}); diff --git a/projects/prettier-plugin/utils/visitNode.mjs b/projects/prettier-plugin/utils/visitNode.mjs new file mode 100644 index 000000000..850464529 --- /dev/null +++ b/projects/prettier-plugin/utils/visitNode.mjs @@ -0,0 +1,44 @@ +/** + * SPDX-FileCopyrightText: © 2020 Liferay, Inc. + * SPDX-License-Identifier: MIT + */ + +import {VISITOR_KEYS as babelVisitorKeys} from '@babel/types'; +import {visitorKeys as tsVisitorKeys} from '@typescript-eslint/visitor-keys'; + +const visitorKeys = { + ...babelVisitorKeys, + ...tsVisitorKeys, + PropertyDefinition: ['value'], +}; + +/* + * This function traverses the AST tree one node at a time. The callback is + * run from a "bottom-up" perspective. Meaning that it runs the callback on the + * child node before calling it on the parent node. + * + * This also benefits us because as we modify the original content of the file + * we don't have to account for the new lines added as they will only be "below" + * the node we are currently at. + */ +export default function visitNode(node, fn) { + if (!(node !== null && typeof node === 'object')) { + return node; + } + + if (Array.isArray(node)) { + for (let i = node.length - 1; i >= 0; i--) { + node[i] = visitNode(node[i], fn); + } + + return node; + } + + const keys = visitorKeys[node.type] || []; + + for (let i = keys.length - 1; i >= 0; i--) { + node[keys[i]] = visitNode(node[keys[i]], fn); + } + + return fn(node) || node; +}