diff --git a/changelog_unreleased/javascript/15826.md b/changelog_unreleased/javascript/15826.md new file mode 100644 index 000000000..87f79f32e --- /dev/null +++ b/changelog_unreleased/javascript/15826.md @@ -0,0 +1,27 @@ +#### Fix comments in `if` (#15826 by @fisker) + + +```js +// Input +if (foo) for (i = 2; i > 0; i--) console.log(i); // comment 1 +else bar(); + +for (;;){ + if (foo) continue; // comment 2 + else bar(); +} + +// Prettier stable +Error: Comment "comment 2" was not printed. Please report this error! + +// Prettier main +if (foo) + for (i = 2; i > 0; i--) console.log(i); // comment 1 +else bar(); + +for (;;) { + if (foo) + continue; // comment 2 + else bar(); +} +``` diff --git a/src/language-js/comments/handle-comments.js b/src/language-js/comments/handle-comments.js index 6b42ad412..8ce2336d3 100644 --- a/src/language-js/comments/handle-comments.js +++ b/src/language-js/comments/handle-comments.js @@ -21,7 +21,6 @@ import { isObjectProperty, isPrettierIgnoreComment, isUnionType, - markerForIfWithoutBlockAndSameLineComment, } from "../utils/index.js"; import isBlockComment from "../utils/is-block-comment.js"; import isTypeCastComment from "../utils/is-type-cast-comment.js"; @@ -202,7 +201,7 @@ function handleIfStatementComments({ addTrailingComment(precedingNode, comment); } else { const isSingleLineComment = - comment.type === "SingleLine" || + isLineComment(comment) || comment.loc.start.line === comment.loc.end.line; const isSameLineComment = comment.loc.start.line === precedingNode.loc.start.line; @@ -211,13 +210,7 @@ function handleIfStatementComments({ // if (cond1) expr1; // comment A // else if (cond2) expr2; // comment A // else expr3; - addDanglingComment( - precedingNode, - comment, - precedingNode.type === "ExpressionStatement" - ? markerForIfWithoutBlockAndSameLineComment - : undefined, - ); + addTrailingComment(precedingNode, comment); } else { addDanglingComment(enclosingNode, comment); } diff --git a/src/language-js/print/estree.js b/src/language-js/print/estree.js index a9cb5e603..54b205893 100644 --- a/src/language-js/print/estree.js +++ b/src/language-js/print/estree.js @@ -371,12 +371,12 @@ function printEstree(path, options, print, args) { adjustClause(node.body, print("body")), ]); case "IfStatement": { - const con = adjustClause(node.consequent, print("consequent")); + const consequent = adjustClause(node.consequent, print("consequent")); const opening = group([ "if (", group([indent([softline, print("test")]), softline]), ")", - con, + consequent, ]); parts.push(opening); diff --git a/src/language-js/print/expression-statement.js b/src/language-js/print/expression-statement.js index f2794c703..006d6c643 100644 --- a/src/language-js/print/expression-statement.js +++ b/src/language-js/print/expression-statement.js @@ -1,9 +1,3 @@ -import { printDanglingComments } from "../../main/comments/print.js"; -import { - CommentCheckFlags, - hasComment, - markerForIfWithoutBlockAndSameLineComment, -} from "../utils/index.js"; import { isSingleJsxExpressionStatementInMarkdown, isSingleVueEventBindingExpressionStatement, @@ -23,21 +17,6 @@ function printExpressionStatement(path, options, print) { parts.push(";"); } - if ( - hasComment( - path.node, - CommentCheckFlags.Dangling, - ({ marker }) => marker === markerForIfWithoutBlockAndSameLineComment, - ) - ) { - parts.push( - " ", - printDanglingComments(path, options, { - marker: markerForIfWithoutBlockAndSameLineComment, - }), - ); - } - return parts; } diff --git a/src/language-js/utils/index.js b/src/language-js/utils/index.js index 50f89ff29..b3bfe1646 100644 --- a/src/language-js/utils/index.js +++ b/src/language-js/utils/index.js @@ -1197,13 +1197,6 @@ function isObjectProperty(node) { ); } -/** - * This is used as a marker for dangling comments. - */ -const markerForIfWithoutBlockAndSameLineComment = Symbol( - "ifWithoutBlockAndSameLineComment", -); - const isBinaryCastExpression = createTypeCheckFunction([ // TS "TSAsExpression", @@ -1284,7 +1277,6 @@ export { isUnionType, iterateCallArgumentsPath, iterateFunctionParametersPath, - markerForIfWithoutBlockAndSameLineComment, needsHardlineAfterDanglingComment, rawText, shouldFlatten, diff --git a/tests/config/format-test.js b/tests/config/format-test.js index f90a9b689..84c949276 100644 --- a/tests/config/format-test.js +++ b/tests/config/format-test.js @@ -46,6 +46,7 @@ const unstableTests = new Map( "jsx/comments/in-attributes.js", ["js/ignore/semi/asi.js", (options) => options.semi === false], "typescript/union/consistent-with-flow/single-type.ts", + "js/if/non-block.js", ].map((fixture) => { const [file, isUnstable = () => true] = Array.isArray(fixture) ? fixture diff --git a/tests/format/js/if/__snapshots__/jsfmt.spec.js.snap b/tests/format/js/if/__snapshots__/jsfmt.spec.js.snap index 0c6146c4d..0f7ac1910 100644 --- a/tests/format/js/if/__snapshots__/jsfmt.spec.js.snap +++ b/tests/format/js/if/__snapshots__/jsfmt.spec.js.snap @@ -123,25 +123,28 @@ function a() { if (a) throw e; /* comment 6d */ else return 2; - // TODO[@fisker]: fix this - // if (a) var a = 1; /* comment 6e */ - // else return 2; + if (a) var a = 1; /* comment 6e */ + else return 2; if (a) if (b); /* comment 6f */ else return 2; } =====================================output===================================== -if (a === 0) doSomething(); // comment A1 -else if (a === 1) doSomethingElse(); // comment B1 +if (a === 0) + doSomething(); // comment A1 +else if (a === 1) + doSomethingElse(); // comment B1 else if (a === 2) doSomethingElse(); // comment C1 if (a === 0) doSomething(); /* comment A2 */ else if (a === 1) doSomethingElse(); /* comment B2 */ else if (a === 2) doSomethingElse(); /* comment C2 */ -if (a === 0) expr; // comment A3 -else if (a === 1) expr; // comment B3 +if (a === 0) + expr; // comment A3 +else if (a === 1) + expr; // comment B3 else if (a === 2) expr; // comment C3 if (a === 0) expr; /* comment A4 */ @@ -156,18 +159,17 @@ else if (a === 2) looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong; // comment C5 function a() { - if (a) return /* comment 6a */; + if (a) return; /* comment 6a */ else return 2; - if (a) return 1 /* comment 6b */; + if (a) return 1; /* comment 6b */ else return 2; - if (a) throw e /* comment 6d */; + if (a) throw e; /* comment 6d */ else return 2; - // TODO[@fisker]: fix this - // if (a) var a = 1; /* comment 6e */ - // else return 2; + if (a) var a = 1; /* comment 6e */ + else return 2; if (a) if (b /* comment 6f */); @@ -294,6 +296,71 @@ async function f5() { ================================================================================ `; +exports[`issue-15168.js format 1`] = ` +====================================options===================================== +parsers: ["babel", "flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +for (const division of collidingDivisions) { + if (!division.properties.canRemove) { + if (division.canContainSingleParagraph(startParagraph)) continue; // selection starting in r/o div: always OK to delete + else if (endParagraph !== division.endParagraph && division.canContainSingleParagraph(endParagraph)) continue; // selection ending in r/o div: OK to delete IF it is NOT last paragraph (which would get absorbed) + else { // selection over whole r/o div OR ending in last paragraph (which would get absorbed) -> FAIL + console.warn('deleteSelectedTextAndJoinRemainingNodes(): cannot delete because of must-keep division: ' + division.properties.name); + } + } +} + +=====================================output===================================== +for (const division of collidingDivisions) { + if (!division.properties.canRemove) { + if (division.canContainSingleParagraph(startParagraph)) + continue; // selection starting in r/o div: always OK to delete + else if ( + endParagraph !== division.endParagraph && + division.canContainSingleParagraph(endParagraph) + ) + continue; // selection ending in r/o div: OK to delete IF it is NOT last paragraph (which would get absorbed) + else { + // selection over whole r/o div OR ending in last paragraph (which would get absorbed) -> FAIL + console.warn( + "deleteSelectedTextAndJoinRemainingNodes(): cannot delete because of must-keep division: " + + division.properties.name, + ); + } + } +} + +================================================================================ +`; + +exports[`non-block.js format 1`] = ` +====================================options===================================== +parsers: ["babel", "flow", "typescript"] +printWidth: 80 + | printWidth +=====================================input====================================== +if (foo) for (i = 2; i > 0; i--) console.log(i); // foo +else bar(); + +if (foo) do { console.log(i) } while(i--) // foo +else bar(); + +=====================================output===================================== +if (foo) + for (i = 2; i > 0; i--) console.log(i); // foo +else bar(); + +if (foo) + do { + console.log(i); + } while (i--); // foo +else bar(); + +================================================================================ +`; + exports[`trailing_comment.js format 1`] = ` ====================================options===================================== parsers: ["babel", "flow", "typescript"] diff --git a/tests/format/js/if/expr_and_same_line_comments.js b/tests/format/js/if/expr_and_same_line_comments.js index 93b949b8e..b316f4fa3 100644 --- a/tests/format/js/if/expr_and_same_line_comments.js +++ b/tests/format/js/if/expr_and_same_line_comments.js @@ -28,9 +28,8 @@ function a() { if (a) throw e; /* comment 6d */ else return 2; - // TODO[@fisker]: fix this - // if (a) var a = 1; /* comment 6e */ - // else return 2; + if (a) var a = 1; /* comment 6e */ + else return 2; if (a) if (b); /* comment 6f */ else return 2; diff --git a/tests/format/js/if/issue-15168.js b/tests/format/js/if/issue-15168.js new file mode 100644 index 000000000..6dc290d13 --- /dev/null +++ b/tests/format/js/if/issue-15168.js @@ -0,0 +1,9 @@ +for (const division of collidingDivisions) { + if (!division.properties.canRemove) { + if (division.canContainSingleParagraph(startParagraph)) continue; // selection starting in r/o div: always OK to delete + else if (endParagraph !== division.endParagraph && division.canContainSingleParagraph(endParagraph)) continue; // selection ending in r/o div: OK to delete IF it is NOT last paragraph (which would get absorbed) + else { // selection over whole r/o div OR ending in last paragraph (which would get absorbed) -> FAIL + console.warn('deleteSelectedTextAndJoinRemainingNodes(): cannot delete because of must-keep division: ' + division.properties.name); + } + } +} diff --git a/tests/format/js/if/non-block.js b/tests/format/js/if/non-block.js new file mode 100644 index 000000000..68c128000 --- /dev/null +++ b/tests/format/js/if/non-block.js @@ -0,0 +1,5 @@ +if (foo) for (i = 2; i > 0; i--) console.log(i); // foo +else bar(); + +if (foo) do { console.log(i) } while(i--) // foo +else bar();