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

odd formatting for lambdas #19

Open
scottbessler opened this issue Dec 17, 2015 · 43 comments
Open

odd formatting for lambdas #19

scottbessler opened this issue Dec 17, 2015 · 43 comments

Comments

@scottbessler
Copy link
Contributor

scottbessler commented Dec 17, 2015

are the extra newlines intentional?

before:

    Stream<ItemKey> itemIdsStream = stream(members)
        .flatMap(m -> m.getFieldValues()
            .entrySet()
            .stream()
            .filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
            .flatMap(fv -> FieldDTO.deserializeStringToListOfStrings(fv.getValue())
                .stream()
                .map(id -> new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id))));

or even

    Stream<ItemKey> itemIdsStream =
        stream(members)
            .flatMap(
                m -> m.getFieldValues()
                    .entrySet()
                    .stream()
                    .filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
                    .flatMap(
                        fv -> FieldDTO.deserializeStringToListOfStrings(fv.getValue())
                            .stream()
                            .map(
                                id -> new ItemKey(
                                    fieldsById.get(fv.getKey()).getItemTypeId(), id))));

after:

    // items
    Stream<ItemKey> itemIdsStream =
        stream(members)
            .flatMap(
                m ->
                    m.getFieldValues().entrySet().stream()
                        .filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
                        .flatMap(
                            fv ->
                                FieldDTO.deserializeStringToListOfStrings(fv.getValue()).stream()
                                    .map(
                                        id ->
                                            new ItemKey(
                                                fieldsById.get(fv.getKey()).getItemTypeId(), id))));
@kevinb9n
Copy link
Contributor

I don't think the problem is lambda-specific, but this lambda-using code is doing a really good job of tripping up on the general issue. If we can solve that issue, ideally this code ought to be able to show up as

Stream<ItemKey> itemIdsStream =
    stream(members)
        .flatMap(m ->
            m.getFieldValues()
                .entrySet()
                .stream()
                .filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
                .flatMap(fv ->
                    FieldDTO.deserializeStringToListOfStrings(fv.getValue())
                        .stream()
                        .map(id -> new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id))));

@cpovirk
Copy link
Member

cpovirk commented Dec 17, 2015

In addition to the general problem, do we also have something lambda-specific? Your example has the line break after ->, but the formatter apparently puts it before. Should that change?

A result of the current behavior is that any chained calls or arguments to the next expression are indented 1 space from the expression, rather than 4. This is probably to spec, and IIRC it also comes up with the ternary operator, but it looks even weirder here because it's 1 space, rather than the 3 we see with the ternary operator. I'm talking about lines like this one...

            -> m.getFieldValues()
                .entrySet()

...(which kind of works out because m happens to be one letter, perhaps a common case for lambda variables) and this one (which is hard to love)...

                                -> new ItemKey(
                                    fieldsById.get(fv.getKey()).getItemTypeId(), id))));

@kevinb9n
Copy link
Contributor

Yes, the issue of whether to break before or after -> is on our list of style guide issues to resolve before we move to Java 8.

@cushon
Copy link
Collaborator

cushon commented Dec 17, 2015

The formatter started always breaking after -> in b8e6744. I thought that decision had been made already, but I think I misread @kevinb9n's comment in #2.

Anyway, the current behaviour is:

x ->
    y()

x -> {
    y();
}

Instead of:

x
    -> y()

x
    -> {
        y();
    }

I like the consistency between expression and statement lambdas, and it seems like the only way to support the suggested formatting of this example:

Stream<ItemKey> itemIdsStream =
    stream(members)
        .flatMap(m ->
            m.getFieldValues()
            ...

@kevinb9n
Copy link
Contributor

The decision might not have been officially made, but it has been now. Keep -> { together, but otherwise break after ->.

@GuiSim
Copy link

GuiSim commented Mar 11, 2016

Has the code been modified to reflect this decision?

@cushon
Copy link
Collaborator

cushon commented Mar 11, 2016

It has been modified to break after -> and -> {. It hasn't been modified to keep e.g. m -> on the previous line (we haven't decided not to do that, it just hasn't happened).

There should be another release soon, or you can build it at head to see the current behaviour.

@jbduncan
Copy link
Contributor

Hmm, I have some code which, before I tried applying google-java-format to it, I half-expected to end up looking like this:

FooTraverser.of(fooGraph).preOrderTraversal(root).forEach(e -> 
    fooGraph.getSuccessors(e).forEach(child -> 
        treeCopy.addChild(graph.findEdge(e, child), e, child)));

but when I actually did format it (using the version built from 00530c0), it looked like this:

FooTraverser.of(fooGraph)
        .preOrderTraversal(root)
        .forEach(
            e ->
                fooGraph
                    .getSuccessors(e)
                    .forEach(
                        child ->
                            treeCopy.addChild(graph.findEdge(e, child), e, child)));

Is the current behaviour intentional?

@basil
Copy link

basil commented Dec 23, 2016

It hasn't been modified to keep e.g. m -> on the previous line (we haven't decided not to do that, it just hasn't happened).

Has a decision been made about this issue? It's really annoying that the lambda parameter and the arrow token always begin a new line. For example, consider how google-java-format indents this example:

import java.util.Arrays;
import java.util.stream.Stream;

class LambdaExample {
  public static void main(String[] args) {
    String[] stringArr = {"a", "b", "c", "d"};
    Stream<String> stream = Arrays.stream(stringArr);
    stream.forEach(
        letter -> {
          System.out.print(letter);
        });
  }
}

This would look much nicer as follows:

import java.util.Arrays;
import java.util.stream.Stream;

class LambdaExample {
  public static void main(String[] args) {
    String[] stringArr = {"a", "b", "c", "d"};
    Stream<String> stream = Arrays.stream(stringArr);
    stream.forEach(letter -> {
      System.out.print(letter);
    });
  }
}

This problem is exacerbated in AOSP mode. Here's how google-java-format indents the same example when in AOSP mode:

import java.util.Arrays;
import java.util.stream.Stream;

class LambdaExample {
    public static void main(String[] args) {
        String[] stringArr = {"a", "b", "c", "d"};
        Stream<String> stream = Arrays.stream(stringArr);
        stream.forEach(
                letter -> {
                    System.out.print(letter);
                });
    }
}

Lambdas in AOSP mode are indented 12 spaces! This creates a huge amount of unnecessary whitespace and inhibits readability, especially compared to using a traditional for loop. I want to encourage people to use lambdas in our codebase, but it's difficult to do that when using them triples the indentation level and makes the code less readable. In contrast, it looks much nicer if the lambda parameter and arrow token are kept on the preceding line:

import java.util.Arrays;
import java.util.stream.Stream;

class LambdaExample {
    public static void main(String[] args) {
        String[] stringArr = {"a", "b", "c", "d"};
        Stream<String> stream = Arrays.stream(stringArr);
        stream.forEach(letter -> {
            System.out.print(letter);
        });
    }
}

Notice how in the above, the body of the lambda is indented only four spaces from the forEach, the same amount as it would have been in a traditional for loop.

@jbduncan
Copy link
Contributor

Hi @basil, I think you can workaround this nasty lambda indentation behaviour for a good number of cases in the meantime, by using method references[1] whenever possible.

If I were to use the example you gave above as a template, then here's what I'd turn it into:

import java.util.Arrays;
import java.util.stream.Stream;

class MethodReferenceExample {
    public static void main(String[] args) {
        String[] stringArr = {"a", "b", "c", "d"};
        Stream<String> stream = Arrays.stream(stringArr);
        stream.forEach(System.out::print);
    }
}

[1] Or refactoring the lambda expression into a private method, and calling it from a method reference or a new lambda...

@alexkleiman
Copy link

@jbduncan that may work in some individual cases, but unfortunately, it is not feasible to apply this workaround when reformatting a codebase which already makes heavy use of lambdas. That being said, it is certainly a useful workaround to apply on a case-by-case basis until this bug is resolved.

novalis added a commit to novalis/google-java-format that referenced this issue Jun 8, 2017
google#19

The style guide says:
"A line is never broken adjacent to the arrow in a lambda, except that
a break may come immediately after the arrow if the body of the lambda
consists of a single unbraced expression."

There are two changes here:
1. Don't put a newline after the arrow.
2. When the only argument to a function is a lambda, don't put
a newline after the open-paren of the function.  I think
this newline was going in because a lambda is a single expression
that is longer than (the remainder of) a line.  But generally, it's
prettier to break inside the lambda.
novalis added a commit to novalis/google-java-format that referenced this issue Jun 8, 2017
google#19

The style guide says:
"A line is never broken adjacent to the arrow in a lambda, except that
a break may come immediately after the arrow if the body of the lambda
consists of a single unbraced expression."

There are two changes here:
1. Don't put a newline after the arrow.
2. When the only argument to a function is a lambda, don't put
a newline after the open-paren of the function.  I think
this newline was going in because a lambda is a single expression
that is longer than (the remainder of) a line.  But generally, it's
prettier to break inside the lambda.
novalis added a commit to novalis/google-java-format that referenced this issue Jun 8, 2017
google#19

The style guide says:
"A line is never broken adjacent to the arrow in a lambda, except that
a break may come immediately after the arrow if the body of the lambda
consists of a single unbraced expression."

There are two changes here:
1. Don't put a newline after the arrow.
2. When the only argument to a function is a lambda, don't put
a newline after the open-paren of the function.  I think
this newline was going in because a lambda is a single expression
that is longer than (the remainder of) a line.  But generally, it's
prettier to break inside the lambda.
@ilya40umov
Copy link

ilya40umov commented Jul 10, 2017

+1 Formatting of lambdas in the current version (1.3) is quite annoying (gets unreadable quite easily).
e.g.

    assertThat(response.getBody())
        .satisfies(
            body -> {
              assertThat(body.data())
                  .hasValueSatisfying(
                      place -> {
                        assertThat(place.getId()).isEqualTo(253955);
                      });
            });

@wangxufire
Copy link

+1 too many break line

@stephenh
Copy link

Not a huge fan of "+1" comments, but I'll commit the faux pax anyway to note that I'm surprised the "new line solely for lambda variable declaration" is not seen as a bigger deal/fixed by now.

Curious why this is still unresolved? E.g. it is technically challenging? Or do the contributors think the current behavior is actually preferred? If so, maybe just decide so and close the issue?

@dmvk
Copy link

dmvk commented Mar 22, 2018

Hello, is there any update on this issue?

@jkaldon
Copy link

jkaldon commented May 16, 2018

Does anyone have any updates on this bug?

@basil
Copy link

basil commented May 22, 2019

It hasn't been modified to keep e.g. m -> on the previous line (we haven't decided not to do that, it just hasn't happened).

@cushon @ronshapiro I am posting to beg for the elimination of mandatory line breaks before the lambda parameter and arrow token. This is needed to avoid unreadable code. Please also see this explanation.

@linusjf
Copy link

linusjf commented Jun 16, 2019

I have set up indentation as the above in my checkstyle configuration. The issue I have with Google Java format is that it auto-indents lambda expressions by 4 spaces on the next line thus breaking the above rule.
Every time I run the formatter it reformats the lambda expressions by an additional 2 spaces.

My source code is hosted at https://github.com/Fernal73/LearnJava and the project you'd want to look at is Switch under the Switch directory. The checkstyle rules are listed in csrules.xml under the root directory. Run the build.xml under the root directory to generate the classpath for Google java format and then either run the format and checkstyle scripts with the project name as parameter or run the build.xml under the Project Switch. The ant tasks are checkstyle and format respectively. The default build runs the formatter and then checkstyle.

@linusjf
Copy link

linusjf commented Jun 16, 2019

Checkstyle indentation rule.

@linusjf
Copy link

linusjf commented Jun 16, 2019

For some reason, the xml snippet is not accepted by this comment box. Kindly checkout the Module Indentation in csrules.xml in root directory.

@linusjf
Copy link

linusjf commented Jun 16, 2019

Would you prefer I list the above as a new issue?

@linusjf
Copy link

linusjf commented Jun 16, 2019

I'm wondering if this is a Google java format error. Line wrapping from the formatter is always four spaces. If I set the forceStrictCindition property in the checkstyle to true, then it flags every other line wap indentation as well as formatted by Google Java Formatter. One workaround would be to set that value to 4. I'm not happy with that. The question is why is the strict check enforced for lambda expressions by Checkstyle when it's set to false?

@sormuras
Copy link
Contributor

sormuras commented Jun 16, 2019

@Fernal73 Using google-java-format means that you don't need a style checking tool at all. Just format the code with google-java-format and that's it. By definition, all .java files touched by google-java-format are formatted "in style".

When running google-java-format on the command line, you may pass --dry-run --set-exit-if-changed to check the format of .java files. A zero exit codes means, all touched files are "in good style", a non-zero value means some format issues were detected. IIRC, a list of bad formatted files is printed on the console as well.

@linusjf
Copy link

linusjf commented Jun 16, 2019 via email

@tbroyer
Copy link
Contributor

tbroyer commented Jun 16, 2019

Then disable the style-related checks in Checkstyle to prevent any "conflict" between the two tools, and only keep the non-style-related checks that you know won't conflict.

@linusjf
Copy link

linusjf commented Jun 16, 2019 via email

@linusjf
Copy link

linusjf commented Jun 16, 2019 via email

@tbroyer
Copy link
Contributor

tbroyer commented Jun 16, 2019

However, I was wondering if it was possible for it to not disturb existing formatting if it found it well within certain guidelines.

See https://github.com/google/google-java-format/wiki/FAQ#why-didnt-it-leave-my-perfectly-valid-code-alone

@linusjf
Copy link

linusjf commented Jun 16, 2019 via email

@linusjf
Copy link

linusjf commented Jun 18, 2019 via email

@linusjf
Copy link

linusjf commented Jun 18, 2019 via email

@jbduncan
Copy link
Contributor

Hi @Fernal73 - which clang-format file are you referring to? :)

@linusjf
Copy link

linusjf commented Jun 18, 2019 via email

@linusjf
Copy link

linusjf commented Jun 19, 2019 via email

@MrIvanPlays
Copy link

Any progress on this?

@linusjf
Copy link

linusjf commented Sep 7, 2019

What do you mean? No, I don't have access to clang-9 on termux, as yet.

The only workaround for now is to manually format the code as expected and wrap it in // clang-format off and // clang-format on markers. That works if you're using clang-format. It will not prevent Google-Java-format from reformatting your code if it deems it necessary.

@linusjf
Copy link

linusjf commented Sep 8, 2019

My usecase is to use Google-Java-format and clang-format in that order. If I don't like what clang-format does, I comment-block that section. Google-Java-format does a slightly better job, overall, but then I haven't access to clang-format 9.

@mpern
Copy link

mpern commented Jul 7, 2020

Since almost a year passed after the last comment and there was a release of GJF recently (so at least the project isn't dead):

Is there any progress on improving the formatting of lambdas by GJF?

@allensuner
Copy link

I would like to echo the above comment I really don't feel like adding a line break or new line solely for the purpose of using a language feature is a good workaround.

@mpern
Copy link

mpern commented Apr 21, 2021

If you want to have a solution now:

Get rid of GJF and use spotless to run the Eclipse JDT formatter.

@kevinb9n
Copy link
Contributor

kevinb9n commented Aug 23, 2021

Wow. I apologize for not having given any update on this issue in so long (and while so much discussion happened). As I suppose is obvious, we haven't been paying attention in an "organized/committed" way but have just watched for low-hanging fruit.

This one seems to be... high-hanging fruit:

  • The formatter is built on the principle of the "rectangle rule" and that's the source of most of what is good about it
  • The best behavior would be if it can recognize when exceptions to that rule are justified, and back off in those cases
  • This is sure as heck one of those cases
  • Yet it has proven surprisingly difficult to figure out how to make it do that without undue collateral damage
  • It seems that we (Google) don't necessarily have enough motivation to work on figuring this out. It's less nasty for us internally because of our 2-space indents and because we generally prefer to refactor nested lambdas into private methods (used via method reference).

@koppor
Copy link

koppor commented Oct 5, 2024

Note that this behavior was the motivation of the fork "Palantir Java Format". However, they seem not to have any capacity to suport Java 21 (see palantir/palantir-java-format#952).

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

No branches or pull requests