non-idempotent runs

[twmb]

echo "package main; func (v *OffsetForLeaderEpochResponse) Throttle() (int32, bool) { return v.ThrottleMillis, v.Version >= 0 }; func (v *OffsetForLeaderEpochResponse) RequestKind() Request { return &OffsetForLeaderEpochRequest{Version: v.Version} }" | gofumpt

output:

package main

func (v *OffsetForLeaderEpochResponse) Throttle() (int32, bool) {
	return v.ThrottleMillis, v.Version >= 0
}
func (v *OffsetForLeaderEpochResponse) RequestKind() Request {
	return &OffsetForLeaderEpochRequest{Version: v.Version}
}

If we pipe the expression through gofumpt twice, we get an additional newline between Throttle and RequestKind:

echo "package main; func (v *OffsetForLeaderEpochResponse) Throttle() (int32, bool) { return v.ThrottleMillis, v.Version >= 0 }; func (v *OffsetForLeaderEpochResponse) RequestKind() Request { return &OffsetForLeaderEpochRequest{Version: v.Version} }" | gofumpt | gofumpt

output:

package main

func (v *OffsetForLeaderEpochReqsponse) Throttle() (int32, bool) {
	return v.ThrottleMillis, v.Version >= 0
}

func (v *OffsetForLeaderEpochResponse) RequestKind() Request {
	return &OffsetForLeaderEpochRequest{Version: v.Version}
}

[mvdan]

Thanks for raising this issue. I admit I've only been testing gofumpt on inputs which are reasonably gofmt-like. If your entire input is crammed into a single line, or has almost no whitespace between tokens, it's not surprising that gofumpt isn't idempotent straight away.

We already do enforce gofmt's style, because under the hood this is a gofmt fork. However, gofumpt's own extra rules work at the level of the input go/ast, so our ability to insert newlines or move code around is relatively limited. I can go into more detail there if it sounds interesting.

One option would be for gofumpt to first run gofmt on the entire input, and afterwards enforce its extra rules as a second pass. This could possibly make gofumpt somewhat slower, but also fix a lot of these idempotency edge cases.

Another option is to try to fix all of these idempotency edge cases one by one. I can certainly try, but I worry that some would be extremely hard to fix, and it would be hard to tell when we're truly "done".

[mvdan]

I see what you did in your generator to run print-code | gofumpt | gofumpt. It would be a bit simpler to do print-code | gofmt | gofumpt instead, and I might make that suggestion in the docs for code generators that don't print gofmt-like output.

[twmb]

Makes sense. I don't fully understand how a repeated run is able to insert an extra newline, that would be interesting to know. I wonder if there's a more general way to ensure idempotence -- it's difficult for me to understand from an I-currently-know-nothing perspective what additional behavior triggers based on differences of input (in this case, an extra newline if things are mostly formatted).

[cespare]

@mvdan I encountered another case where the output of gofumpt is not gofumpt-safe. Example:

package main

import "strings"
import "bytes"

func main() {
	_ = bytes.Buffer
	_ = strings.Builder
}

Running gofumpt once gives:

package main

import (
        "strings"
        "bytes"
)

func main() {
        _ = bytes.Buffer
        _ = strings.Builder
}

Running it twice gives:

package main

import (
        "bytes"
        "strings"
)

func main() {
        _ = bytes.Buffer
        _ = strings.Builder
}

This came up when I ran the output of a protobuf code generator through gofumpt and found that I needed to run it twice before it stopped changing.

Should I file a separate bug? To me, grouping and sorting the imports is really something that gofumpt ought to be able to do in a single pass. (In general I do think code formatters ought to always reach a fixed point after a single iteration, but I take from your previous comments that there are practical reasons about the way gofumpt is written which makes that difficult.)

[mvdan]

Using the same bug seems fine; thanks for reporting! I do intend to fix non-idempotency bugs, even if some might be tricky or require slowing down the tool a bit.

[mvdan]

The last example is very similar to #225. In both edge cases, moving imports around results in bad formatting. I just can't fix these with the current mechanism of modifying go/ast; I will need a different approach entirely.