enforce groups of third-party and current module imports about gofumpt

[mvdan]

I want to think about this problem again. Here's a new proposal:

We enforce three groups of imports. Standard library (already enforced), third party, current module. Extra groups after these three are only allowed if they are preceded by a comment, which would normally explain why the extra import group is wanted. Otherwise, any extra import groups would be joined into the main three ones.

Here's an example of how this could look:

import (
    "encoding/json"
    "fmt"

    "google.golang.org/grpc"
    "google.golang.org/grpc/codes"

    "this.module/foo"
    "this.module/bar"

    // underscore imports for side effects go here
    _ "net/http/pprof"
    _ "some.third/party/plugin"
    _ "some.third/party/sql/driver"
)

[mvdan]

@zeebo brings up a good point on Slack; sometimes "local imports" are more than just the current module. Imagine a large company at myorg.com having many modules under the domain. In module myorg.com/foo, an import like myorg.com/bar should still be considered local. Their developers would use goimports -local=myorg.com, just like they'd use GOPRIVATE=myorg.com if their code was private.

So the mechanism proposed above isn't enough. We also can't rely on any file in the parent directory shared by all the modules, because the modules might live in different git repositories and be cloned in different places.

I also don't want to go back to requiring an explicit -local flag, because then every developer in that org needs to use exactly the same flag. The tool grows exponentially less useful if there are different ways to run it. A human can tell what "local" means in a module by just looking at it, so the tool should attempt to do the correct thing on its own.

So here's a suggested way to fix this:

1. Maintain a list of well-known code hosting sites, especially those housing different projects/organizations all at once. This would include github.com, gitlab.com, and so on. cmd/go has a similar hard-coded list, so we could start there.
2. Find what the module import path is for each Go directory (or package in the world of Modules). If there is none found, this entire feature is disabled and we don't alter the non-std groups at all.
3. With the module path found, we check if it begins with one of the known code hosting sites. If it does, we use it plus the next path element as the "local" prefix, e.g. github.com/myuser.
4. If the module path does not begin with a known code hosting site, we use the first element as the "local" prefix, e.g. myorg.com.

The list of well-known code hosting sites would actually be pretty short, because we don't need to include an endless array of self-hosted gitlabs, giteas, etc. For example, if a module's path is gitlab.gnome.org/foo/bar, the "local" entity or organization there is gitlab.gnome.org, because everything underneath it is Gnome-related.

During a transition period of a few months, the tool would support a GOFUMPT_LOCAL environment variable akin to a -local flag, so that the user can override the automatic detection in case it's wrong. The aim is to then find how many people need this environment variable and why, and attempt to fix any bugs before its support is removed.

[cristaloleg]

Looks like the template above is the most popular one:

• what everyone have(stdlib)
• what we use as a 3rd party
• what we wrote
• and some other underscore-ish

However 2 and 3 are often are swapped (also I prefer this style, 'cause internal project related things are more important)

[mvdan]

Would it still support the -local option?

@ryancurrah I think my comment above should answer your question now :)

[marcelloh]

You can test this, by having a library that doesn't exist on an external website.
(So it does not start with a domain name.)
gofumpt just assumes it must be an internal library.

[cristaloleg]

BTW, why not to check golang/go to see what is more popular 2-3 or 3-2 ?

Quick regex shows that cmd/go/internal/modcmd and packages near it has something like:

import (
	"context"
	"fmt"
	"os"
	"path/filepath"
	"runtime"
	"strings"

	"cmd/go/internal/base"
	"cmd/go/internal/cfg"
	"cmd/go/internal/load"
	"cmd/go/internal/search"
	"cmd/go/internal/str"
	"cmd/go/internal/vcs"
	"cmd/go/internal/web"
	"cmd/go/internal/work"

	"golang.org/x/mod/module"
)

[mvdan]

I don't think we can enforce one way to lay out the imports. Some people use two groups, some people use three groups, some people use many more. For example, I've seen underscore imports being a separate fourth group too.

Solving the "too many empty lines" problem is generally hard. It's the same with statements and declarations. For now, we're not solving that problem.

I guess we could group imports by prefix, for cases like the one you pasted just now. But the problem then is - what prefix? If we use the domain, we'd always join all of github.com, which is too agressive. If we use the use the first two or three path elements, that wouldn't support short module paths like mvdan.cc/sh or gioui.org.

If someone has a specific suggestion in mind, I'm happy to discuss it, but I can't think of a simple rule right now.

[mvdan]

@kyteague I don't understand. Whatever gofmt or goimports do, gofumpt shouldn't break.

This issue is about having gofumt enforce a specific grouping of third-party imports, similar to goimports -local. I couldn't come up with a sane way to do that, especially because different projects have different styles, hence the feature request issue was closed for the time being.

[mvdan]

Yes, please open a new issue. But to reiterate, gofmt does not organize imports in any way, it just sorts import groups.

[mvdan]

That being said, internal/... can never be imported from outside the standard library, so I guess we could assume it's not part of the standard library. I'll attempt a fix in a little bit.

This is now done; see the commit above.

[mvdan]

Have you seen #37? This is essentially a very hard problem to solve; see my comment there.

[ronnylt]

Thanks.

Do you have any other suggestion on how to solve this? Currently if not controlled by a human reviewer it can grow up to several groups.

package grpcserver_test

import (
	"context"
	"testing"


	"github.com/xxx/xxx/pkg/grpcserver"

	"google.golang.org/grpc"

	"google.golang.org/grpc/codes"

	"google.golang.org/grpc/status"
)

[marcelloh]

perhaps make it configurable.
We do separate the following groups:
internals, externals, our own libraries, the module packages.
now gofumpt smashes everything together, except for the externals :-(

[mvdan]

@marcelloh it should only ever join imports if they belong in the standard library. If that's not the case, please share a piece of code for me to reproduce the bug.

That's different to this issue, though. This issue is purely an enhancement, not a bug.

[mvdan]

@marcelloh that's #22, which in turn follows golang/go#32819. See #22 for the reason behind why it works like it does. If you disagree, you can comment there. Please don't comment here though, as this issue is entirely separate.

[mvdan]

I still think there's no clear direction we could take to improve the situation. Like I mentioned earlier, different projects have different setups with different numbers of import groups, and this is without mentioning goimports -local. So far, the only consistently enforced rule is that standard library imports must be in the first group, separate from other imports. We already do that bit.

I'm happy to hear proposals for specific changes or improvements we could make to that logic, such as "force the second group to be X", or "force underscore imports to be grouped together", or "force imports sharing a common path prefix to be in the same group". Note that I'm not endorsing any of these ideas, they are just examples of specific ideas we could look at.

[kyteague]

@mvdan what is wrong with gofmt's handling of imports (respecting groups and reordering within them)? This single issue is preventing us from adopting gofumpt.

[kyteague]

@mvdan Here is the difference between gofmt and gofumpt.

Original

package main

import (
	"net"
	"os"
	"strings"
	"time"

	"github.com/a"
	"github.com/c"
	"github.com/b"

	"internal/pkg2"
	"internal/pkg1"
)

gofmt

package main

import (
	"net"
	"os"
	"strings"
	"time"

	"github.com/a"
	"github.com/b"
	"github.com/c"

	"internal/pkg1"
	"internal/pkg2"
)

gofumpt

package main

import (
	"internal/pkg1"
	"internal/pkg2"
	"net"
	"os"
	"strings"
	"time"

	"github.com/a"
	"github.com/b"
	"github.com/c"
)

[mvdan]

That's a different issue, and sort of expected; internal/pkg1 is ambiguous because it doesn't have a dot like github.com, so we assume it belongs in the standard library. See golang/go#32819.

That being said, internal/... can never be imported from outside the standard library, so I guess we could assume it's not part of the standard library. I'll attempt a fix in a little bit.

[kyteague]

Happy to start a different issue if that makes more sense. The bigger problem is that gofumpt's output for imports is objectively worse than gofmt. Have groups of imports (system, 3rd party, org) is a fairly common convention that gofmt respects, but gofumpt does not. It would be great to at least have an option to disable the new functionality and fall back to the gofmt way of organizing imports.

[mvdan]

Just so I understand you - are you agreeing with my proposal above? There wouldn't be an option to swap the second and third groups. If too many people use them in reverse order, we could always allow both orders, but I would rather not.

[ryancurrah]

Would it still support the -local option?

[cristaloleg]

Sorry for a late reply @mvdan but yeah, totally agree. Also agree that having an option on changing groups is not stable (I mean there is no 1 constant output, as gofmt tools should be).

However I prefer having own package right after stdlib, if it still matters ;D

[cristaloleg]

Hi again, any plans on this? The question of import sorting popped-up in 1 chat, curious do you @mdvan have a stronger opinion on that or basically what are the open questions.

Thanks in advance.

[shashankram]

@mvdan is there a plan to address this? Thanks

[mvdan]

It is on my radar. Please keep in mind that there are dozens of these. I appreciate the enthusiasm, but asking for updates doesn't help - it just adds stress on my end. If you want to help, you can send good PRs or sponsor me so that I can make more free time.

[cristaloleg]

it just adds stress on my end

Sorry for that, I was asking in general, just to understand are there any blockers or open questions. Nothing more.

PRs or sponsor me so

thanks for the reminder, I forgot that I have changed my card but haven't updated sponsorship!

[mvdan]

Not aimed at anyone in particular, just in general :) No harm done. Only want to give my perspective so that others don't feel like I'm actively ignoring them.

[shashankram]

It is on my radar. Please keep in mind that there are dozens of these. I appreciate the enthusiasm, but asking for updates doesn't help - it just adds stress on my end. If you want to help, you can send good PRs or sponsor me so that I can make more free time.

@mvdan I was more curious if this is planned versus something you don't wish to incorporate. The tool is fantastic, and the only thing blocking us from using it is the fact that we can't group std, third-party, local imports similar to goimports -local. I hope you don't feel stressed about users asking for stuff, they are doing so because they are interested in using the great work you have put into building this tool.

[mvdan]

Understood. The issue remains open, meaning I mean to get it done at some point, or would accept PRs for it. I'd close it otherwise.