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

Suboptimal line breaks for long switch rules (with -> arrows) #880

Closed
PhilippWendler opened this issue Jan 13, 2023 · 4 comments
Closed

Comments

@PhilippWendler
Copy link

For the following code, I would expect line breaks to be added at the arrows (->) of the rules of the switch expression:

public class Test {

  public String f(int i) {
    return switch (i) {
      case 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 -> "looooooooooooooooooooooooooooooooooooooooong expression";
      default -> "looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong expression";
    };
  }
}

However, google-java-format 1.15.0 formats it like this:

public class Test {

  public String f(int i) {
    return switch (i) {
      case 0,
          1,
          2,
          3,
          4,
          5,
          6,
          7,
          8,
          9 -> "looooooooooooooooooooooooooooooooooooooooong expression";
      default -> "looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong"
                     + " expression";
    };
  }
}

It even prefers breaking up the string over a line break at the arrow. Please increase the priority of line breaking at the arrow. I guess it should be of higher priority than the list of cases and anything within the expression.

@kevinb9n
Copy link
Contributor

kevinb9n commented Jan 25, 2023

Some overall thoughts on arrowform switch that y'all can pick apart

  • Agreed, the formatter's favorite break should be after the arrow or the arrow-and-open brace
  • Arrow always stays attached to previous token, like comma (seems to do this already)
  • If there are braces, treat as a block (probably does this already)
  • If no braces, and the entire expression won't stay with the arrow for whatever reason, wrap after arrow (as noted) and probably use a regular indent rather than continuation indent to avoid the overall switch expression being jagged
  • If even then the expression still wraps, I guess that calls for a continuation indent from that level usually, right? (But not relative to the start of the expression; relative to the previous line's indentation level; that might not matter here)
  • If case-plus-labels-plus-arrow must wrap, I think those wrapped lines are right to use a continuation indent as shown
  • I can't remember if the formatter generally takes steps to avoid one-per-line formatting when a separated list contains very small elements like shown above, but this should ideally be the same as the other cases

@PhilippWendler
Copy link
Author

I found another suboptimal formatting that is related to this, namely if there is a line comment right after the arrow. In this case, google-java-format misses to add indentation:

public class Test {
  public boolean test(int i) {
    return switch (i) {
      case 0 -> // zero
      false;
      case 1 -> "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".length()
          == 0;
      default -> // otherwise
      true;
    };
  }
}

Together, these issues (plus #876) make using arrow rules not so nice currently, so any improvement here would be highly appreciated! We would likely move over to arrow rules completely in our code base once formatting works fine.

@cushon
Copy link
Collaborator

cushon commented Dec 7, 2023

I have some work-in-progress improvements to switch rule handling that result in the following for these examples. This output is non-final but I think it's a step in the right direction.

There are also likely more issues lurking with the new features, so reports/feedback welcome if you see cases that aren't handled well with the new features!

class I880 {
  public String f(int i) {
    return switch (i) {
      case 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ->
          "looooooooooooooooooooooooooooooooooooooooong expression";
      default ->
          "looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong expression";
    };
  }

  public String f(int i) {
    return switch (i) {
      case 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 ->
          "looooooooooooooooooooooooooooooooooooooooong expression";
      default ->
          "looooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooong" + " expression";
    };
  }

  public boolean test(int i) {
    return switch (i) {
      case 0 -> // zero
          false;
      case 1 ->
          "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".length() == 0;
      default -> // otherwise
          true;
    };
  }
}

copybara-service bot pushed a commit that referenced this issue Dec 8, 2023
Fixes #937, #880

PiperOrigin-RevId: 588882242
copybara-service bot pushed a commit that referenced this issue Dec 8, 2023
Fixes #937, #880

PiperOrigin-RevId: 588882242
copybara-service bot pushed a commit that referenced this issue Dec 8, 2023
Fixes #937, #880

PiperOrigin-RevId: 589140113
@PhilippWendler
Copy link
Author

Thanks!

eholmer pushed a commit to palantir/palantir-java-format that referenced this issue Oct 21, 2024
eholmer pushed a commit to palantir/palantir-java-format that referenced this issue Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants