go/printer: adjust formatting of structs with unexported fields

[dsnet]

The documentation for sql.NamedArg currently looks like:

type NamedArg struct {

    // Name of the parameter placeholder. If empty the ordinal position in the
    // argument list will be used.
    //
    // Name must omit any symbol prefix.
    Name string

    // Value of the parameter. It may be assigned the same value types as
    // the query arguments.
    Value interface{}
    // contains filtered or unexported fields
}

Can we fix the formatting to look nicer? I expect it to be like:

type NamedArg struct {
    // Name of the parameter placeholder. If empty the ordinal position in the
    // argument list will be used.
    //
    // Name must omit any symbol prefix.
    Name string

    // Value of the parameter. It may be assigned the same value types as
    // the query arguments.
    Value interface{}

    // Contains filtered or unexported fields.
}

This applies to interfaces as well.

[dsnet]

We could avoid the extra newline at the front if we move the _Named_Fields_Required struct{} to the bottom of the field list, but then we would run into #12884

[magical]

Godoc ultimately relies on the go/printer package for printing definitions, and it seems go/printer likes to preserve blank lines around comments. For example, the following code is unchanged by gofmt

package foo

type _ struct {

	// comment
	_ int
}

If you remove the comment, the blank line goes away as well

package foo

type _ struct {
	_ int
}

Without really knowing much about the internals of go/printer, it seems like a possible fix might be to teach go/printer not to preserve whitespace between an opening brace and a comment.

[dsnet]

@magical, I agree. I looked into this issue as well from the perspective of cmd/doc and came to the same conclusion that the issue in go/printer.

\cc @griesemer

[griesemer]

@magical The problem here is that the printer (must) follows the line information that's in the nodes. The reason there's a blank line here is that there's an unexported field in the source ( https://tip.golang.org/src/database/sql/sql.go?s=1976:2351#L71 ) which is removed from the doc. The remaining fields have their line positions unchanged, and thus for the printer it looks like that there's (at least) one empty line (it doesn't print more than one empty line).

This is non-trivial to fix with the current printer (how does it know that this is not the actual source?). The go/doc lib could try to adjust line numbers but that's really tricky for other reasons.

The real solution (and the solution for several pending printer issues) is a (overdue) rewrite of go/printer, based on a new internal format representation (currently in used by the compiler: cmd/compile/internal/syntax). But it's a big task and it's not high priority.

[griesemer]

Actually, there's perhaps a simple work-around which is a filter that post-processed the output generated by go/printer. Such a filter could simply look for struct { and an empty line (same for interfaces) and remove that empty line. Cheap and dirty but might to the trick. Feel free to submit a CL...

Leaving unplanned for now.

[magical]

@griesemer Correct me if i'm wrong—you're saying that because godoc deletes the _Named_Fields_Required field, that code gets seen by go/printer as

type NamedArg struct {


	// Name is the name of the parameter placeholder.
	//
	// If empty, the ordinal position in the argument list will be
	// used.
	//
	// Name must omit any symbol prefix.
	Name string

	// Value is the value of the parameter.
	// It may be assigned the same value types as the query
	// arguments.
	Value interface{}
}

Right?

I understand that. I'm proposing that go/printer be modified to always omit blank lines between an opening brace of a struct declaration and a comment, like it's already capable of doing when the opening brace is followed by a field. It doesn't have to know anything about the original source.

If that's too difficult or distruptive, i'd be happy to submit a kludge for godoc.

[dsnet]

The downside to a godoc-only kludge is that it doesn't benefit other doc-like tools like cmd/doc. It seems the solution is dirty in either case. So it's do a kludge in go/printer once or do the same kludge for every doc-like tool out there.

[griesemer]

@magical Fair point, that should be reasonably simple in fact.

[griesemer]

Somewhat related to #6996. Maybe it's time to tackle this. I'll mark it for 1.9.

[griesemer]

Too late for 1.9.

[gopherbot]

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

[gdey]

I made an attempt at this. This issue was the chosen randomly for me at the Go Contributor workshop at GopherCon 2017. Here is the CL: https://go-review.googlesource.com/c/49510

[gopherbot]

Change https://golang.org/cl/71990 mentions this issue: go/printer: forbid empty line before first comment in block

[tsuna]

I'm not opposed to this change but just from the point of view of early adopters testing our codebase against tip regularly, this broke our build as we check that files are correctly gofmt'ed and we had to change dozens of files to remove these empty lines. Not a big deal for us (we already merged the change anyways) but I expect many other builds around the world to start failing similarly once this lands.

[griesemer]

@tsuna Thanks for the feedback; I was wondering about possible ripple-effects. We may need to think about a way to roll this out in a different way.

[willfaught]

@dsnet I saw the note in the tip 1.10 release notes about removing blank lines following an opening brace, and I was curious why blank lines preceding a closing brace aren't also removed, so I tracked this issue down to ask. Is there a reason? It seems unexpected to go from this:

func foo() {

    bar()

}

to just this:

func foo() {
    bar()

}

Wouldn't we want this:

func foo() {
    bar()
}

[dsnet]

It only takes effect in between a "{" and a comment, so would not affect your example. However, the following similar transformation does seem unwarranted:

func foo() {

// Comment
bar()

}

becomes:

func foo() {
    // Comment
    bar()

}

The purpose of this change was the address the following: https://play.golang.org/p/36J4nibR1l

For const, var, and type blocks, it already collapses the leading a trailing empty lines (except for when comments are present). We may need to limit the added logic to only trigger for those tokens.

[dsnet]

I think I'll just revert this change and make another attempt in Go1.11.

There are two ways to fix this:

1. Limit the leading/trailing empty line removal to only apply to const, var, type blocks. The formatter already tries to do this, but is buggy in the presence of comments.
2. Always remove leading/trailing empty lines. This is effectively cmd/gofmt: remove leading/trailing blank lines from function bodies #6996, and it is not clear people desire this.

[dsnet]

Revert submitted in CL/81335. Re-opening.

[Eun]

Shameless self plug: https://github.com/Eun/goremovelines