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/format: Inconsistent behaviour against gofmt for partial Go programs, comments are stripped #5551

Closed
dmitshur opened this issue May 24, 2013 · 19 comments

Comments

@dmitshur
Copy link
Contributor

What steps will reproduce the problem?
If possible, include a link to a program on play.golang.org.
1. Use go/format's Source() to format a partial Go program, e.g.
http://play.golang.org/p/9YzQX0xWi1

What is the expected output?
i := 5 /* Comment */

(Comments should be preserved)

Open a terminal and perform: echo -n "i := 5 /* Comment */" | gofmt
The result is: i := 5 /* Comment */

What do you see instead?
i := 5

(Commets are stripped out)

Which compiler are you using (5g, 6g, 8g, gccgo)?
6g

Which operating system are you using?
OS X 10.8.3

Which version are you using?  (run 'go version')
go version go1.1 darwin/amd64

Please provide any additional information below.

Please see https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/lYEARys24oo
for a very detailed analysis of the root of the problem.
@nigeltao
Copy link
Contributor

Comment 1:

Owner changed to @griesemer.

@dmitshur
Copy link
Contributor Author

Comment 2:

I can volunteer to try my hand at writing a patch for this issue, if there isn't anyone
already working on it.
It would be my first time making a contribution to golang, so I'd have to learn about
the process as I go, but I'm quite willing to try if I get some confirmation that I
should go for it.

@griesemer
Copy link
Contributor

Comment 3:

Clearly a bug in go/format.
The actual fix in go/format/format.go is trivial (use a printer.CommentedNode), but
go/printer/printer.go needs to be adjusted as well (possibly should use an
ast.CommentMap instead of the existing code). I will get to this eventually.
Re #2, shurcooL: I'd prefer to fix this myself. This is not a good "first bug" to fix in
the system. Thanks.

Status changed to Accepted.

@dmitshur
Copy link
Contributor Author

Comment 4:

Ok, that makes sense. I suspected it's a not a good "first bug" to fix. Thanks for the
feedback.
Currently, the gofmt command doesn't make use of the functionality of go/format. What
are your thoughts on that? Is it something that's planned to be refactored one day so
there's only one codebase to maintain?

@griesemer
Copy link
Contributor

Comment 5:

Yes, ideally gofmt should use go/format.
If I remember correctly, I didn't do this when I wrote go/format because of some
non-trivial differences (gofmt does other stuff, too), but I may remember incorrectly. I
wrote go/format to factor out various other uses of the parsing and printing, which it
achieved.
I'll revisit this when I fix the issue (not the highest priority at the moment).

@minux
Copy link
Member

minux commented May 27, 2013

Comment 6:

@shurcooL, i'm glad that you want to help.
perhaps you could get started with issues labeled Suggested or ExpertNeeded.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 7:

Labels changed: added priority-later, go1.2maybe, removed priority-triage.

@rsc
Copy link
Contributor

rsc commented Jul 30, 2013

Comment 8:

Labels changed: added feature.

@robpike
Copy link
Contributor

robpike commented Aug 29, 2013

Comment 9:

Not for 1.2.

@robpike
Copy link
Contributor

robpike commented Aug 30, 2013

Comment 10:

Not for 1.2.

Labels changed: removed go1.2maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 11:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Nov 27, 2013

Comment 12:

Labels changed: removed feature.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 13:

Labels changed: added release-none, removed go1.3maybe.

@rsc
Copy link
Contributor

rsc commented Dec 4, 2013

Comment 14:

Labels changed: added repo-main.

@dmitshur
Copy link
Contributor Author

Comment 15:

Hi,
It's been over a year and I'd really like to see this bug fixed.
Another easier way of fixing it is to use the parse() from gofmt.
In doing so, I realized that go/format.Source() and gofmt have different output on one
of go/format's test cases.
"\n\t\t\n\n\t\t\tx := 0\n\t\t\tconst s = `\nfoo\n`\n\n\n", // no indentation inside raw
strings
go/format will not add indentation inside raw strings, but gofmt will. So simply copying
gofmt's behavior (which preserves comments in partial Go programs) will cause
go/format's tests to fail.
format $ go test
--- FAIL: TestPartial (0.00 seconds)
    format_test.go:123: formatting incorrect:
        source: "\n\t\t\n\n\t\t\tx := 0\n\t\t\tconst s = `\nfoo\n`\n\n\n"
        result: "\n\t\t\n\n\t\t\tx := 0\n\t\t\tconst s = `\n\t\t\tfoo\n\t\t\t`\n\n\n"
FAIL
exit status 1
Is that a bug that should be filed in cmd/gofmt? Which of the two behaviors should be
considered "correct"?

@griesemer
Copy link
Contributor

Comment 16:

gofmt should never change the contents of a literal, string or other. If you have a test
case, please file an issue against gofmt.
On the current issue: if you want to get your hands dirty, feel free to give it a shot.
Using gofmt's parse might indeed work - it does maintain comments.
The real underlying problem is that go/ast doesn't attach comments to nodes. I am
working on a larger-scale solution to fix that issue, but it's not quite ready yet (and
won't be for 1.4).

@dmitshur
Copy link
Contributor Author

Comment 17:

> On the current issue: if you want to get your hands dirty, feel free to give it a
shot. Using gofmt's parse might indeed work - it does maintain comments.
> 
> The real underlying problem is that go/ast doesn't attach comments to nodes. I am
working on a larger-scale solution to fix that issue, but it's not quite ready yet (and
won't be for 1.4).
Sounds good. I can definitely tackle this issue by now. Using parse() does preserve the
comments and I've already tested it out, you can see at
https://github.com/shurcooL/go/compare/golang-issue-5551. It makes sense to simply reuse
the code of gofmt until there's a larger-scale solution for go/ast with attached
comments, which can then be applied to both.
> gofmt should never change the contents of a literal, string or other.
Makes sense, so I'll open issue about the bug in gofmt. I'll have to fix that first
before I can start using gofmt's parse() in go/format to close this issue, otherwise
go/format tests will fail.

@gopherbot
Copy link

Comment 18:

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

@griesemer
Copy link
Contributor

Comment 19:

This issue was closed by revision 912ec19.

Status changed to Fixed.

dmitshur added a commit to shurcooL/go that referenced this issue Dec 24, 2014
Woohoo! golang/go#5551 and golang/go#4449 are fixed and available to
everyone in Go 1.4. Thank you for assistance @griesemer!
@golang golang locked and limited conversation to collaborators Jun 24, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
Fixes golang#5551.
Fixes golang#4449.

Adds tests for both issues.

Note that the two issues occur only when formatting partial Go code
with indent.

The best way to understand the change is as follows: I took the code
of cmd/gofmt and go/format, combined it into one unified code that
does not suffer from either 4449 nor 5551, and then applied that code
to both cmd/gofmt and go/format.

As a result, there is now much more identical code between the two
packages, making future code deduplication easier (it was not possible
to do that now without adding public APIs, which I was advised not to
do at this time).

More specifically, I took the parse() of cmd/gofmt which correctly
preserves comments (issue 5551) and modified it to fix issue where
it would sometimes modify literal values (issue 4449).

I ended up removing the matchSpace() function because it no longer
needed to do some of its work (insert indent), and a part of its work
had to be done in advance (determining the indentation of first code
line), because that calculation is required for cfg.Fprint() to run.

adjustIndent is used to adjust the indent of cfg.Fprint() to compensate
for the body of wrapper func being indented by one level. This allows
to get rid of the bytes.Replace text manipulation of inner content,
which was problematic and sometimes altered raw string literals (issue
4449). This means that sometimes the value of cfg.Indent is negative,
but that works as expected.

So now the algorithm for formatting partial Go code is:

1. Determine and prepend leading space of original source.
2. Determine and prepend indentation of first code line.
3. Format and write partial Go code (with all of its leading &
   trailing space trimmed).
4. Determine and append trailing space of original source.

LGTM=gri
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://golang.org/cl/142360043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018
Fixes golang#5551.
Fixes golang#4449.

Adds tests for both issues.

Note that the two issues occur only when formatting partial Go code
with indent.

The best way to understand the change is as follows: I took the code
of cmd/gofmt and go/format, combined it into one unified code that
does not suffer from either 4449 nor 5551, and then applied that code
to both cmd/gofmt and go/format.

As a result, there is now much more identical code between the two
packages, making future code deduplication easier (it was not possible
to do that now without adding public APIs, which I was advised not to
do at this time).

More specifically, I took the parse() of cmd/gofmt which correctly
preserves comments (issue 5551) and modified it to fix issue where
it would sometimes modify literal values (issue 4449).

I ended up removing the matchSpace() function because it no longer
needed to do some of its work (insert indent), and a part of its work
had to be done in advance (determining the indentation of first code
line), because that calculation is required for cfg.Fprint() to run.

adjustIndent is used to adjust the indent of cfg.Fprint() to compensate
for the body of wrapper func being indented by one level. This allows
to get rid of the bytes.Replace text manipulation of inner content,
which was problematic and sometimes altered raw string literals (issue
4449). This means that sometimes the value of cfg.Indent is negative,
but that works as expected.

So now the algorithm for formatting partial Go code is:

1. Determine and prepend leading space of original source.
2. Determine and prepend indentation of first code line.
3. Format and write partial Go code (with all of its leading &
   trailing space trimmed).
4. Determine and append trailing space of original source.

LGTM=gri
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://golang.org/cl/142360043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
Fixes golang#5551.
Fixes golang#4449.

Adds tests for both issues.

Note that the two issues occur only when formatting partial Go code
with indent.

The best way to understand the change is as follows: I took the code
of cmd/gofmt and go/format, combined it into one unified code that
does not suffer from either 4449 nor 5551, and then applied that code
to both cmd/gofmt and go/format.

As a result, there is now much more identical code between the two
packages, making future code deduplication easier (it was not possible
to do that now without adding public APIs, which I was advised not to
do at this time).

More specifically, I took the parse() of cmd/gofmt which correctly
preserves comments (issue 5551) and modified it to fix issue where
it would sometimes modify literal values (issue 4449).

I ended up removing the matchSpace() function because it no longer
needed to do some of its work (insert indent), and a part of its work
had to be done in advance (determining the indentation of first code
line), because that calculation is required for cfg.Fprint() to run.

adjustIndent is used to adjust the indent of cfg.Fprint() to compensate
for the body of wrapper func being indented by one level. This allows
to get rid of the bytes.Replace text manipulation of inner content,
which was problematic and sometimes altered raw string literals (issue
4449). This means that sometimes the value of cfg.Indent is negative,
but that works as expected.

So now the algorithm for formatting partial Go code is:

1. Determine and prepend leading space of original source.
2. Determine and prepend indentation of first code line.
3. Format and write partial Go code (with all of its leading &
   trailing space trimmed).
4. Determine and append trailing space of original source.

LGTM=gri
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://golang.org/cl/142360043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018
Fixes golang#5551.
Fixes golang#4449.

Adds tests for both issues.

Note that the two issues occur only when formatting partial Go code
with indent.

The best way to understand the change is as follows: I took the code
of cmd/gofmt and go/format, combined it into one unified code that
does not suffer from either 4449 nor 5551, and then applied that code
to both cmd/gofmt and go/format.

As a result, there is now much more identical code between the two
packages, making future code deduplication easier (it was not possible
to do that now without adding public APIs, which I was advised not to
do at this time).

More specifically, I took the parse() of cmd/gofmt which correctly
preserves comments (issue 5551) and modified it to fix issue where
it would sometimes modify literal values (issue 4449).

I ended up removing the matchSpace() function because it no longer
needed to do some of its work (insert indent), and a part of its work
had to be done in advance (determining the indentation of first code
line), because that calculation is required for cfg.Fprint() to run.

adjustIndent is used to adjust the indent of cfg.Fprint() to compensate
for the body of wrapper func being indented by one level. This allows
to get rid of the bytes.Replace text manipulation of inner content,
which was problematic and sometimes altered raw string literals (issue
4449). This means that sometimes the value of cfg.Indent is negative,
but that works as expected.

So now the algorithm for formatting partial Go code is:

1. Determine and prepend leading space of original source.
2. Determine and prepend indentation of first code line.
3. Format and write partial Go code (with all of its leading &
   trailing space trimmed).
4. Determine and append trailing space of original source.

LGTM=gri
R=golang-codereviews, bradfitz, gri
CC=golang-codereviews
https://golang.org/cl/142360043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

7 participants