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

go/printer: adjust formatting of structs with unexported fields #18264

Open
dsnet opened this issue Dec 9, 2016 · 20 comments
Open

go/printer: adjust formatting of structs with unexported fields #18264

dsnet opened this issue Dec 9, 2016 · 20 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Dec 9, 2016

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
Copy link
Member Author

dsnet commented Dec 9, 2016

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

@bradfitz bradfitz changed the title godoc: adjust formatting of structs with unexported fields x/tools/godoc: adjust formatting of structs with unexported fields Dec 9, 2016
@bradfitz bradfitz added this to the Go1.9Maybe milestone Dec 9, 2016
@bradfitz bradfitz added help wanted Suggested Issues that may be good for new contributors looking for work to do. labels Dec 9, 2016
@magical
Copy link
Contributor

magical commented Dec 13, 2016

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
Copy link
Member Author

dsnet commented Dec 13, 2016

@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
Copy link
Contributor

griesemer commented Dec 13, 2016

@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 griesemer modified the milestones: Unplanned, Go1.9Maybe Dec 13, 2016
@griesemer griesemer assigned griesemer and unassigned griesemer Dec 13, 2016
@griesemer
Copy link
Contributor

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
Copy link
Contributor

magical commented Dec 13, 2016

@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
Copy link
Member Author

dsnet commented Dec 13, 2016

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
Copy link
Contributor

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

@griesemer
Copy link
Contributor

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

@griesemer griesemer modified the milestones: Go1.9, Unplanned Dec 13, 2016
@griesemer griesemer self-assigned this Dec 13, 2016
@dsnet dsnet changed the title x/tools/godoc: adjust formatting of structs with unexported fields go/printer: adjust formatting of structs with unexported fields Dec 16, 2016
@griesemer
Copy link
Contributor

Too late for 1.9.

@griesemer griesemer modified the milestones: Go1.10, Go1.9 Jun 20, 2017
@gopherbot
Copy link

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

@gdey
Copy link

gdey commented Jul 18, 2017

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
Copy link

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

@tsuna
Copy link
Contributor

tsuna commented Nov 7, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Member Author

dsnet commented Dec 1, 2017

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 dsnet assigned dsnet and unassigned griesemer Dec 1, 2017
@dsnet
Copy link
Member Author

dsnet commented Dec 1, 2017

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 dsnet added NeedsFix The path to resolution is known, but the work has not been done. and removed help wanted Suggested Issues that may be good for new contributors looking for work to do. labels Dec 1, 2017
@dsnet dsnet modified the milestones: Go1.10, Go1.11 Dec 1, 2017
@dsnet
Copy link
Member Author

dsnet commented Dec 1, 2017

Revert submitted in CL/81335. Re-opening.

@Eun
Copy link

Eun commented Feb 18, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests