seems to split imports into three groups in some edge cases

[mmlb]

This is pretty strange but I noticed that gofumpt was creating unexpected groups in import (...) blocks. I've tracked it down to gofumpt doing the wrong thing when path/filepath is imported (maybe others?). Here's what I'm seeing.

I've seen the following on both v0.3.1 and latest master commit:

[16:42:45]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> git status -s
?? gofumpt_does_not_like_path_filepath
                                                                                                                                                                      
[16:42:47]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> git rev-parse HEAD
b5619f8b06cad833eac1c454a48024599d8f5192

With path/filepath in the imports:

[16:47:18]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package tests_test

import (
	"path/filepath"
	"time"
	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
	"k8s.io/apimachinery/pkg/types"
	"sigs.k8s.io/yaml"
)
                                                                                                                                                                      
[16:47:24]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-25 16:47:24.556880440 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-25 16:47:24.556880440 -0400
@@ -3,7 +3,9 @@
 import (
 	"path/filepath"
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
+
 	"sigs.k8s.io/yaml"
 )

and now without path/filepath:

[16:47:51]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package tests_test

import (
	"time"
	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
	"k8s.io/apimachinery/pkg/types"
	"sigs.k8s.io/yaml"
)
                                                                                                                                                                      
[16:47:53]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath.go
--- gofumpt_does_not_like_path_filepath.orig	2022-04-25 16:47:53.213097982 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-25 16:47:53.213097982 -0400
@@ -2,6 +2,7 @@
 
 import (
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/yaml"

git bisect says b7afc71 is the first bad commit. This is the commit where stdlib is separated from other imports. Before this commit all the commits are kept grouped together, but at least handled right.

Still with path/filepath in the imports:

[16:57:13]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> git checkout b7afc715b566b948526ad840fc20d53b642f6b6c
Previous HEAD position was 4bef639 Update README.md
HEAD is now at b7afc71 join all standard library imports
                                                                                                                                                                      
[16:57:15]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> rm gofumpt; go build; ./gofumpt -d gofumpt_does_not_like_path_filepath 
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-25 16:57:16.125345285 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-25 16:57:16.125345285 -0400
@@ -5,5 +5,6 @@
 	"time"
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
+
 	"sigs.k8s.io/yaml"
 )
                                                                                                                                                                      
[16:57:23]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> git checkout b7afc715b566b948526ad840fc20d53b642f6b6c~
Previous HEAD position was b7afc71 join all standard library imports
HEAD is now at 4bef639 Update README.md
                                                                                                                                                                      
[16:57:24]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> rm gofumpt; go build; ./gofumpt -d gofumpt_does_not_like_path_filepath 
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-25 16:57:24.571408753 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-25 16:57:24.571408753 -0400
@@ -1,9 +1,9 @@
 package tests_test
 
 import (
-	"path/filepath"
-	"time"
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
+	"path/filepath"
 	"sigs.k8s.io/yaml"
+	"time"
 )

[mmlb]

path/filepath -> path same deal, path -> encoding/json does the right thing...

[17:08:52]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package tests_test

import (
	"path"
	"time"
	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
	"k8s.io/apimachinery/pkg/types"
	"sigs.k8s.io/yaml"
)
                                                                                                                                                                      
[17:08:54]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-25 17:08:54.102578903 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-25 17:08:54.102578903 -0400
@@ -3,7 +3,9 @@
 import (
 	"path"
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
+
 	"sigs.k8s.io/yaml"
 )
                                                                                                                                                                      
[17:09:03]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package tests_test

import (
	"enconding/json"
	"time"
	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
	"k8s.io/apimachinery/pkg/types"
	"sigs.k8s.io/yaml"
)
                                                                                                                                                                      
[17:09:06]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-25 17:09:06.095668682 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-25 17:09:06.095668682 -0400
@@ -3,6 +3,7 @@
 import (
 	"enconding/json"
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/yaml"

[mvdan]

Not sure I understand; gofumpt is meant to separate imports into groups, where the first group is the standard library. Isn't that what you are seeing?

[mmlb]

@mvdan not quite. gofumpt is creating 3 import groups instead of just 2. I was expecting

 import (
 	"path"
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/yaml"
 )

And it does do that if I don't have path in the import block. FYI I don't think the issue is with path itself for all cases, its just what seems to trigger this unexpected behavior in my imports. Here's another buggy example not tied to path:

Only 2 groups as expected:

[09:49:20]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package main

import (
	"encoding/json"
	"time"
	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
	"k8s.io/apimachinery/pkg/types"
	"k8s.io/apimachinery/pkg/types2"
	"sigs.k8s.io/yaml"
)
                                                                                                                                                                      
[09:49:23]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath 
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-26 09:49:23.724699015 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-26 09:49:23.724699015 -0400
@@ -3,6 +3,7 @@
 import (
 	"encoding/json"
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/types2"
                                                                                                                                                                      

But now I add package zz.zz/foo/bar and now we get 3 groups, unexpectedly:

[09:49:40]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package main

import (
	"encoding/json"
	"time"
	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
	"k8s.io/apimachinery/pkg/types"
	"k8s.io/apimachinery/pkg/types2"
	"sigs.k8s.io/yaml"
	"zz.zz/foo/bar"
)
                                                                                                                                                                      
[09:49:46]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath 
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig	2022-04-26 09:49:46.932899018 -0400
+++ gofumpt_does_not_like_path_filepath	2022-04-26 09:49:46.932899018 -0400
@@ -3,9 +3,11 @@
 import (
 	"encoding/json"
 	"time"
+
 	"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/types2"
 	"sigs.k8s.io/yaml"
+
 	"zz.zz/foo/bar"
 )

[mvdan]

Thank you - that is very weird, and I can reproduce.

[mvdan]

It seems like what happens here is that our logic to reorder imports doesn't quite work in some edge cases - which is understandable given that we have to fix up the position offsets, and doing that while keeping empty lines and comments where they should be is... surprisingly hard. Properly fixing this might require a bit of a rethink.

[mmlb]

yeah when I went trawling for an easy fix I noticed the same and thus brought the issue up instead of opening a PR :)