We were previously behaving as though the slice/map values in the
options struct could be modified directly. The options should be cloned
before modification. Also, convert any usage of source.Options to
*source.Options.
Fixesgolang/go#39592
Change-Id: Ib39f668bca0fa1038162206bd7793fd2049af576
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254558
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
A copy and paste error meant that gopls iterated over the built-in
release tags rather than the ones read from go list. If gopls was built
with an older release than the go command in use, it would read past the
end of the slice and die.
Change-Id: Ifc222c7a0bd9662bcd7deedaa069797e213df7b3
Reviewed-on: https://go-review.googlesource.com/c/tools/+/255360
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change enables the multi-module workspace mode by default, so that
we can catch all of the test failures and edge cases. It is still
disabled in GOPATH mode and for any workspaces that contain a module
with a vendor directory.
A few minor changes had to be made to handle changes caused by the
workspace module pseudoversions.
Updates golang/go#32394
Change-Id: Ib433b269dfc435d73365677945057c1c2cbb1869
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254317
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
When we're in workspace module mode, there may not be a go.mod at the
root of the workspace, so checking modURI != "" doesn't tell us whether
module mode is on. Add a flag to workspaceMode which is exactly that.
There are probably many more places that should be updated, but since
the design is in flux I don't want to cause more churn than necessary.
This is enough to unblock -mod=readonly by default.
(I missed this in CL 253799 because I was running the wrong Go Version.)
Change-Id: I718ee44c7a2547c90f9d62393aba390c9b4b0f46
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254756
Trust: Heschi Kreinick <heschi@google.com>
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
As we have modified the ways that we control which analyzers get
executed for a given case, we have lost the behavior of enabling and
disabling staticcheck smoothly. This CL splits out the staticcheck
analyzers from the main group so that the "staticcheck" setting can
override whether or not a given staticcheck analysis is enabled.
Fixesgolang/go#41311
Change-Id: I9c1695afe4a8f89cd0ee50a79e83b2f42a2c20cb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/254038
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Go 1.16 may set -mod=readonly by default. To maintain current behavior,
gopls needs to override that by passing -mod=mod to all its go
invocations.
While this behavior should be safe on all modern versions of Go, I gated
it on 1.16 just for safety's sake.
Change-Id: Ic8088213d1ab9ab3a3ed0b51f47b2c222974d613
Reviewed-on: https://go-review.googlesource.com/c/tools/+/253799
Run-TryBot: Heschi Kreinick <heschi@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This change adds an experimental configuration, which when enabled,
shifts gopls to operate in multi-module mode. It implements the
super-module as described in
https://github.com/golang/proposal/blob/master/design/37720-gopls-workspaces.md.
Replace directives are also added when a workspace module requires
another workspace module (which has not yet been mentioned in the design
doc).
A user-provided workspace gopls.mod file is not yet supported, as it is
not yet testable. Clients will need to add support for change
notifications for the gopls.mod once it is added.
Updates golang/go#32394
Change-Id: I5089358603bca34c5c8db9e5a00f93e1cca0b93f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247819
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopls has many settings. We want to automatically generate
documentation, and we want to allow clients to perform their own
validation if they so desire.
Using all three of AST, type information, and reflection, generate a
JSON description of the settings and their default values. Add a gopls
command that prints it. Add a documentation generator that uses it to
write settings.md.
I assumed that everything not explicitly documented was experimental,
and moved it into that section. I also moved expandWorkspaceToModule to
experimental; I hope it's not long for this world, personally.
Along the way, rename many fields, make the enum matching case
insensitive, and add a stringer call so that the defaults print nicely.
Fixesgolang/go#33544.
Change-Id: Ibb652002933e355ed3c6038d6ca48345b39b3025
Reviewed-on: https://go-review.googlesource.com/c/tools/+/252322
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
I think we had discussed making this change after you finished your
cache rewrites, but I never got back to it. Let me know if you think
there's a better way to do this.
Change-Id: Ia7dfe6830110f21e0d2683e64f8ee0d2549afdea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/253280
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
The type checker sometimes emits secondary diagnostics. For instance,
if a function is defined twice, then when it sees the second definition
it emits a diagnostic at the second definition and a secondary diagnostic
pointing to the first diagnostic. Presently gopls treats these as two
separate diagnostics. The changed code still produces two diagnostics,
but now the secondary diagnostic is also converted into a
RelatedInformation so the user sees a xpointer to the earlier definition.
Updates https://github.com/golang/go/issues/39062.
Change-Id: Ic421ec91d2b46c28681ab3ec010d5b02c0442e68
Reviewed-on: https://go-review.googlesource.com/c/tools/+/251617
Run-TryBot: Peter Weinberger <pjw@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This is better than parsing the default output.
Also, change the start progress message, since it ends up duplicating
the title in the message that the user sees.
Change-Id: I3540d30c7976c6be0722531b2e258341081e0b72
Reviewed-on: https://go-review.googlesource.com/c/tools/+/251920
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Our metadata reloading model makes typing out import paths manually
very slow. We can avoid some of the slowness by not invalidating
metadata when a new import path is obviously invalid.
Updates golang/go#35877
Change-Id: Ifcf9ebaac0b146a2098ef8d411fa85fefa7ba6ca
Reviewed-on: https://go-review.googlesource.com/c/tools/+/251086
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Danish Dua <danishdua@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
This change adds a test for this case (empty test file in GOPATH mode)
to go/packages. It is skipped for now, as the go/packages work-around is
too brittle to justify adding. Instead, we suppress gopls-generated
error messages in pop-ups--users should only see error messages if they
come from go/packages.
Fixesgolang/go#40825
Change-Id: I0e2af53578d65739ebae582075498d997fc019d7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/250949
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Seems we've drifted a bit from go1.12 support, mostly due to error
wrapping.
Fix this, as well as some assorted other failures.
I haven't tested 1.12 interactively.
For golang/go#39146
Change-Id: Id347ead2a13e89b76d2ae0047750e6b6b49911eb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/250941
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
CL 248380 forced all type checking to be in the default workspace mode.
In that CL, I said I couldn't think of any features that would break. It
appears I didn't think very hard. Navigation features inside of
dependencies are something I use all the time and they broke.
Reintroduce the ability to get packages in a particular mode, and make
it convenient to get them in all relevant modes. Update some critical
features to do so, and add regression tests.
Fixesgolang/go#40809.
Change-Id: I96279f4ff994203694629ea872795246c410b206
Reviewed-on: https://go-review.googlesource.com/c/tools/+/249120
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
CL 244117 introduced a bug when modFile == os.DevNull: v.root is left
uninitialized, resulting in a view that appears to own all files. Fixing
that exposes a problem where opening a folder with no Go files and
GO111MODULE=on shows a popup. Skip the popup when no Go files are found.
Change-Id: I7f8b2d6fd2f954af64c3a65156ff44c649f3a5b2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248620
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Every package has a default type checking mode dictated by whether it's
in the workspace or not. Some features force full rather than exported
type checking, but AFAICT that ends up being more harm than good. For
example, let's say we want to Find References on fmt.Printf in the stdlib.
Before this CL, we'd force a new type check of the fmt package, then
find no references because nothing else would have been checked against
that new version.
While there may be some features that work better in the current regime,
I can't think of any, and we have no test coverage for them. So I'd
rather start with what makes sense, and if we want to change it maybe
let's write some tests.
Change-Id: Iea589efb4b4374fd2a54451c868b6e2bd5484e20
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248380
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
While looking at Kubernetes I noticed that golang.org/x/text packages
were some of the largest. The problem is the large code-generated
tables, which use ellipsis array literals. Teach gopls to trim the cases
that matter there.
While silly, this trims ~60MB off the live heap, so I think it might be
worth it.
Change-Id: I0cfd80bd5fbc8703ac628312982af9c6ed871758
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248180
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
In the past, we assumed that we would only run these functions on the
view's go.mod file. As we expand the concept of a view to possibly
include multiple go.mod files, we need to allow these functions to work
on multiple go.mod files.
Change-Id: If9e7d131007e0977fc48ee2264365773da7d41f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/248097
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Some users may intentionally be opening subdirectories to avoid having
gopls load the whole module. Allow this via a configuration.
Fixesgolang/go#40567
Change-Id: I6167f62a74a1c0b7cf07c1cb247adda839ee41f2
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247617
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
A user need not necessarily set GOPACKAGESDRIVER in their environment,
but they may still provide it through their configuration.
Change-Id: Ic48328e6a1596ff653a048b24256b8dc44c45b8e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247817
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The GC-based cache has given us a number of problems. First, memory
leaks driven by reference cycles: the Go runtime cannot collect cycles
involving finalizers, which prevents us from writing natural code in
Bind callbacks. If we screw it up, we get a mysterious leak that takes a
long time to track down. Second, the behavior is generally mysterious;
it's hard to predict how long a value lasts, and harder to tell if a
value being live is a bug. Third, we think that it may be interacting
poorly with the GC, resulting in unnecessary memory usage.
The structure of the values we put in the cache is not actually that
complicated -- there are only 5 significant types: parse, typecheck,
analyze, parse mod, and analyze mod. Managing them manually should not
be conceptually difficult, and in fact we already do most of the work
in (*snapshot).clone.
In this CL the cache adds the concept of "generations", which function
as reference counts on cache entries. Entries are still global and
shared across generations, but will be explicitly deleted once no
generations refer to them. The idea is that each snapshot is a new
generation, and can inherit entries from the previous snapshot or leave
them behind to be deleted.
One obvious risk of this scheme is that we'll leave dangling references
to values without actually inheriting them across generations. To
prevent that, getting a value requires passing in the generation at
which it's being read, and an error will be returned if that generation
is dead.
Change-Id: I4b30891efd7be4e10f2b84f4c067b0dee43dcf9c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/242838
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
This change adds the notion of a "workspace directory", which is
basically the set of directories that contains workspace packages. These
are mainly used for replace targets right now. It's a little trickier
than expected because the set of workspace directories can technically
change on any go.mod change.
At first, I wanted DidModifyFiles to report whether there was a change,
but I don't think it's actually that expensive to check on each call
and it complicates the code a bit. I can change it back if you think
it's worth doing.
The parse mod handle changes are because I needed an unlocked way of
parsing the mod file, but I imagine they'll conflict with CL 244769
anyway.
The next CL will be to "promote" replace targets to the level of
workspace packages, meaning we will be able to find references in them.
Change-Id: I5dd58fe29415473496ca6634a94a3134923228dc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245327
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
This CL is mostly a refactoring of the logic to compute errors for
`go mod tidy` diagnostics. It had been getting a little confusing, so
hopefully this makes things easier to read.
I made a few other small changes, such as slightly changing a few error
messages and showing diagnostics in the go.mod file for all missing
modules.
Change-Id: Ia8cf580731b997248591a2d64dff133accd9c5aa
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244610
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
We missed a possible case in checking for gopackagesdriver - a binary
named gopackagesdriver works the same way as setting GOPACKAGESDRIVER.
Change-Id: I676800d253950cb35d74211558bafab340310653
Reviewed-on: https://go-review.googlesource.com/c/tools/+/247179
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Michael Matloob <matloob@golang.org>
snapshot.View().Session().Cache().FileSet() has been driving me crazy
for a while. Add it to snapshot. Along the way, discover that the Cache
interface is now totally unused and delete it.
I also changed a bunch of View arguments to Snapshot while I was in the
area.
Change-Id: I1064d0020b1567c2ed28d2d55e0f4649eb94c060
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245324
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
parseGoHandles have lifetimes separate from the packages they belong to.
For example, a package may be invalidated by a change to one of its
files, but we still want to retain the parse results for all the rest.
Track them explicitly.
Change-Id: I03a4ffe283bf2b252d2d838bdb2cf332cd981075
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245059
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
FileHandle currently includes LSP-level information about Version and
Session. That's dangerous, because the cache operates in terms of
URIs and content only -- we explicitly want to share results across
sessions and versions if they happen to be the same.
Split the LSP information into separate types, VersionedFileHandle and
VersionedFileIdentity.
Change-Id: I158646b783375b58245468599301e2a29c657e71
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245058
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The builtin package was the one special case where we parsed Go outside
the context of a Snapshot. Move it up.
Change-Id: I1f4bb536adb40019e0ea9c5c89f38b15737abb8c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245057
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
To manually collect cache entries, we need to know when a snapshot is
idle. Add a reference count in the form of a WaitGroup and keep track of
its uses. The pattern is that any time a snapshot is returned, it comes
with a release function that decrements the ref count.
Almost all uses of a snapshot originate in a user-facing request,
handled in beginFileRequest. There it's mostly an exercise in passing
Snapshots around instead of Views.
In the other places I took the path of least resistance. For file
modifications I tried to minimize the amount of code that needed to deal
with snapshots. For diagnostics I just acquired the snapshot at the
diagnostics call.
Change-Id: Id48a2df3acdd97f27d905e2c2be23072f28f196b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/241837
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Previously, we were only invalidating workspace packages when the go.mod
changed, but we really need to be invalidating all known packages.
Fixesgolang/go#40456
Change-Id: I9ad353a26ab40c74c7760ed7a1c5de517640cfab
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245779
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Continuing the massacre, remove ParseModHandle, and Mod*Handle, from the
source API.
Notably, having the snapshot available means we can simplify the go
command invocation paths a lot.
Change-Id: Ief4ef41e42f93d653f719a230004861e5e1ef70b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244769
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
There were a few merge conflict-related issues in the GC optimization
details CL. Also fixed a few things I noticed after the fact, like
separating out a new mutex.
Staticcheck caught a few things, and I also fixed a bug I noticed
in the cache package.
Change-Id: I3fc519373253418586dca08fdec3114b30a247ea
Reviewed-on: https://go-review.googlesource.com/c/tools/+/245399
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Peter Weinberger <pjw@google.com>
Just like ParseGoHandle, PackageHandle isn't very useful as part of the
public API. Remove it.
Having PackagesForFile take a URI rather than a FileHandle seems
reasonable, and made me wonder if that logic applies to other calls like
ParseGo. For now I'm going to stop here. I could also revert that part
of the change.
Change-Id: Idba8e9fdba0b0c48e841a698eb97e47fd5f23cf5
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244637
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
ParseGoHandles serve two purposes: they pin cache entries so that
redundant calculations are cached, and they allow users to obtain the
actual parsed AST. The former is an implementation detail, and the
latter turns out to just be an annoyance.
Parsed Go files are obtained from two places. By far the most common is
from a type checked package. But a type checked package must by
definition have already parsed all the files it contains, so the PGH
is already computed and cannot have failed. Type checked packages can
simply return the parsed file without requiring a separate Check
operation. We do want to pin the cache entries in this case, which I've
done by holding on to the PGH in cache.pkg.
There are some cases where we directly parse a file, such as for the
FoldingRange LSP call, which doesn't need type information. Those parses
can actually fail, so we do need an error check. But we don't need the
PGH; in all cases we are immediately using and discarding it.
So it turns out we don't actually need the PGH type at all, at least not
in the public API. Instead, we can pass around a concrete struct that
has the various pieces of data directly available.
This uncovered a bug in typeCheck: it should fail if it encounters any
real errors.
Change-Id: I203bf2dd79d5d65c01392d69c2cf4f7744fde7fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244021
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The FileIdentity struct mixes information about the file itself
(filename, hash) with information about the LSP references to that file
(session ID, version). When we create a cache key using it, we only want
the former, as returned by the String method. Otherwise we split the
cache whenever those irrelevant fields are different.
We also use FileIdentity as an element of diagnosticsKey, but I believe
that use is appropriate.
Change-Id: I094e00d2700e05778da635effbb69d0ebcb6726e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244020
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Due to the runtime's inability to collect cycles involving finalizers,
we can't close over handles in memoize.Functions without causing memory
leaks. Up until now we've dealt with that by closing over all the bits
of the snapshot that we want, but it distorts the design of all the code
used in the Functions.
We can solve the problem another way: instead of closing over the
snapshot/view, we can force the caller to pass it in. This is somewhat
scary: there is no requirement that the argument matches the data that
we're working with. But the reality is that this is not a new problem:
the Function used to calculate a cache value is not necessarily the one
that the caller expects. As long as the cache key fully identifies all
the inputs to the Function, the output should be correct. And since the
caller used the snapshot/view to calculate that cache key, it should
always be safe to pass in that snapshot/view. If it's not, then we
already had a bug.
The Arg type in memoize is clumsy, but I thought it would be nice to
have at least a little bit of type safety. I'm open to suggestions.
Change-Id: I23f546638b0c66a4698620a986949087211f4762
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244019
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
The memoize cache buys us little for files: the cache value is not
really a function of the inputs, but rather the filesystem state. It's
pretty much just as easy to manage them explicitly, and it's a start at
simplifying our caching strategy.
We do lose one small feature: if we try to read the same file
concurrently, reads will not be deduplicated. I suspect that doesn't
matter.
Change-Id: I75e219467fb7a512d9cfdf87443d012c85f03df9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/243197
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Robert Findley <rfindley@google.com>
The PackageHandle interface is fairly redundant with the Package
interface. As much as convenient, move users to Package and
weaken/remove methods from PackageHandle.
I would like to get rid of CompiledGoFiles too but
NarrowestPackageHandle is a little annoying. I think this is
unambiguously a step forward so I figured we can get it in and keep
iterating.
Change-Id: I6c5a3f462b1f19cbca6a267fedc36ce54613b6fc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/244018
Reviewed-by: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
