Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: hug lambdas without blocks #653

Merged
merged 1 commit into from
Jun 10, 2024
Merged

fix: hug lambdas without blocks #653

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/prettier-plugin-java/src/printers/classes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
hasLeadingComments,
hasLeadingLineComments
} from "./comments/comments-utils.js";
import { handleCommentsParameters } from "./comments/handle-comments.js";
import { builders } from "prettier/doc";
import { BaseCstPrettierPrinter } from "../base-cst-printer.js";
import {
Expand Down Expand Up @@ -569,6 +570,11 @@ export class ClassesPrettierVisitor extends BaseCstPrettierPrinter {
}

methodDeclarator(ctx: MethodDeclaratorCtx) {
const parameters = [
...(ctx.receiverParameter ?? []),
...(ctx.formalParameterList?.[0].children.formalParameter ?? [])
];
handleCommentsParameters(ctx.LBrace[0], parameters, ctx.RBrace[0]);
const identifier = printTokenWithComments(ctx.Identifier[0]);
const receiverParameter = this.visit(ctx.receiverParameter);
const formalParameterList = this.visit(ctx.formalParameterList);
Expand Down Expand Up @@ -719,6 +725,11 @@ export class ClassesPrettierVisitor extends BaseCstPrettierPrinter {
}

constructorDeclarator(ctx: ConstructorDeclaratorCtx) {
const parameters =
ctx.receiverParameter ??
ctx.formalParameterList?.[0].children.formalParameter ??
[];
handleCommentsParameters(ctx.LBrace[0], parameters, ctx.RBrace[0]);
const typeParameters = this.visit(ctx.typeParameters);
const simpleTypeName = this.visit(ctx.simpleTypeName);
const receiverParameter = this.visit(ctx.receiverParameter);
Expand Down Expand Up @@ -948,6 +959,9 @@ export class ClassesPrettierVisitor extends BaseCstPrettierPrinter {
]);
}
recordHeader(ctx: RecordHeaderCtx) {
const recordComponents =
ctx.recordComponentList?.[0].children.recordComponent ?? [];
handleCommentsParameters(ctx.LBrace[0], recordComponents, ctx.RBrace[0]);
const recordComponentList = this.visit(ctx.recordComponentList);
return putIntoBraces(
recordComponentList,
Expand Down
33 changes: 33 additions & 0 deletions packages/prettier-plugin-java/src/printers/comments/handle-comments.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { hasLeadingComments, hasTrailingComments } from "./comments-utils.js";
import {
BinaryExpressionCtx,
CstNode,
IToken,
UnaryExpressionCstNode
} from "java-parser";
Expand All @@ -10,6 +11,38 @@ export function handleCommentsBinaryExpression(ctx: BinaryExpressionCtx) {
moveExpressionTrailingCommentsToNextOperator(ctx);
}

export function handleCommentsParameters(
lBrace: IToken,
parameters: CstNode[] | IToken[],
rBrace: IToken
) {
const lBraceTrailingComments = lBrace.trailingComments;
const firstParameter = parameters.at(0);
if (lBraceTrailingComments && firstParameter) {
delete lBrace.trailingComments;
firstParameter.leadingComments = [
...lBraceTrailingComments,
...(firstParameter.leadingComments ?? [])
];
}
const lastParameter = parameters.at(-1);
const rBraceLeadingComments = rBrace.leadingComments;
if (rBraceLeadingComments) {
delete rBrace.leadingComments;
if (lastParameter) {
lastParameter.trailingComments = [
...(lastParameter.trailingComments ?? []),
...rBraceLeadingComments
];
} else {
lBrace.trailingComments = [
...(lBrace.trailingComments ?? []),
...rBraceLeadingComments
];
}
}
}

function moveOperatorLeadingCommentsToNextExpression(ctx: BinaryExpressionCtx) {
let unaryExpressionIndex = 1;
ctx.BinaryOperator?.forEach(binaryOperator => {
Expand Down
193 changes: 115 additions & 78 deletions packages/prettier-plugin-java/src/printers/expressions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,16 @@ import {
} from "java-parser/api";

import forEach from "lodash/forEach.js";
import { builders } from "prettier/doc";
import { builders, utils } from "prettier/doc";
import { BaseCstPrettierPrinter } from "../base-cst-printer.js";
import { isAnnotationCstNode } from "../types/utils.js";
import { printArgumentListWithBraces } from "../utils/index.js";
import { printTokenWithComments } from "./comments/format-comments.js";
import { handleCommentsBinaryExpression } from "./comments/handle-comments.js";
import { concat, dedent, fill, group, indent } from "./prettier-builder.js";
import {
handleCommentsBinaryExpression,
handleCommentsParameters
} from "./comments/handle-comments.js";
import { concat, dedent, group, indent, join } from "./prettier-builder.js";
import {
binary,
findDeepElementInPartsArray,
Expand All @@ -77,61 +80,70 @@ import {
sortTokens
} from "./printer-utils.js";

const { hardline, ifBreak, line, lineSuffixBoundary, softline } = builders;
const {
breakParent,
conditionalGroup,
ifBreak,
label,
line,
lineSuffixBoundary,
softline
} = builders;
const { removeLines, willBreak } = utils;

export class ExpressionsPrettierVisitor extends BaseCstPrettierPrinter {
expression(ctx: ExpressionCtx, params: any) {
return this.visitSingle(ctx, params);
const expression = this.visitSingle(ctx, params);
return params?.hug && expression.label !== undefined
? label(expression.label, expression)
: expression;
}

lambdaExpression(
ctx: LambdaExpressionCtx,
params?: { shouldBreak?: boolean }
) {
const lambdaParameters = group(this.visit(ctx.lambdaParameters, params));
lambdaExpression(ctx: LambdaExpressionCtx, params?: { hug?: boolean }) {
const lambdaParameters = group(this.visit(ctx.lambdaParameters));
const lambdaBody = this.visit(ctx.lambdaBody);

const isLambdaBodyABlock = ctx.lambdaBody[0].children.block !== undefined;
if (isLambdaBodyABlock) {
return rejectAndJoin(" ", [lambdaParameters, ctx.Arrow[0], lambdaBody]);
const suffix = [
" ",
ctx.Arrow[0],
...(isLambdaBodyABlock
? [" ", lambdaBody]
: [group(indent([line, lambdaBody]))])
];
if (params?.hug) {
return willBreak(lambdaParameters)
? label({ huggable: false }, concat([lambdaParameters, ...suffix]))
: concat([removeLines(lambdaParameters), ...suffix]);
}

return group(
indent(
rejectAndJoin(line, [
rejectAndJoin(" ", [lambdaParameters, ctx.Arrow[0]]),
lambdaBody
])
)
);
return concat([lambdaParameters, ...suffix]);
}

lambdaParameters(
ctx: LambdaParametersCtx,
params?: { shouldBreak?: boolean }
) {
lambdaParameters(ctx: LambdaParametersCtx) {
if (ctx.lambdaParametersWithBraces) {
return this.visitSingle(ctx, params);
return this.visitSingle(ctx);
}

return printTokenWithComments(this.getSingle(ctx) as IToken);
}

lambdaParametersWithBraces(
ctx: LambdaParametersWithBracesCtx,
params?: { shouldBreak?: boolean }
) {
const lambdaParameterList = this.visit(ctx.lambdaParameterList, params);
lambdaParametersWithBraces(ctx: LambdaParametersWithBracesCtx) {
const lambdaParameters =
ctx.lambdaParameterList?.[0].children.explicitLambdaParameterList?.[0]
.children.lambdaParameter ??
ctx.lambdaParameterList?.[0].children.inferredLambdaParameterList?.[0]
.children.Identifier ??
[];
handleCommentsParameters(ctx.LBrace[0], lambdaParameters, ctx.RBrace[0]);
const lambdaParameterList = this.visit(ctx.lambdaParameterList);

if (findDeepElementInPartsArray(lambdaParameterList, ",")) {
const separator = params?.shouldBreak === false ? "" : softline;
const content = putIntoBraces(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that there is some logic handling comments around braces in the putIntoBraces function: does it still works well if you remove the putIntoBraces method ?

Copy link
Contributor Author

@jtkiesel jtkiesel Apr 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're very right! This ended up sending me down quite a bit of a rabbit hole.

I realized that, because of the way putIntoBraces removes comments and puts them directly into the Doc it returns (rather than moving them to another token/node), it meant that certain code paths in which I leveraged the conditionalGroup function (introduced in #632) could result in comments being removed from the code entirely (😱).

I believe that I have now fixed that problem, and have added a bunch of tests for comments around methods and lambdas to ensure I handled all those cases properly.

lambdaParameterList,
separator,
return concat([
ctx.LBrace[0],
indent([softline, lambdaParameterList]),
softline,
ctx.RBrace[0]
);
return content;
]);
}

// removing braces when only no comments attached
Expand All @@ -154,29 +166,18 @@ export class ExpressionsPrettierVisitor extends BaseCstPrettierPrinter {
return lambdaParameterList;
}

lambdaParameterList(
ctx: LambdaParameterListCtx,
params?: { shouldBreak?: boolean }
) {
return this.visitSingle(ctx, params);
lambdaParameterList(ctx: LambdaParameterListCtx) {
return this.visitSingle(ctx);
}

inferredLambdaParameterList(
ctx: InferredLambdaParameterListCtx,
params?: { shouldBreak?: boolean }
) {
const commaSuffix = params?.shouldBreak === false ? " " : line;
const commas = ctx.Comma?.map(comma => concat([comma, commaSuffix]));
inferredLambdaParameterList(ctx: InferredLambdaParameterListCtx) {
const commas = ctx.Comma?.map(comma => concat([comma, line]));
return rejectAndJoinSeps(commas, ctx.Identifier);
}

explicitLambdaParameterList(
ctx: ExplicitLambdaParameterListCtx,
params?: { shouldBreak?: boolean }
) {
explicitLambdaParameterList(ctx: ExplicitLambdaParameterListCtx) {
const lambdaParameter = this.mapVisit(ctx.lambdaParameter);
const commaSuffix = params?.shouldBreak === false ? " " : line;
const commas = ctx.Comma?.map(comma => concat([comma, commaSuffix]));
const commas = ctx.Comma?.map(comma => concat([comma, line]));
return rejectAndJoinSeps(commas, lambdaParameter);
}

Expand Down Expand Up @@ -626,31 +627,44 @@ export class ExpressionsPrettierVisitor extends BaseCstPrettierPrinter {
);
}

argumentList(
ctx: ArgumentListCtx,
params?: { isHuggable?: boolean; shouldBreak?: boolean }
) {
const shouldBreak = params?.shouldBreak;
const expressions = this.mapVisit(ctx.expression, params);

const lastArgument = expressions.pop();
const commaSuffix =
shouldBreak === true ? hardline : shouldBreak === false ? " " : line;
const commas = ctx.Comma?.map(comma => concat([comma, commaSuffix]));
const otherArguments = rejectAndJoinSeps(commas, expressions);

if (lastArgument && params?.isHuggable) {
const argumentListGroupId = Symbol("argumentList");
const separator =
shouldBreak === true ? hardline : shouldBreak === false ? "" : softline;
return concat([
group([separator, otherArguments], { id: argumentListGroupId }),
ifBreak([lastArgument, dedent(separator)], dedent(lastArgument), {
groupId: argumentListGroupId
})
]);
argumentList(ctx: ArgumentListCtx) {
const headArgs = this.mapVisit(ctx.expression.slice(0, -1)).map(
(expression, index) => concat([expression, ctx.Comma![index], line])
);
const lastExpression = ctx.expression.at(-1);
const lastArg = this.visit(lastExpression);

if (this.isArgumentListHuggable(ctx)) {
const huggedLastArg = this.visit(lastExpression, { hug: true });
const lastArgNotHuggable =
typeof huggedLastArg === "object" &&
!Array.isArray(huggedLastArg) &&
huggedLastArg.type === "label" &&
huggedLastArg.label?.huggable === false;

if (lastArgNotHuggable || headArgs.some(willBreak)) {
return group([indent([line, ...headArgs, lastArg]), line], {
shouldBreak: true
});
}
const suffix = lastExpression?.children.lambdaExpression?.[0].children
.lambdaBody[0].children.block
? ""
: line;
const hugged = [
...headArgs,
group([huggedLastArg, suffix], { shouldBreak: true })
];
const expanded = group([indent([line, ...headArgs, lastArg]), line], {
shouldBreak: true
});

return willBreak(huggedLastArg)
? [breakParent, conditionalGroup([hugged, expanded])]
: conditionalGroup([[...headArgs, huggedLastArg], hugged, expanded]);
}
return rejectAndConcat([otherArguments, lastArgument]);

return group([indent([softline, ...headArgs, lastArg]), softline]);
}

arrayCreationExpression(ctx: ArrayCreationExpressionCtx) {
Expand Down Expand Up @@ -760,6 +774,9 @@ export class ExpressionsPrettierVisitor extends BaseCstPrettierPrinter {
}

recordPattern(ctx: RecordPatternCtx) {
const componentPatterns =
ctx.componentPatternList?.[0].children.componentPattern ?? [];
handleCommentsParameters(ctx.LBrace[0], componentPatterns, ctx.RBrace[0]);
const referenceType = this.visit(ctx.referenceType);
const componentPatternList = this.visit(ctx.componentPatternList);
return concat([
Expand Down Expand Up @@ -799,6 +816,26 @@ export class ExpressionsPrettierVisitor extends BaseCstPrettierPrinter {
return "isRefTypeInMethodRef";
}

private isArgumentListHuggable(argumentList: ArgumentListCtx) {
const expressions = argumentList.expression;
const lastArgument = expressions.at(-1);
const lastArgumentLambdaBodyExpression =
lastArgument?.children.lambdaExpression?.[0].children.lambdaBody[0]
.children.expression?.[0].children;
const lastArgumentLambdaBodyTernaryExpression =
lastArgumentLambdaBodyExpression?.ternaryExpression?.[0].children;
return (
!lastArgument?.leadingComments &&
!lastArgument?.trailingComments &&
(!lastArgumentLambdaBodyExpression ||
lastArgumentLambdaBodyTernaryExpression?.QuestionMark !== undefined ||
lastArgumentLambdaBodyTernaryExpression?.binaryExpression[0].children
.unaryExpression.length === 1) &&
expressions.findIndex(({ children }) => children.lambdaExpression) ===
expressions.length - 1
);
}

private isBreakableNewExpression(newExpression?: NewExpressionCtx) {
const arrayCreationExpression =
newExpression?.arrayCreationExpression?.[0].children;
Expand Down
6 changes: 6 additions & 0 deletions packages/prettier-plugin-java/src/printers/interfaces.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { concat, group, indent } from "./prettier-builder.js";
import { printTokenWithComments } from "./comments/format-comments.js";
import { handleCommentsParameters } from "./comments/handle-comments.js";
import {
displaySemicolon,
getInterfaceBodyDeclarationsSeparator,
Expand Down Expand Up @@ -278,6 +279,11 @@ export class InterfacesPrettierVisitor extends BaseCstPrettierPrinter {

let annoArgs: Doc = "";
if (ctx.LBrace) {
const elementValues =
ctx.elementValuePairList?.[0].children.elementValuePair ??
ctx.elementValue ??
[];
handleCommentsParameters(ctx.LBrace[0], elementValues, ctx.RBrace![0]);
if (ctx.elementValuePairList) {
annoArgs = putIntoBraces(
this.visit(ctx.elementValuePairList),
Expand Down
Loading
Loading