Go: x/tools/imports: use named imports for all import path/package name mismatches

[heschik]

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.

[bradfitz]

I feel like this was discussed elsewhere. I'll try to find it later.

I'm not entirely opposed to it. One additional benefit is that it kinda nudges people towards making them match, since their callers' imports will be uglier otherwise.

But I'm a little concerned that you might be proposing that the move to go/packages will make this path slower, at least on the first run (before it adds the proposed alias hint).

I've spent a fair bit of time making sure goimports is fast (the directory fastwalk stuff, the heuristics on which order we parse packages in to get a match sooner than later to minimize I/O and parsing, etc). I don't want to throw those speed benefits out with some move to an abstraction layer.

If anything, can't go/packages cache aggressively somewhere? disk/memory?

[heschik]

Sorry, hopefully not too redundant.

Yeah, moving it to go/packages is a big slowdown today, something like a factor of 20. It's not good. Russ has mentioned some thoughts about adding an on-disk cache to go list but nobody has immediate plans to do anything AFAIK.

We wouldn't merge go/packages into goimports until the performance hit is lower, but shelling out to go list will always have some cost. Also, we want to give early adopters a version of goimports that works with modules. If we don't make a change like this, it'll be annoyingly slow, but if we make the change only to the modules version then the two versions will conflict.

Basically, while this isn't a magic bullet, it simplifies the goimports code, makes the module version a lot faster in the relevant cases, the non-module version a tiny bit faster in all cases, and IMO makes the code more readable. So I think it's worth doing.

the heuristics on which order we parse packages in

For the record, we do lose this. I don't think there's any way to avoid it without calling go list multiple times, which is almost certainly worse than just biting the bullet.

[bradfitz]

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.

[heschik]

I haven't been in the strategic discussions, but my understanding is that Russ wants go list to be the only API for accessing package information. goimports by 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 way go list is 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. If go list is the only way to access package information in module information, and go list is 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.

[bradfitz]

Given that, I don't think it's helpful to blame the slowdown on go/packages

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 maybe go list needs some sort of hint/sort/streaming flags. I don't know because I don't know any of the designs.

[heschik]

Okay, but that's not what this bug was asking about. go list is 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 of goimports, just the modules version, or tell people to live with it/fix it manually. Do you have any opinion on that question?

[bradfitz]

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.

[gopherbot]

Change https://golang.org/cl/145699 mentions this issue: imports: create named imports for name/path mismatches

[dgryski]

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.

[heschik]

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.

[heschik]

I'm looking to submit CL 145699 soon. I'm guessing from the lack of comments that there aren't strong feelings here, so my inclination is to try the simplest thing and see how it goes. Please speak now if you disagree.

[magical]

@heschik Did you mean to close this issue?

[bradfitz]

Reopening.

[heschik]

It's been a week with no comment so I'm going to go ahead and submit.

(@magical, thanks for pointing out that I'd closed the issue, I had no idea that would happen and didn't get email about it)

[heschik]

Rolled back for #28645.

[gopherbot]

Change https://golang.org/cl/148878 mentions this issue: imports: create named imports for name/path mismatches (again)

[gopherbot]

Change https://golang.org/cl/152000 mentions this issue: imports: create named imports for name/path mismatches (again)