Linebreaks in generics

[jhaber]

In testing an upgrade to 1.6.1, we're sometimes seeing line breaks introduced into generics. Seems like this is probably a result of #512. I can see this being useful in pathological cases with extremely long generic type lists, but the issue is that we run prettier with a pretty aggressive line length limit, so we end up seeing split generics even in relatively straight-forward code samples. And it's pretty jarring when you first see it, because as far as I know, splitting generics like this isn't an idiomatic way to format Java (at least I haven't seen it in the wild before).

I tested the prettier-java upgrade on ~1,500 of our internal GitHub repos, and it definitely seemed like, in most cases, this generic splitting was hurting the code readability more than it helped. But curious to get your thoughts on how attached you are to this feature.

Prettier-Java 1.6.1

# Options (if any):

Input:

public abstract class GenericWblGeneratorWithLongName<OBJECT_TYPE, RECORD_TYPE> {

  public static <RECORD, OFFSET> PagedResult<RECORD, OFFSET> appendCustomOffsets(
    Arg arg1,
    Arg arg2
  ) {
    // implementation
  }
}

Output:

public abstract class GenericWblGeneratorWithLongName<
  OBJECT_TYPE, RECORD_TYPE
> {

  public static <
    RECORD, OFFSET
  > PagedResult<RECORD, OFFSET> appendCustomOffsets(Arg arg1, Arg arg2) {
    // implementation
  }
}

Expected behavior:

public abstract class GenericWblGeneratorWithLongName<OBJECT_TYPE, RECORD_TYPE> {

  public static <RECORD, OFFSET> PagedResult<RECORD, OFFSET> appendCustomOffsets(
    Arg arg1,
    Arg arg2
  ) {
    // implementation
  }
}

[jhaber]

👋 just wanted to see if anyone has thoughts on this

[clementdessoude]

Hi @jhaber !

I am not sure on the best behaviour here. It sure seems that your expected behaviour looks nicer in your case, and we might want to solve a formatting problem that happens in very rare use cases. The three options I see are:

1. keeping the rule as it is
2. remove it completely
3. trying to adapt it so that it could be break only for very long generics name or lots of generics, but it could create some complexity for a limited result…

@jhipster/developers as most of you has a lot of experience in Java, do you have an opinion on this ?

[pascalgrimaud]

I'd vote for keeping the rule as it is, as the problem comes from GenericWblGeneratorWithLongName, it could be easy to find one shorter.

Anyway, I'd suggest to use some // prettier-ignore when it's needed :)

[jhaber]

I'd vote for keeping the rule as it is, as the problem comes from GenericWblGeneratorWithLongName, it could be easy to find one shorter.

There is also a method in the code sample I posted and it doesn't contain any lengthy identifiers. Unfortunately I don't think renaming existing classes and rewriting existing method signatures is a viable workaround at our company with 1,000+ engineers and millions of lines of existing code. We all assumed it was a bug in prettier-java until we found #512. The code essentially looked mangled to us since we've never seen anyone format code like this (nor any autoformatter).

But I understand that it's not possible to please everyone when making an opinionated code formatter. Given that #512 is a very small code change, we can just maintain a fork without that behavior and it shouldn't cause too many merge conflicts when trying to pull from upstream

[pascalgrimaud]

I can understand your concerns @jhaber but what would be your vote, according to what @clementdessoude suggest ?
As the final decision is for @clementdessoude :)

[jhaber]

My vote would probably be for option 2. I don't see any issues linked to #512, so I'm not sure how many people were having issues with long generics. Option 3 could be nice but I think it's better to keep the formatting simple and consistent rather than trying to make it smart with heuristics.

[jhaber]

Just wanted to see if there are any other thoughts here. We've been holding off on upgrading past 1.4.0 because of this issue, but now that's limiting our ability to use Java 17 syntax features

[jhaber]

If #512 gets reverted, people can always line-break complicated generics manually and then use // prettier-ignore as needed. In my experience, I just haven't seen very much code out there that looks like the example in #512

[jtkiesel]

Based on how Prettier TypeScript deals with line breaking and generics, this is how I would expect Prettier Java to format this issue's input:

public abstract class GenericWblGeneratorWithLongName<
  OBJECT_TYPE,
  RECORD_TYPE
> {

  public static <RECORD, OFFSET> PagedResult<RECORD, OFFSET> appendCustomOffsets(
    Arg arg1,
    Arg arg2
  ) {
    // implementation
  }
}

With respect to line breaking and generics, Prettier TypeScript does 2 things differently from Prettier Java right now (that were noticeable to me from this issue's input, there may be other differences):

1. Whenever it breaks on generic type parameters, it puts each of them onto their own lines, similar to method parameters.
2. Whenever it needs to break a method definition, it chooses to break on the method parameters first, before it chooses to break on the generic type parameters.

I like that Prettier Java can now break long lines on generics, but I do think it could be improved a bit (which IMO would be to look more like the example above).

[ecanja]

Hi, I find Prettier-Java very cool (thanks for building it!), but I was also very surprised by these linebreaks in generic types and didn't expect that.

I do like the linebreaks for complex generics definitions in class declarations (which was the rationale of #512), but I do not like the linebreak for generics declared for methods or even in return types of methods. In addition to the comment by @jtkiesel, I'd like to illustrate this by a different example. For these examples, the configured printWidth is 80 (I usually have a longer line length, but I do also have longer method names usually, that's why I still ran into this problem):

Input:

public class ReasonableLengthTypeFinder {

  public Optional<ReasonableLengthType> findFirstReasonableLengthTypeValidOnOrAfterDate(LocalDate date) {
    // Implementation
  }

  public YetAnotherReasonableLengthType findOtherReasonableLengthTypeValidOnOrAfterDate(LocalDate date) {
    // Implementation
  }
 
}

Output:

public class ReasonableLengthTypeFinder {

  public Optional<
    ReasonableLengthType
  > findFirstReasonableLengthTypeValidOnOrAfterDate(LocalDate date) {
    // Implementation
  }

  public YetAnotherReasonableLengthType findOtherReasonableLengthTypeValidOnOrAfterDate(
    LocalDate date
  ) {
    // Implementation
  }
 
}

Expected behaviour:

public class ReasonableLengthTypeFinder {

  public Optional<ReasonableLengthType> findFirstReasonableLengthTypeValidOnOrAfterDate(
    LocalDate date
  ) {
    // Implementation
  }

  public YetAnotherReasonableLengthType findOtherReasonableLengthTypeValidOnOrAfterDate(
    LocalDate date
  ) {
    // Implementation
  }
 
}

I would expect this behaviour, because the two methods have exactly the same number of characters in return type, method name and argument. I don't think that it makes sense to have a difference here just because one return type has generics and the other doesn't.

Or, at least, I'd also (as @jtkiesel) suggest to first break the parameters. If that's not enough, a return type with generics could be broken up, but I'm not sure if I would actually choose to do that. Whether to do that or not looks more like a matter of taste than the above behaviour, so even if you decide to do that (after breaking parameters), it would seem like a reasonable choice to me for an opinionated formatter. On the contrary, the above behaviour does feel more like a bug to me.

Another alternative as a second formatting action – if the method declaration gets too long, even after breaking on method parameters – would be to break after the return type, so that the method name is moved to the next line. E.g. in the above expected code, the method line is still longer than 80 and could (even without generics) be broken up like this:

public class ReasonableLengthTypeFinder {

  public YetAnotherReasonableLengthType
    findOtherReasonableLengthTypeValidOnOrAfterDate(
        LocalDate date
  ) {
    // Implementation
  }

  // Or alternatively, if with parameters the second line is less than 80 chars:
  public YetAnotherReasonableLengthType
    findOtherReasonableLengthTypeValidOnOrAfterDate(LocalDate date) {
    // Implementation
  }

}

But this doesn't happen now (even if the class name is itself longer than 80 chars, I could never make this linebreak happen), so I guess Prettier (or Prettier-Java?) generally decided against that. I would still like that more than breaking the generics.

Conclusion

I would suggest to change the behaviour of breaking of generic types in method declarations (not in class declarations, though – I do like the example given in #512). I'd be happy with any of the following options:

1. Only ever break method parameters, never generics in declaration or return type (or in method parameters – haven't seen that happen, but maybe it actually would happen).
2. First break method parameters, then break after the return type, if still necessary (reverting the parameter break if reasonable, see above). Never break generics in declaration or return type.
3. First break method parameters, then break after the return type, if still necessary (reverting the parameter break if reasonable, see above), then break generics in declaration or return type.
4. First break method parameters, then break generics in declaration or return type.

[benjavalero]

After updating from 1.6.1 to 2.3.1, I have seen a reformatting that it seems to fall into this issue. Not sure if related to #512 or #584.

Before (1.6.1):

SetValuedMap<WikipediaLanguage, StandardMisspelling> oldItems = (SetValuedMap<WikipediaLanguage, StandardMisspelling>) evt.getOldValue();

After (2.3.1):

SetValuedMap<WikipediaLanguage, StandardMisspelling> oldItems = (SetValuedMap<
        WikipediaLanguage,
        StandardMisspelling
    >) evt.getOldValue();

See diff in GitHub.

In this case, wouldn't it be better to just move to the next line everything after the =?

Thanks for your work,

[jtkiesel]

I've looked into this a bit more now, and I might have been mistaken (or perhaps recent PRs have solved some of these problems), because it appears as though Prettier Java is formatting code pretty close to the way I was suggesting. For example:

Input:

class Example<TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM> {

  // the arguments violate printWidth
  <TYPE_PARAM, TYPE_PARAM> ReturnType<TYPE_PARAM, TYPE_PARAM> example(ArgType arg, ArgType arg) {}

  // the return type violates printWidth
  <TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM> ReturnType<TYPE_PARAM, TYPE_PARAM> example(ArgType arg, ArgType arg) {}

  // the type parameters violate printWidth
  <TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM> ReturnType<TYPE_PARAM, TYPE_PARAM> example(ArgType arg, ArgType arg) {}
}

Output:

class Example<
  TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM
> {

  // the arguments violate printWidth
  <TYPE_PARAM, TYPE_PARAM> ReturnType<TYPE_PARAM, TYPE_PARAM> example(
    ArgType arg,
    ArgType arg
  ) {}

  // the return type violates printWidth
  <TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM> ReturnType<
    TYPE_PARAM,
    TYPE_PARAM
  > example(ArgType arg, ArgType arg) {}

  // the type parameters violate printWidth
  <
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM
  > ReturnType<TYPE_PARAM, TYPE_PARAM> example(ArgType arg, ArgType arg) {}
}

Almost all of this is formatted exactly as I would expect, when comparing it to similar code formatted with Prettier JavaScript (on TypeScript). The only difference that I would expect is for the class' type parameters to be each broken onto their own lines. For example:

class Example<
  TYPE_PARAM,
  TYPE_PARAM,
  TYPE_PARAM,
  TYPE_PARAM,
  TYPE_PARAM,
  TYPE_PARAM
> {

  // the arguments violate printWidth
  <TYPE_PARAM, TYPE_PARAM> ReturnType<TYPE_PARAM, TYPE_PARAM> example(
    ArgType arg,
    ArgType arg
  ) {}

  // the return type violates printWidth
  <TYPE_PARAM, TYPE_PARAM, TYPE_PARAM, TYPE_PARAM> ReturnType<
    TYPE_PARAM,
    TYPE_PARAM
  > example(ArgType arg, ArgType arg) {}

  // the type parameters violate printWidth
  <
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM,
    TYPE_PARAM
  > ReturnType<TYPE_PARAM, TYPE_PARAM> example(ArgType arg, ArgType arg) {}
}

I do think that @benjavalero's example is something that should be improved, even if it's only the indentation that is changed. For example:

Input:

class Example {

  SetValuedMap<WikipediaLanguage, StandardMisspelling> oldItems = (SetValuedMap<WikipediaLanguage, StandardMisspelling>) evt.getOldValue();
}

Output (hypothetical):

class Example {

  SetValuedMap<WikipediaLanguage, StandardMisspelling> oldItems = (SetValuedMap<
    WikipediaLanguage,
    StandardMisspelling
  >) evt.getOldValue();
}

[jhaber]

One last nudge in the hopes of reverting this generic linebreak behavior. I believe we're one of the largest (if not the largest?) users of prettier-java, but our infra team wants to migrate to something else because of this issue