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: applying gofmt to partial programs changes raw string literal values #4449

Closed
griesemer opened this issue Nov 26, 2012 · 15 comments
Closed
Assignees

Comments

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 26, 2012

$ 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.
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Nov 27, 2012

Comment 1:

A simpler example:
$ cat x.go
    const s = `
foo
`
$ cat x.go | gofmt
    const s = `
    foo
    `
@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2012

Comment 2:

Labels changed: added priority-later, removed priority-triage.

@rsc
Copy link
Contributor

@rsc rsc commented Mar 11, 2013

Comment 3:

I think it's too late to be changing gofmt right now. We're going to have to keep living
with this bug (so far it doesn't seem to have caused much trouble).

Labels changed: added go1.2, removed go1.1.

@dsymonds
Copy link
Member

@dsymonds dsymonds commented Aug 21, 2013

Comment 4:

There doesn't seem time or interest for this making it into Go1.2.

Labels changed: removed go1.2.

@rsc
Copy link
Contributor

@rsc rsc commented Nov 27, 2013

Comment 5:

Labels changed: added go1.3maybe.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 6:

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

@rsc
Copy link
Contributor

@rsc rsc commented Dec 4, 2013

Comment 7:

Labels changed: added repo-main.

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 1, 2014

Comment 8:

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).
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 2, 2014

Comment 9:

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)
    }
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Sep 2, 2014

Comment 10:

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.
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 15, 2014

Comment 11:

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?
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Sep 17, 2014

Comment 12:

Re: your last comment in #11: It's not as unusual as it may seem; that's the reason why
gofmt has so much (pretty ugly) machinery to handle this case. One reason might be that
there's only a partial package file present, or that parts of the file are syntactically
incomplete.
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Sep 17, 2014

Comment 13:

Re: your last comment in #11: It's not as unusual as it may seem; that's the reason why
gofmt has so much (pretty ugly) machinery to handle this case. One reason might be that
there's only a partial package file present, or that parts of the file are syntactically
incomplete.
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 22, 2014

Comment 14:

CL https://golang.org/cl/142360043 mentions this issue.
@griesemer
Copy link
Contributor Author

@griesemer griesemer commented Sep 30, 2014

Comment 15:

This issue was closed by revision 912ec19.

Status changed to Fixed.

@griesemer griesemer self-assigned this Sep 30, 2014
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 added 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 added 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 added 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 added 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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.