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

go/printer: confusing indentation for nested structs #16371

Open
tbertelsen opened this Issue Jul 14, 2016 · 17 comments

Comments

Projects
None yet
9 participants
@tbertelsen

tbertelsen commented Jul 14, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version go1.6.1 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="linux"
    GOOS="linux"
    GOPATH=""
    GORACE=""
    GOROOT="/usr/lib/google-golang"
    GOTOOLDIR="/usr/lib/google-golang/pkg/tool/linux_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="gcc"
    GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -gno-record-gcc-switches -fdebug-prefix-map=/tmp/go-build255917305=/tmp/go-build"
    CXX="g++"
    CGO_ENABLED="1"
  3. What did you do?
    Run gofmt on a file containing this code, that has confusing indention
    tests := []struct {
        name  string
        input *StructA
        want  string
    }{
        {"empty", &StructA{
            NestedB: &StructB{},
        },                       //  <- Confusion indention
            "",                  //  <- Confusion indention
        },
    }
  1. What did you expect to see?
    The indention was changed to something less confusing.
  2. What did you see instead?
    The indention is what gofmt intends, even though it is confusion.
@mvdan

This comment has been minimized.

Member

mvdan commented Jul 14, 2016

This is expected. It won't be as confusing if you don't cram {"empty", &StructA{ in a single line.

@tbertelsen

This comment has been minimized.

tbertelsen commented Jul 14, 2016

It might be expected, but I don't find it really readable. Don't you agree, that the code is relatively easy to misunderstand?

It can very well be that it is not the intention that is the source problem, but the line breaks. I will then suggest that gofmt fixes the linebreaks, for example something like:

        {
            "empty", &StructA{
                NestedB: &StructB{},
            },
            "",
        },

or

        { "empty",
            &StructA{
                NestedB: &StructB{},
            },
            "",
        },
@tbertelsen

This comment has been minimized.

tbertelsen commented Jul 14, 2016

FWIW: Andrew Gerrand (@nf) finds the current behavior "surprising".

@mvdan

This comment has been minimized.

Member

mvdan commented Jul 14, 2016

Perhaps forcing a newline in this edge case is an option. I'll poke around gofmt to see if I can find anything.

@josharian josharian changed the title from Gofmt chooses confusing indention for nested structs. to cmd/gofmt: chooses confusing indention for nested structs Jul 14, 2016

@josharian

This comment has been minimized.

Contributor

josharian commented Jul 14, 2016

FWIW, I also found this behavior surprising. I expected the line after "NestedB" to be indented. I can't think of any other cases offhand in which gofmt has such mismatched indentation.

cc @griesemer

@tbertelsen

This comment has been minimized.

tbertelsen commented Jul 14, 2016

@mvdan Thanks for taking a shot at this :)

@mvdan

This comment has been minimized.

Member

mvdan commented Jul 14, 2016

I've got a working solution with some tests, will post a CL shortly. This is my first tinkering with go/printer so comments would be appreciated.

@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jul 14, 2016

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented Jul 14, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Jul 14, 2016

CL https://golang.org/cl/24882 mentions this issue.

@nigeltao

This comment has been minimized.

Contributor

nigeltao commented Jul 15, 2016

To play devil's advocate, I don't find the existing behavior surprising. Let's label some lines from 0 to 4:

0:  {"empty", &StructA{
1:      NestedB: &StructB{},
2:  },                       //  <- Confusion indention
3:      "",                  //  <- Confusion indention
4:  }

Note that there are two open-curlies on line 0. Let's call them 0a and 0b.

The close-curly on line 2 is vertically aligned with line 0. They both have the same indentation. Note that line 0 is the one containing the matching open-curly, namely 0b. This seems consistent to me, similar to how the matching close-curly in:

func foo() {
    etc
}

is vertically aligned with the start of the line containing the matching open-curly. Similarly for struct literals in general.

As for the "" on line 3, again that seems consistent to me. That is a field of the struct literal (the struct type is an anonymous struct), and the general rule is that fields are indented more than the literal itself, i.e. its defining curlies: 0a and the one on line 4.

I think that gofmt is at least being self-consistent here, and applying simple rules. As for the ugliness of the gofmt'ed output, personally, I'd format it differently:

tests := []struct {
    name  string
    input *StructA
    want  string
}{{
    "empty",
    &StructA{
        NestedB: &StructB{},
    },
    "",
}, {
    etc
}}

Everything seems consistent to me: every close-curly that starts a line is vertically aligned with the line containing the matching open-curly. Every field of a struct literal is at equal indentation, and a greater indentation than its surrounding struct literal.

@mvdan

This comment has been minimized.

Member

mvdan commented Jul 18, 2016

I think that gofmt is at least being self-consistent here, and applying simple rules.

I agree that it's being consistent - this is why I initially said "this is expected, you can get a less confusing output by changing the input".

Everything seems consistent to me: every close-curly that starts a line is vertically aligned with the line containing the matching open-curly. Every field of a struct literal is at equal indentation, and a greater indentation than its surrounding struct literal.

If you look at my CL, I have done something very similar. In particular, @tbertelsen's first suggestion:

        {
            "empty", &StructA{
                NestedB: &StructB{},
            },
            "",
        },

I think this is slightly better as it is less intrusive. I would add the minimum number of newlines to avoid the indent confusion. Forcing every field to its own line seems unnecessary to me, especially as go/printer would not do that otherwise.

By the way, this issue should probably be retitled to concern go/printer and not gofmt.

@griesemer griesemer changed the title from cmd/gofmt: chooses confusing indention for nested structs to cmd/gofmt, go/printer: chooses confusing indention for nested structs Jul 19, 2016

@griesemer griesemer changed the title from cmd/gofmt, go/printer: chooses confusing indention for nested structs to go/printer (cmd/gofmt): chooses confusing indention for nested structs Jul 19, 2016

@josharian josharian changed the title from go/printer (cmd/gofmt): chooses confusing indention for nested structs to go/printer: chooses confusing indention for nested structs Oct 31, 2016

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 12, 2016

Fixing this issue correctly is non-trivial. For details see my comments on the CL above. The necessary changes require "measuring" the layout of composite literal elements (go/printer/nodes.go contains a nodeSize method for that purpose) and also collecting layout information (e.g. does an element itself contain indentation). With this information we can then decide whether an extra newline is warranted before such an element, if it occurs on the same line as the opening '{' of the containing composite literal. The extra newline will then force correct indentation for the rest.

The nodeSize method will have to be expanded while maintaining it's current functionality. The results need to be cached ("memoized") so we don't end up in exponential behavior with deep nested composite literals.

I suspect the resulting changes may be too large, now that we're in the freeze. I'll give it a shot on Monday to see how bad it is, but I'm going to mark this 1.8 Maybe for now.

@griesemer griesemer modified the milestones: Go1.8Maybe, Go1.8 Nov 12, 2016

@griesemer griesemer self-assigned this Nov 12, 2016

@gopherbot

This comment has been minimized.

gopherbot commented Nov 15, 2016

CL https://golang.org/cl/33294 mentions this issue.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 15, 2016

@mvdan Here's a first step: https://go-review.googlesource.com/#/c/33294/. This CL adds the code to compute whether a composite literal element contains indentation. One should be able to use this for a complete fix. I haven't quite figured out a clean and correct approach for the rest of the code. It also needs some cleanup. Feel free to give it a shot.

@mvdan

This comment has been minimized.

Member

mvdan commented Nov 16, 2016

Thanks @griesemer, that helps.

Given that this would mean redoing the CL from scratch, I suggest we mark this for 1.9. I still want to give it a shot, I just don't think it's a good idea to rush it or get such a non-trivial fix in at this stage of the freeze.

I'll be sure to get another CL in before the 1.9 tree opens in a few months.

While at it, do you have any thoughts regarding other indented lists, such as call arguments? For example: https://play.golang.org/p/LMAfTqtN2O

I think we should be consistent with this "avoiding confusing indentation" fix. At the same time, it seems like the two fixes would be in different parts of the code and should come in separate CLs.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 16, 2016

@mvdan I agree that argument lists should behave the same if we do this.

@griesemer griesemer modified the milestones: Go1.9, Go1.8Maybe Nov 16, 2016

@griesemer griesemer changed the title from go/printer: chooses confusing indention for nested structs to go/printer: confusing indentation for nested structs Jun 2, 2017

@griesemer

This comment has been minimized.

Contributor

griesemer commented Jun 20, 2017

Too late for 1.9.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment