Skip to content

go/format: Source output is not idempotent #26930

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.

Sign up for GitHub

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

Jump to bottom
Closed
dsnet opened this issue Aug 11, 2018 · 7 comments
Closed

go/format: Source output is not idempotent #26930

dsnet opened this issue Aug 11, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.12

Comments

@dsnet
Copy link
Member

dsnet commented Aug 11, 2018

Consider the following:

package main

import (
	"bytes"
	"go/format"
	"testing"
)

func Test(t *testing.T) {
	const src = `
package foo

var _ = foo{
F1: []string{
},
Field2: []string{},
}
`
	want, err := format.Source([]byte(src))
	if err != nil {
		t.Fatalf("format.Source() error: %v", err)
	}
	got, err := format.Source([]byte(want))
	if err != nil {
		t.Fatalf("format.Source() error: %v", err)
	}
	if !bytes.Equal(got, want) {
		t.Errorf("gofmt was not idempotent")
	}
}

Running format.Source on the output of itself should produce no changes. However, it does. Bisect traces the regression to https://go-review.googlesource.com/125260.

\cc @mvdan @griesemer
@dsnet dsnet added this to the Go1.11 milestone Aug 11, 2018
@mvdan
Copy link
Member

mvdan commented Aug 11, 2018

Thanks - will take a look as soon as I can.

@bcmills bcmills mentioned this issue Aug 11, 2018
Closed
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/129097 mentions this issue: go/printer: skip formfeed when breaking alignments

@griesemer
Copy link
Contributor

I don't believe this is a release blocker. For one, there are other situations where gofmt is not idempotent, and they have been around for a long time. That is not to mean that we don't care; it just means that we have been able to live with it for now. We do want to fix these but it doesn't warrant blocking a release.

In this case the problem arises only (as far as I can tell) if the (first) entry []string{} is an empty slice/composite literal where the closing } is on a new line. It's uncommon. There's a manual work-around (write {} on a single line) which is the common way of writing it.

Marking for 1.12 until further evidence that this is more urgent.

@griesemer griesemer modified the milestones: Go1.11, Go1.12 Aug 13, 2018
@griesemer griesemer added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed release-blocker labels Aug 13, 2018
@dsnet
Copy link
Member Author

dsnet commented Aug 13, 2018 •
edited
Loading

This is a regression that is breaking some tests. It's also a result of a change submitted 4 days ago. I could fix the broken tests, or we could revert the change causing this. But I believe some action should be made for Go1.11.

@griesemer
Copy link
Contributor

The change that was submitted 4 days ago also fixed a regression. Let's look at the broken tests (in person). And let's keep thinking about a possible fix for this.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/129255 mentions this issue: go/printer: revert "make empty lines break table alignment"

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/129256 mentions this issue: go/printer: consider empty lines in table layout computation

gopherbot pushed a commit that referenced this issue Aug 14, 2018
@griesemer
go/printer: consider empty lines in table layout computation
c882f4b 
In previous versions of Go including 1.10, an empty line would break the
alignment of elements within an expression list.

golang.org/cl/104755 changed the heuristic, with the side effect that
empty lines no longer broke the table alignment.

A prior fix (https://go-review.googlesource.com/c/go/+/125260, reverted)
introduced another regression (#26930) which this change doesn't produce.

Added test cases for both #26352 and #26930.

Fixes #26352.
Updates #26930.

Change-Id: I371f48e6f3620ebbab53f2128ec5e58bcd4a62f1
Reviewed-on: https://go-review.googlesource.com/129256
Run-TryBot: Robert Griesemer <gri@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Alan Donovan <adonovan@google.com>
@golang golang locked and limited conversation to collaborators Aug 14, 2019
@rsc rsc unassigned mvdan Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.12
Development

No branches or pull requests
4 participants
@mvdan @dsnet @griesemer @gopherbot