go: x/tools/imports: use named imports for all import path/package name mismatches
Some packages’ import paths don’t match their package name. The only way to know for sure what package an import path contains is to parse it a little. That means that you can’t tell just by looking at a Go file whether all its imports are satisfied, because you don’t know for sure which import statements provide which package names.
goimports does its parsing in the importPathToNameGoPathParse function. As the name suggests, it’s GOPATH-specific. In module mode, mapping an import path to a package is much more costly – you have to run go list, perhaps via go/packages. Having that call in the middle of the fast path of goimports is not ideal, especially today where it can take a good chunk of a second to run.
After talking about this with @ianthehat, our proposal is that goimports add an import alias for all mismatched packages. (Version suffixes would be okay.) That would make the fast path of goimports purely syntactic, so it can be even faster than today. We think it’s also clearer for readers.
@bradfitz, does this sound good to you? It may be pretty noticeable to some users but hopefully they’ll like it.
About this issue
- Original URL
- State: closed
- Created 6 years ago
- Reactions: 3
- Comments: 17 (14 by maintainers)
Commits related to this issue
- Fix goimports issue goimports has been changed to add named imports when the imported package path does not match the package name: https://github.com/golang/go/issues/28428. We did not use named im... — committed to Juniper/contrail by deleted user 6 years ago
- More goimports Recent version of goimports committed a few days ago now adds an explicit package name tag for imports whose last path component differ from the package name (see golang/go#28428 and h... — committed to harmony-ek/harmony by deleted user 5 years ago
- lint: Enable goimports goimports checks import lines, adding missing ones and removing unreferenced ones: https://godoc.org/golang.org/x/tools/cmd/goimports It also requires named imports for packag... — committed to linkerd/linkerd2 by siggy 5 years ago
- lint: Enable goimports goimports checks import lines, adding missing ones and removing unreferenced ones: https://godoc.org/golang.org/x/tools/cmd/goimports It also requires named imports for packag... — committed to linkerd/linkerd2 by siggy 5 years ago
- lint: Enable goimports (#2366) goimports checks import lines, adding missing ones and removing unreferenced ones: https://godoc.org/golang.org/x/tools/cmd/goimports It also requires named import... — committed to linkerd/linkerd2 by siggy 5 years ago
- Updated goimports; did make format; go mod tidy. Main change is the import alias: https://github.com/golang/go/issues/28428 Quite annoying but nothing much we can do. Signed-off-by: Bartek Plotka <b... — committed to thanos-io/thanos by bwplotka 5 years ago
- Updated goimports; did make format; go mod tidy. Main change is the import alias: https://github.com/golang/go/issues/28428 Quite annoying but nothing much we can do. Signed-off-by: Bartek Plotka <b... — committed to thanos-io/thanos by bwplotka 5 years ago
- Updated goimports; did make format; go mod tidy. (#1143) Main change is the import alias: https://github.com/golang/go/issues/28428 Quite annoying but nothing much we can do. Signed-off-by: Barte... — committed to thanos-io/thanos by bwplotka 5 years ago
I wonder if common mismatches should be ignored? There are a lot of repos out there with a prefix of
go-or a suffix of-go.I’m fine with always adding the aliases in any/all versions of goimports, and doing so at any time.
Sorry for the tangent. This bug just made me (perhaps rightfully) scared of upcoming performance problems.
I’m open to the idea, but IMO we need some principle behind what we do and don’t make exceptions for. Without that, this feels like a slippery slope to me. I would rather add a few spurious aliases than confuse people.
That said, dashes are a relatively easy case, since they’re not valid in package names. There’s currently some logic that handles them by removing the dashes altogether and doing a substring match; I think I’d rather do something like check every combination of fragments. It’ll complicate the code a bit, but it wouldn’t be the end of the world.
This worries me.
Before goimports moves to a new in-development abstraction layer, I think the new abstraction layer needs to consider the needs of one of its early users and be designed (& implemented) accordingly.
20x or even 2x performance loss is pretty sad. 2x would be more palatable, though.
Okay, but that’s not what this bug was asking about.
go listis not going to get faster before 1.13 at the earliest, and we still need to give early adopters something before then. We can make this change in all versions ofgoimports, just the modules version, or tell people to live with it/fix it manually. Do you have any opinion on that question?I’m not blaming any specific piece. I haven’t been following closely enough to even be able to do that. When I say “go/packages” I just mean “all the new stuff”, whatever that may be. I just want the new stuff to coordinate with its own pieces to be able to do what goimports needs, quickly.
If we have to go through
go list, then maybego listneeds some sort of hint/sort/streaming flags. I don’t know because I don’t know any of the designs.I haven’t been in the strategic discussions, but my understanding is that Russ wants
go listto be the only API for accessing package information.goimportsby definition needs to load first the package being analyzed, and then (if necessary) the candidate imports. Those calls are almost the entire performance difference, accounting for between 70-95% of the total runtime of the test cases I looked at. There is very little room for optimization of the waygo listis called. There might be some slack in the loading of type information, but it’s not the main problem right now.Given that, I don’t think it’s helpful to blame the slowdown on
go/packages. Ifgo listis the only way to access package information in module information, andgo listis slow, then everything that supports modules is going to be slow, regardless of any abstraction layers.Again, nobody is saying that this is going to be merged until the performance is better. We are taking the same experimental approach with many other tools, though admittedly most of them are less performance sensitive or run as a server with some form of in-memory caching. See #24661.