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.

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

Fix line breaks in switch expression #394

Merged
merged 6 commits into from
Jan 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-394.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: fix
fix:
description: Fix line breaks in switch expression
links:
- https://github.com/palantir/palantir-java-format/pull/394
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ boolean isYes() {
}

/** Whether to collapse empty blocks. */
enum CollapseEmptyOrNot {
protected enum CollapseEmptyOrNot {
YES,
NO;

Expand All @@ -201,7 +201,7 @@ boolean isYes() {
}

/** Whether to allow leading blank lines in blocks. */
enum AllowLeadingBlankLine {
protected enum AllowLeadingBlankLine {
YES,
NO;

Expand All @@ -211,7 +211,7 @@ static AllowLeadingBlankLine valueOf(boolean b) {
}

/** Whether to allow trailing blank lines in blocks. */
enum AllowTrailingBlankLine {
protected enum AllowTrailingBlankLine {
YES,
NO;

Expand Down Expand Up @@ -1218,6 +1218,11 @@ public Void visitLabeledStatement(LabeledStatementTree node, Void unused) {
public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
sync(node);
boolean statementBody = node.getBodyKind() == LambdaExpressionTree.BodyKind.STATEMENT;
visitLambdaExpression(node, statementBody);
return null;
}

protected void visitLambdaExpression(LambdaExpressionTree node, boolean statementBody) {
boolean parens = builder.peekToken().equals(Optional.of("("));
builder.open("lambda arguments", parens ? plusFour : ZERO);
if (parens) {
Expand Down Expand Up @@ -1266,7 +1271,6 @@ public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
scan(node.getBody(), null);
}
builder.close();
return null;
}

@Override
Expand Down Expand Up @@ -2108,7 +2112,7 @@ void visitAnnotations(List<? extends AnnotationTree> annotations, BreakOrNot bre
}

/** Helper method for blocks. */
private void visitBlock(
protected void visitBlock(
BlockTree node,
CollapseEmptyOrNot collapseEmptyOrNot,
AllowLeadingBlankLine allowLeadingBlankLine,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@
import com.palantir.javaformat.OpsBuilder;
import com.palantir.javaformat.java.JavaInputAstVisitor;
import com.sun.source.tree.BindingPatternTree;
import com.sun.source.tree.BlockTree;
import com.sun.source.tree.CaseTree;
import com.sun.source.tree.ClassTree;
import com.sun.source.tree.ExpressionTree;
import com.sun.source.tree.InstanceOfTree;
import com.sun.source.tree.LambdaExpressionTree;
import com.sun.source.tree.SwitchExpressionTree;
import com.sun.source.tree.Tree;
import com.sun.source.tree.Tree.Kind;
import com.sun.source.tree.YieldTree;
import com.sun.tools.javac.code.Flags;
import com.sun.tools.javac.tree.JCTree;
Expand Down Expand Up @@ -199,15 +202,17 @@ public Void visitCase(CaseTree node, Void unused) {
} else {
token("case", plusTwo);
builder.space();
builder.open(plusTwo);
boolean first = true;
for (ExpressionTree expression : node.getExpressions()) {
if (!first) {
token(",");
builder.space();
builder.breakOp(" ");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for case 2.

}
scan(expression, null);
first = false;
}
builder.close();
}
switch (node.getCaseKind()) {
case STATEMENT:
Expand All @@ -226,12 +231,36 @@ public Void visitCase(CaseTree node, Void unused) {
token("-");
token(">");
builder.space();
scan(node.getBody(), null);
if (node.getBody().getKind() == BLOCK) {
// Explicit call with {@link CollapseEmptyOrNot.YES} to handle empty case blocks.
visitBlock(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For collapsing empty case blocks: case ENUM -> {}

Copy link
Contributor

Choose a reason for hiding this comment

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

Mind adding this as a source comment? :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment 👍

(BlockTree) node.getBody(),
CollapseEmptyOrNot.YES,
AllowLeadingBlankLine.NO,
AllowTrailingBlankLine.NO);
} else {
scan(node.getBody(), null);
}
builder.guessToken(";");
break;
default:
throw new AssertionError(node.getCaseKind());
}
return null;
}

/**
* TODO(fwindheuser): Collapse with
* {@link JavaInputAstVisitor#visitLambdaExpression(LambdaExpressionTree, Void)}} after dropping Java 11
* compatibility.
*/
@Override
public Void visitLambdaExpression(LambdaExpressionTree node, Void unused) {
sync(node);
// Also format switch expressions as statement body instead of inlining them
boolean statementBody = node.getBodyKind() == LambdaExpressionTree.BodyKind.STATEMENT
|| node.getBody().getKind() == Kind.SWITCH_EXPRESSION;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix for case 1. This has to be in the Java14InputAstVisitor.java class to not break java 11 compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

should we add some kind of todo to collapse once we drop 11 (probably several years out)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the todo.

But tbh, once we drop java 11 compat, we would collapse the whole Java14InputAstVisitor with the base class.

visitLambdaExpression(node, statementBody);
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,23 @@ class ExpressionSwitch {
};
return val;
}
}

int wrapping(Wrapping w) {
switch (w) {
case THIS_IS_A_VERY_LONG_ENUM_VALUE_ONE,
THIS_IS_A_VERY_LONG_ENUM_VALUE_TWO,
THIS_IS_A_VERY_LONG_ENUM_VALUE_THREE -> {}
}

switch (w) {
case CASE_A, CASE_B, CASE_C -> {}
}
}

Supplier<Integer> lambda(int k) {
return () -> switch (k) {
case 0 -> true;
default -> false;
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,23 @@ class ExpressionSwitch {
};
return val;
}

int wrapping(Wrapping w) {
switch (w) {
case THIS_IS_A_VERY_LONG_ENUM_VALUE_ONE,
THIS_IS_A_VERY_LONG_ENUM_VALUE_TWO,
THIS_IS_A_VERY_LONG_ENUM_VALUE_THREE -> {}
}

switch (w) {
case CASE_A, CASE_B, CASE_C -> {}
}
}

Supplier<Integer> lambda(int k) {
return () -> switch (k) {
case 0 -> true;
default -> false;
};
}
}