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

cmd/gofmt: comment lines broken at new position #18782

Closed
kortschak opened this issue Jan 25, 2017 · 11 comments
Closed

cmd/gofmt: comment lines broken at new position #18782

kortschak opened this issue Jan 25, 2017 · 11 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@kortschak
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go1.8rc2

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOOS="linux"

What did you do?

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
Run gofmt -d on https://github.com/biogo/biogo/blob/master/align/matrix/matrices.go

What did you expect to see?

No difference.

What did you see instead?

Log output here https://travis-ci.org/biogo/biogo/jobs/195029064#L501

@griesemer
Copy link
Contributor

griesemer commented Jan 25, 2017

This appears to be a regression from 1.7. The problem doesn't appear with https://play.golang.org/p/iptkp861qM (simple repro case) in the playground. But with 1.8 we get:

package p

var _ = [][]int{
	/*       a, b, c, d, e */
	/* a */
	{0, 0, 0, 0, 0},
	/* b */ {0, 5, 4, 4, 4},
	/* c */ {0, 4, 5, 4, 4},
	/* d */ {0, 4, 4, 5, 4},
	/* e */ {0, 4, 4, 4, 5},
}

Likely related to https://go-review.googlesource.com/#/c/33016/ (need to confirm).

It's probably too late to get this fixed for 1.8 unless there's a trivial mistake. Unrolling the previous change (if it's the culprit) would expose two other issues that it fixed.

@griesemer griesemer added this to the Go1.9 milestone Jan 25, 2017
@griesemer griesemer added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 25, 2017
@griesemer griesemer modified the milestones: Go1.9Early, Go1.9 Jan 25, 2017
@bradfitz
Copy link
Contributor

@griesemer, you're confident this is sufficiently rare to push off to Go 1.9?

@griesemer
Copy link
Contributor

@bradfitz I haven't seen this before and none of the code we regularly gofmt appears to have the problem. That is all I can say at the moment. I will look into the problem today - perhaps there's a safe and easy fix.

@griesemer
Copy link
Contributor

Confirming that https://go-review.googlesource.com/#/c/33016/ (commit a0d2e96) is indeed the culprit. The bug is not present before (commit 8cd5561).

@kortschak
Copy link
Contributor Author

@griesemer, you're confident this is sufficiently rare to push off to Go 1.9?

As a data point, this breaks my CI as I perform automated gomft checks on that repo. The failure masks other real failures.

@griesemer
Copy link
Contributor

@kortschak Understood. I've started working on this.

@kortschak
Copy link
Contributor Author

@griesemer Thank you

@griesemer
Copy link
Contributor

@kortschak Not a completely satisfactory "solution", but a work-around: The following source

var _ = [][]int{ /* a, b, c, d, e */
	/*    a */ {0, 0, 0, 0, 0},
	/*    b */ {0, 5, 4, 4, 4},
	/*    c */ {0, 4, 5, 4, 4},
	/*    d */ {0, 4, 4, 5, 4},
	/*    e */ {0, 4, 4, 4, 5},
}

(first comment on same line as /*) will be preserved. Still trying to see if there's a simple (1.8-acceptable solution).

@griesemer
Copy link
Contributor

@kortschak Can you verify that https://go-review.googlesource.com/35811 fixes the problem for you? Thanks.

@gopherbot
Copy link

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

@kortschak
Copy link
Contributor Author

@griesemer Yes, that fixes the problem. Thanks.

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

No branches or pull requests

4 participants