cmd/gofmt: applying gofmt to partial programs changes raw string literal values #4449
Labels
Comments
I've been working on fixing this bug, but in the process I've realized it may not be a
bug. Consider the following.
Note that the issue can only occur under these 2 conditions:
- Applying gofmt to a partial Go program.
- The partial Go code has some indent (i.e. first non-blank line has leading whitespace).
Now, consider the context where the above may happen. It could happen when you're trying
to include some partial Go code in the docs/comments for a func.
For example,
// if err!=nil {
// source:= strings.NewReader("line 1.\nline 2.\n")
// return source
// }
It may make sense to try and apply gofmt to that indented partial Go code (by taking the
"commented out text" value, which does not include the // comment symbols), which should
make it look like this:
// if err != nil {
// source := strings.NewReader("line 1.\nline 2.\n")
// return source
// }
Now consider what the equivalent comment should look like if it used a raw string
literal (with the same value). I would argue it makes the most sense to do:
// if err != nil {
// source := strings.NewReader(`line 1.
// line 2.
// `)
// return source
// }
I would argue that it makes most sense. When godoc parses it, it displays the code
correctly, see http://godoc.org/github.com/shurcooL/play/54#Issue4449Sample1.
Consider the alternatives:
// if err != nil {
// source := strings.NewReader(`line 1.
//line 2.
//`)
// return source
// }
// if err != nil {
// source := strings.NewReader(`line 1.
line 2.
`)
// return source
// }
The first one would no longer be parsed by godoc such that the code segment is
considered a single code segment (see
http://godoc.org/github.com/shurcooL/play/54#Issue4449Sample2). The second alternative
would render the original full Go program with the comment to be invalid, since a part
of the comment stops being a comment when // is removed (see
https://github.com/shurcooL/play/blob/90362c11c0d5560ec7eca380c2b6b062793a355c/54/main.go#L25-L40).
So, I think the best way to go forward is to not consider the leading indent in front of
partial Go code should to be a part of said partial Go code. If that's the case, then
the current gofmt behavior is correct - it does _not_ change the raw string literal
values. However, under that perspective, the current go/format behavior would become
incorrect, because it does change raw string literal value of partial indented Go code
(but that's a separate issue).
|
Although, I've just realized the current indent behavior cannot be correct either,
because it's not idempotent.
$ cat x.go
func f() {
const s = `
foo
`
println(s)
}
$ cat x.go | gofmt
func f() {
const s = `
foo
`
println(s)
}
$ cat x.go | gofmt | gofmt | gofmt | gofmt
func f() {
const s = `
foo
`
println(s)
}
Sigh. Trying to have "correct" behavior of preserving leading whitespace is really
tricky and I wish it was handled by external tools rather than gofmt itself. I don't see
a good solution now. Even if you try to strip the leading whitespace when parsing
partial Go code (to fix the lack of idempotence), what happens if some of the lines do
not contain the same leading whitespace? The value of the raw string literal in code
like this will become ambiguous:
func f() {
const s = `
foo
`
println(s)
}
|
Thank you for investigating the issue and uncovering the various problem scenarios (and welcome to the complicated world of gofmt...:-). I think the common case for partial program gofmt-ing is application inside an editor (emacs, etc.): A selected piece of code should be gofmt-ed; it's likely for that code to be indented because it's inside a function. In that case it shouldn't modify raw strings. The case of raw strings inside documentation comments seems much less likely. So I'd consider this still a bug. |
Fair enough, I'll continue to consider this a bug. Just an update here, I have code that fixes this and 5551 issues, but I haven't gotten around to making a CL out of it yet. You can see it here: https://github.com/shurcooL/go/compare/0f4891c007a...golang-issues-5551-4449 The code works and does the job (and all tests pass), but it's still very unclean and needs to be improved. I hope to do that as soon as possible... > I think the common case for partial program gofmt-ing is application inside an editor (emacs, etc.): > A selected piece of code should be gofmt-ed; it's likely for that code to be indented because it's > inside a function. In that case it shouldn't modify raw strings. I just want to comment that sounds very unusual. I see no reason why anyone would want to select and gofmt a part of a .go file instead of just having an on-save hook that runs either gofmt or goimports on the entire file (in emacs, vim, Sublime Text or any other editor). What would happen if you select a partial for statement, or start selection not from the beginning of a line? Why not gofmt the entire .go file? |
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.
$ cat x.go func f() { const s = ` foo ` println(s) } $ cat x.go | gofmt func f() { const s = ` foo ` println(s) } After gofmt, the raw string s is changed: it shouldn't be indented like the rest of the code.The text was updated successfully, but these errors were encountered: