This restores the concurrent walking that was removed in CL 508506,
since that concurrency turns out to actually matter in practice
(see golang/go#44863), but uses a different (and in my opinion simpler)
concurrency pattern based on the one shown in my 2018 GopherCon talk
(https://drive.google.com/file/d/1nPdvhB0PutEJzdCq5ms6UI58dp50fcAN/view,
slide 114), and removes the arbitrary 4-goroutine minimum.
On my machine this speeds up the benchmark from CL 561436
by a factor of around 3½.
goos: linux
goarch: amd64
pkg: golang.org/x/tools/internal/imports
cpu: Intel(R) Xeon(R) CPU @ 2.20GHz
│ before.txt │ after.txt │
│ sec/op │ sec/op vs base │
ModuleResolver_InitialScan-24 1728.0m ± 5% 505.2m ± 7% -70.76% (p=0.000 n=10)
Fixesgolang/go#65531.
Updates golang/go#44863.
Change-Id: I082bb3375f7775d55d130bf75ae71f53312aace1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/561675
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
filepath.WalkDir may invoke its callback with a nil DirEntry and
non-nil error — a detail I missed when converting from fastwalk.Walk
to filepath.WalkDir in CL 508506. Even though gopathwalk.Walk ignores
filesystem errors, its callback needs to check for that one.
Looking into this bug, I also realized that the walk callback used to
return only SkipDir or ErrTraverseLink. In retrospect, that makes
sense given that Walk and WalkDir don't return errors, so I have
documented that property and fixed a few places where it no longer
held.
To help to simplify the code enough to rule out other similar errors,
we also inline walker.shouldTraverse and simplify the case for
traversing symlinks. This results in slight changes in the visited
package directories when a root contains cyclic symlink paths,
but since we never expect symlink cycles to occur in the first place
I don't expect that the changes will affect any users in practice.
Fixesgolang/go#58054.
Updates golang/go#58035.
Change-Id: Ie8cf1e4b5186b0d7b4650305bfc863330541f080
Reviewed-on: https://go-review.googlesource.com/c/tools/+/539480
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Previously we used os.SameFile, but that is needlessly expensive when
we expect the original (user-provided) paths to be preserved during
the walk: the user-provided list of ignored directories is relative.
Because 'go list' does not traverse symlinks, we do not expect a
user's source roots to contain symlinks for Go package source either,
and in the rare even that the tree contains a directory that resides
within a non-ignored subdirectory symlink, the user can explicitly
include all of the relative paths by which the directory may be
encountered.
This reduces benchmark latency by ~7.5% compared to CL 508506, bringing
the overall latency to +~6% over the previous approach using
internal/fastwalk.
~/x/tools$ benchstat cl508506.txt cl508507.txt
goos: linux
goarch: amd64
pkg: golang.org/x/tools/internal/imports
cpu: AMD EPYC 7B12
│ cl508506.txt │ cl508507.txt │
│ sec/op │ sec/op vs base │
ScanModCache-24 264.4m ± 1% 244.6m ± 1% -7.49% (p=0.000 n=10)
~/x/tools$ benchstat cl508505.txt cl508507.txt
goos: linux
goarch: amd64
pkg: golang.org/x/tools/internal/imports
cpu: AMD EPYC 7B12
│ cl508505.txt │ cl508507.txt │
│ sec/op │ sec/op vs base │
ScanModCache-24 231.0m ± 1% 244.6m ± 1% +5.90% (p=0.000 n=10)
For golang/go#58035.
Change-Id: I937faf7793b8fad10a88b2fdc21fa4e4001c7246
Reviewed-on: https://go-review.googlesource.com/c/tools/+/508507
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
golang.org/x/tools/internal/fastwalk adds a lot of complexity to avoid
unnecessary calls to os.Stat. It also adds very limited concurrency
(a maximum of 4 goroutines).
filepath.WalkDir, added in Go 1.16, also avoids unnecessary calls to
os.Stat, and is part of the standard library (so it adds much less
complexity to x/tools). Switching to that substantially simplifies the
implementation of internal/gopathwalk, at the cost of losing the small
amount of concurrency added by fastwalk.
The internal/gopathwalk package does not appear to have any benchmarks
of its own, but the benchmark impact shows a ~14.5% increase in the
imports.ScanModCache benchmark on Linux with a warm filesystem cache,
but that can be reduced to ~5.5% by also removing an apparently
unnecessary call to os.SameFile (in followup CL 508507).
To me, the decrease in code complexity seems worth the slight increase
in latency.
~/x/tools$ benchstat cl508505.txt cl508506.txt
goos: linux
goarch: amd64
pkg: golang.org/x/tools/internal/imports
cpu: AMD EPYC 7B12
│ cl508505.txt │ cl508506.txt │
│ sec/op │ sec/op vs base │
ScanModCache-24 231.0m ± 1% 264.4m ± 1% +14.47% (p=0.000 n=10)
Fixesgolang/go#58035.
Change-Id: Iffcdab4e9aea11c0f0d7a87904935635e20b2fe7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/508506
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Robert Findley <rfindley@google.com>
ioutil defines 7 functions. 6 of these are replaced by
functions in io or os with the same signature.
ReadDir is deprecated, but the suggested replacement has a different
signature.
These changes were generated by a program, with some manual adjutments.
The program replaces ReadDir with a call to a function named ioutilReadDir
that has the same signature. The code for this function
is added to files if necessary. The program replaces all the others
with their new versions. The program removes the 'io/ioutil' import
and adds, as necessary, 'os', 'io', and 'io/fs', the latter being
needed for the signature of ioutilReadDir.
The automatic process fails in a few ways:
1. ReadFile occurs only in a comment but the program adds an unneeded import.
2. ioutilReadDir is added to more than one file in the same package
Both of these could be viewed as bugs and fixed by looking harder.
After manual adjustment, two tests failed:
1. gopls/internal/lsp/regtesg/mis:TestGenerateProgress. The reason
was a use of ioutil in a txtar constant. The calls were changed,
but the code is not smart enough to change the import inside the
string constant. (Or it's not smart enough not to change the
contents of a string.)
2. gopls/internal/lsp/analysis/deprecated, which wants to see a use
of ioutil
These tests were adjused by hand, and all tests (-short) pass.
Change-Id: If9efe40bbb0edda36173d9a88afaf71245db8e79
Reviewed-on: https://go-review.googlesource.com/c/tools/+/527675
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Peter Weinberger <pjw@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
internal/gopathwalk.walk remove an unnecessary call to os.Lstat when
checking if a symlink should be traversed. And remove a call to
filepath.EvalSymlinks that should be unnecessary as we are using
os.Stat to compare files.
Change-Id: Ie5ad1cb99cbd6d62f08f2b482a3e84b6fe41114d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/392095
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
Auto-Submit: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Trust: Ian Lance Taylor <iant@golang.org>
We failed to initialize the ProcessEnv when adding stdlib imports.
Somehow this went unnnoticed for a month or something. Initialize on
demand there too, and add a stronger check so there's a better error
message.
Fixesgolang/go#40670.
Change-Id: I391c115f417390be4a0e18f92fbfbcbfbdcc052c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247797
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In all cases, use a Logf field to configure debug logging. Non-nil means
that logging is enabled through the given function.
Fixes accidental debug spam from goimports, which had a separate Debug
flag that was supposed to guard logging, but wasn't used when creating
the gocommand.Invocation.
Change-Id: I448fa282111db556ac2e49801268d0affc19ae30
Reviewed-on: https://go-review.googlesource.com/c/tools/+/221557
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Missing function descriptions are added. init is renamed setIgnoredDirs.
Change-Id: I56b5a7c3e4fcc27991b33aef16d506948ce4b36b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/208358
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Since a user's module cache is generally going to be much bigger than
their main module, one would expect that caching just information about
the module cache would be sufficient. It turns out that's not correct.
When we discover something in the module cache, we have to make sure
that a different version of it isn't already in scope. Doing that can
require information about the main module or replace targets, so that
needs to be cached too.
Concretely, when I'm working in x/tools, if a scan discovers a version
of x/tools in the module cache, it should usually ignore that version.
But that might not be true in more complicated cases, especially those
involving nested modules whose boundaries change.
So, cache everything except GOROOT. Since the new data is mutable,
we store it separately from the module cache data so that it can be
discarded easily between runs.
Change-Id: I47364f6c0270fee03af8898fec6c85d1b9c8d780
Reviewed-on: https://go-review.googlesource.com/c/tools/+/202045
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Packages found in the module cache do not change. When we encounter a
directory we have already processed in the module cache, skip that
directory and add the packages that have already been computed.
Change-Id: Ib1bf0bf22727110b8073b415b145034acceb6787
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186921
Run-TryBot: Suzy Mueller <suzmue@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Change-Id: Ie127474e5d1e1d71177e0e38d8a4cba2ce415db3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/188497
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
go/packages needs to call `go list` multiple times, which causes
redundant work and slows down goimports. If we reimplement `go list` in
memory, we can reuse state, saving time. `go list` also does work we
don't really need, like adding stuff to go.mod, and skipping that saves
more time.
We start with `go list -m`, which does MVS and such. The remaining work
is mostly mapping import paths and directories through the in-scope
modules to make sure we're giving the right answers. Unfortunately this
is quite subtle, and I don't know where all the traps are. I did my
best.
cmd/go already has tests for `go list`, of course, and packagestest is
not well suited to tests of this complexity. So I ripped off the script
tests in cmd/go that seemed relevant and made sure that our logic
returns the right stuff in each case. I'm sure that there are more cases
to cover, but this hit all the stuff I knew about and quite a bit I
didn't.
Since we may want to use the go/packages code path in the future, e.g.
for Bazel, I left that in place. It won't be used unless the magic env
var is set.
Files in internal and imports/testdata/mod were copied verbatim from
cmd/go.
Change-Id: I1248d99c400c1a0c7ef180d4460b9b8a3db0246b
Reviewed-on: https://go-review.googlesource.com/c/158097
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
The imports package's public API is build.Default, but that doesn't mean
we need to use it in the internal implementation or the tests. Now we
have a new type, fixEnv, that contains everything relevant from
build.Context, as well as the various global vars that were only used
for testing.
Don't worry too much about the new function parameters; they mostly
move into the resolvers in the next CL.
Refactoring only; no user-visible changes intended.
Change-Id: I0d4c904955c5854dcdf904009cb3413c734baf88
Reviewed-on: https://go-review.googlesource.com/c/158437
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
go list in module mode doesn't like looking at vendor directories in
GOROOT. Skip them.
Change-Id: Iec501fbab70914ea6dd76dcbed97ecda461358d0
Reviewed-on: https://go-review.googlesource.com/c/148159
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
If users don't have a module cache yet, or put a nonexistant directory
in their GOPATH, it doesn't make sense to print an error. Just ignore it
and move on.
No tests; I don't think it makes sense to set up log scraping for this.
Change-Id: I90719297ade37999e8b401767a0a37c940828c27
Reviewed-on: https://go-review.googlesource.com/c/142977
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Add an implementation of name= for go list. It will be used to
implement goimports and godoc-like lookups by package name.
Imported a copy of the semver package from the stdlib to do version
comparison, and tweaked the gopathwalk API to include a hint about what
kind of source directory is being traversed.
Note that the tests, despite my best efforts, are not hermetic: go list
insists on doing version lookups in situations where it seems to me like
it shouldn't need to.
I think this implementation is ready for serious use. The one thing I'm
nervous about is that it currently does a substring match when looking
for a package name, so if you look up a package named "a" you will get
a huge number of results. This matches goimports' behavior but I don't
know if it's suitable for general use.
Change-Id: I2b7f823b74571fe30d3bd9c7dfafb4e6a40df5d3
Reviewed-on: https://go-review.googlesource.com/c/138878
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>
Extract goimports' logic for walking Go source directories into a
separate package, suitable for use in go/packages. No functional
changes. Added a convenience feature to fastwalk, allowing the user to
say that they're done with a directory and stop receiving callbacks for
it.
Testing is a little light; I expect goimports' tests to cover most
everything we care about.
Change-Id: If047ada4414f5f282637d11fd07e8342fadc9c33
Reviewed-on: https://go-review.googlesource.com/c/138877
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Bryan C. Mills
Peter Weinbergr
Charlie Vieth
Heschi Kreinick
Rebecca Stambler
Arda Güçlü
Suzy Mueller
Anthony Fok