go/format: Inconsistent behaviour against gofmt for partial Go programs, comments are stripped #5551
Labels
Comments
|
Owner changed to @griesemer. |
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. |
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. |
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? |
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). |
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"?
|
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). |
> 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. |
CL https://golang.org/cl/142360043 mentions this issue. |
|
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!
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.
The text was updated successfully, but these errors were encountered: