Skip to content

Commit a501ae3

Browse files
committed
test and document why we use go-cmp
I also had broken the logic to always return true in a very recent commit since the last release, for some odd reason. Perhaps I was checking that tests for #316 were missing by intentionally breaking the logic? Clearly I forgot that I did that. Fixes #316.
1 parent 66262a4 commit a501ae3

File tree

3 files changed

+20
-6
lines changed

3 files changed

+20
-6
lines changed

Diff for: format/format.go

+11-5
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import (
2323
"unicode"
2424
"unicode/utf8"
2525

26-
// "github.com/google/go-cmp/cmp"
26+
"github.com/google/go-cmp/cmp"
2727
"golang.org/x/tools/go/ast/astutil"
2828

2929
"mvdan.cc/gofumpt/internal/govendor/go/format"
@@ -1036,16 +1036,22 @@ func (f *fumpter) shouldMergeAdjacentFields(f1, f2 *ast.Field) bool {
10361036
return false
10371037
}
10381038

1039-
// Only merge if the types are equal.
1040-
// opt := cmp.Comparer(func(x, y token.Pos) bool { return true })
1041-
return true
1039+
// Only merge if the types that the syntax nodes represent are equal,
1040+
// e.g. two *ast.Ident nodes "int" are equal, but the two *ast.Ident nodes
1041+
// "string" and "bool" are not. Hence we use go-cmp to do deep comparisons
1042+
// while ignoring position information, as it is irrelevant.
1043+
//
1044+
// Note that we could in theory use go/types here, but in practice gofumpt
1045+
// needs to be fast, hence it shouldn't rely on expensive typechecking.
1046+
opt := cmp.Comparer(func(x, y token.Pos) bool { return true })
1047+
return cmp.Equal(f1.Type, f2.Type, opt)
10421048
}
10431049

10441050
var posType = reflect.TypeOf(token.NoPos)
10451051

10461052
// setPos recursively sets all position fields in the node v to pos.
10471053
func setPos(v reflect.Value, pos token.Pos) {
1048-
if v.Kind() == reflect.Ptr {
1054+
if v.Kind() == reflect.Pointer {
10491055
v = v.Elem()
10501056
}
10511057
if !v.IsValid() {

Diff for: go.mod

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ go 1.23.0
44

55
require (
66
github.com/go-quicktest/qt v1.101.0
7+
github.com/google/go-cmp v0.6.0
78
github.com/rogpeppe/go-internal v1.14.1
89
golang.org/x/mod v0.24.0
910
golang.org/x/sync v0.13.0
@@ -12,7 +13,6 @@ require (
1213
)
1314

1415
require (
15-
github.com/google/go-cmp v0.6.0 // indirect
1616
github.com/kr/pretty v0.3.1 // indirect
1717
github.com/kr/text v0.2.0 // indirect
1818
)

Diff for: testdata/script/func-merge-parameters.txtar

+8
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,10 @@ func dontMergeMultipleLines2(
5151
z int,
5252
) {
5353
}
54+
55+
func dontMergeDifferentKinds(format string, args ...string) {}
56+
57+
func dontMergeDifferentTypesReturn() (n int, err error) {}
5458
-- foo.go.golden --
5559
package p
5660

@@ -93,3 +97,7 @@ func dontMergeMultipleLines2(
9397
z int,
9498
) {
9599
}
100+
101+
func dontMergeDifferentKinds(format string, args ...string) {}
102+
103+
func dontMergeDifferentTypesReturn() (n int, err error) {}

0 commit comments

Comments
 (0)