From 5a4eba5af06f7508c8a90c5568193de040e17de1 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Thu, 3 Nov 2022 18:28:39 -0400 Subject: [PATCH] internal/lsp/cache: remove support for invalid metadata Remove support for keeping track of invalid metadata, as discussed in golang/go#55333. Fixes golang/go#55333 Change-Id: I7727f43e2cffef610096d20af4898f326c5a8450 Reviewed-on: https://go-review.googlesource.com/c/tools/+/447741 TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan gopls-CI: kokoro Run-TryBot: Robert Findley --- gopls/doc/settings.md | 13 -- gopls/internal/lsp/cache/check.go | 20 +-- gopls/internal/lsp/cache/debug.go | 4 +- gopls/internal/lsp/cache/graph.go | 38 +---- gopls/internal/lsp/cache/load.go | 46 +++--- gopls/internal/lsp/cache/metadata.go | 9 -- gopls/internal/lsp/cache/mod_tidy.go | 2 +- gopls/internal/lsp/cache/snapshot.go | 95 +++-------- gopls/internal/lsp/source/api_json.go | 8 - gopls/internal/lsp/source/options.go | 13 +- .../regtest/diagnostics/diagnostics_test.go | 153 ------------------ 11 files changed, 55 insertions(+), 346 deletions(-) diff --git a/gopls/doc/settings.md b/gopls/doc/settings.md index 5595976363..4c3c7a8182 100644 --- a/gopls/doc/settings.md +++ b/gopls/doc/settings.md @@ -161,19 +161,6 @@ be removed. Default: `false`. -#### **experimentalUseInvalidMetadata** *bool* - -**This setting is experimental and may be deleted.** - -experimentalUseInvalidMetadata enables gopls to fall back on outdated -package metadata to provide editor features if the go command fails to -load packages for some reason (like an invalid go.mod file). - -Deprecated: this setting is deprecated and will be removed in a future -version of gopls (https://go.dev/issue/55333). - -Default: `false`. - #### **standaloneTags** *[]string* standaloneTags specifies a set of build constraints that identify diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 25da922db1..1f3622a73d 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -46,7 +46,7 @@ type packageHandle struct { promise *memoize.Promise // [typeCheckResult] // m is the metadata associated with the package. - m *KnownMetadata + m *Metadata // key is the hashed key for the package. // @@ -105,12 +105,8 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so // Don't use invalid metadata for dependencies if the top-level // metadata is valid. We only load top-level packages, so if the // top-level is valid, all of its dependencies should be as well. - if err != nil || m.Valid && !depHandle.m.Valid { - if err != nil { - event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, source.SnapshotLabels(s)...) - } else { - event.Log(ctx, fmt.Sprintf("%s: invalid dep handle for %s", id, depID), source.SnapshotLabels(s)...) - } + if err != nil { + event.Error(ctx, fmt.Sprintf("%s: no dep handle for %s", id, depID), err, source.SnapshotLabels(s)...) // This check ensures we break out of the slow // buildPackageHandle recursion quickly when @@ -136,7 +132,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so // syntax trees are available from (*pkg).File(URI). // TODO(adonovan): consider parsing them on demand? // The need should be rare. - goFiles, compiledGoFiles, err := readGoFiles(ctx, s, m.Metadata) + goFiles, compiledGoFiles, err := readGoFiles(ctx, s, m) if err != nil { return nil, err } @@ -146,7 +142,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so experimentalKey := s.View().Options().ExperimentalPackageCacheKey phKey := computePackageKey(m.ID, compiledGoFiles, m, depKey, mode, experimentalKey) promise, release := s.store.Promise(phKey, func(ctx context.Context, arg interface{}) interface{} { - pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m.Metadata, mode, deps) + pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m, mode, deps) return typeCheckResult{pkg, err} }) @@ -166,7 +162,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so // packageHandle to the handles for parsing its files and the // handles for type-checking its immediate deps, at which // point there will be no need to even access s.meta.) - if s.meta.metadata[ph.m.ID].Metadata != ph.m.Metadata { + if s.meta.metadata[ph.m.ID] != ph.m { return nil, fmt.Errorf("stale metadata for %s", ph.m.ID) } @@ -174,7 +170,7 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so if prev, ok := s.packages.Get(packageKey); ok { prevPH := prev.(*packageHandle) release() - if prevPH.m.Metadata != ph.m.Metadata { + if prevPH.m != ph.m { return nil, bug.Errorf("existing package handle does not match for %s", ph.m.ID) } return prevPH, nil @@ -225,7 +221,7 @@ func (s *snapshot) workspaceParseMode(id PackageID) source.ParseMode { // computePackageKey returns a key representing the act of type checking // a package named id containing the specified files, metadata, and // combined dependency hash. -func computePackageKey(id PackageID, files []source.FileHandle, m *KnownMetadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey { +func computePackageKey(id PackageID, files []source.FileHandle, m *Metadata, depsKey source.Hash, mode source.ParseMode, experimentalKey bool) packageHandleKey { // TODO(adonovan): opt: no need to materalize the bytes; hash them directly. // Also, use field separators to avoid spurious collisions. b := bytes.NewBuffer(nil) diff --git a/gopls/internal/lsp/cache/debug.go b/gopls/internal/lsp/cache/debug.go index d665b011da..fd82aff301 100644 --- a/gopls/internal/lsp/cache/debug.go +++ b/gopls/internal/lsp/cache/debug.go @@ -47,7 +47,7 @@ func (s *snapshot) dumpWorkspace(context string) { for _, id := range ids { pkgPath := s.workspacePackages[id] - m, ok := s.meta.metadata[id] - debugf(" %s:%s (metadata: %t; valid: %t)", id, pkgPath, ok, m.Valid) + _, ok := s.meta.metadata[id] + debugf(" %s:%s (metadata: %t)", id, pkgPath, ok) } } diff --git a/gopls/internal/lsp/cache/graph.go b/gopls/internal/lsp/cache/graph.go index 83336a23e1..be5a0618ca 100644 --- a/gopls/internal/lsp/cache/graph.go +++ b/gopls/internal/lsp/cache/graph.go @@ -15,7 +15,7 @@ import ( // graph of Go packages, as obtained from go/packages. type metadataGraph struct { // metadata maps package IDs to their associated metadata. - metadata map[PackageID]*KnownMetadata + metadata map[PackageID]*Metadata // importedBy maps package IDs to the list of packages that import them. importedBy map[PackageID][]PackageID @@ -27,12 +27,12 @@ type metadataGraph struct { // Clone creates a new metadataGraph, applying the given updates to the // receiver. -func (g *metadataGraph) Clone(updates map[PackageID]*KnownMetadata) *metadataGraph { +func (g *metadataGraph) Clone(updates map[PackageID]*Metadata) *metadataGraph { if len(updates) == 0 { // Optimization: since the graph is immutable, we can return the receiver. return g } - result := &metadataGraph{metadata: make(map[PackageID]*KnownMetadata, len(g.metadata))} + result := &metadataGraph{metadata: make(map[PackageID]*Metadata, len(g.metadata))} // Copy metadata. for id, m := range g.metadata { result.metadata[id] = m @@ -74,51 +74,25 @@ func (g *metadataGraph) build() { } // Sort and filter file associations. - // - // We choose the first non-empty set of package associations out of the - // following. For simplicity, call a non-command-line-arguments package a - // "real" package. - // - // 1: valid real packages - // 2: a valid command-line-arguments package - // 3: invalid real packages - // 4: an invalid command-line-arguments package for uri, ids := range g.ids { sort.Slice(ids, func(i, j int) bool { - // 1. valid packages appear earlier. - validi := g.metadata[ids[i]].Valid - validj := g.metadata[ids[j]].Valid - if validi != validj { - return validi - } - - // 2. command-line-args packages appear later. cli := source.IsCommandLineArguments(ids[i]) clj := source.IsCommandLineArguments(ids[j]) if cli != clj { return clj } - // 3. packages appear in name order. + // 2. packages appear in name order. return ids[i] < ids[j] }) // Choose the best IDs for each URI, according to the following rules: // - If there are any valid real packages, choose them. // - Else, choose the first valid command-line-argument package, if it exists. - // - Else, keep using all the invalid metadata. // // TODO(rfindley): it might be better to track all IDs here, and exclude // them later in PackagesForFile, but this is the existing behavior. - hasValidMetadata := false for i, id := range ids { - m := g.metadata[id] - if m.Valid { - hasValidMetadata = true - } else if hasValidMetadata { - g.ids[uri] = ids[:i] - break - } // If we've seen *anything* prior to command-line arguments package, take // it. Note that ids[0] may itself be command-line-arguments. if i > 0 && source.IsCommandLineArguments(id) { @@ -134,7 +108,7 @@ func (g *metadataGraph) build() { // // If includeInvalid is false, the algorithm ignores packages with invalid // metadata (including those in the given list of ids). -func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...PackageID) map[PackageID]bool { +func (g *metadataGraph) reverseTransitiveClosure(ids ...PackageID) map[PackageID]bool { seen := make(map[PackageID]bool) var visitAll func([]PackageID) visitAll = func(ids []PackageID) { @@ -144,7 +118,7 @@ func (g *metadataGraph) reverseTransitiveClosure(includeInvalid bool, ids ...Pac } m := g.metadata[id] // Only use invalid metadata if we support it. - if m == nil || !(m.Valid || includeInvalid) { + if m == nil { continue } seen[id] = true diff --git a/gopls/internal/lsp/cache/load.go b/gopls/internal/lsp/cache/load.go index dcd343a56b..0498264324 100644 --- a/gopls/internal/lsp/cache/load.go +++ b/gopls/internal/lsp/cache/load.go @@ -157,7 +157,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc moduleErrs := make(map[string][]packages.Error) // module path -> errors filterer := buildFilterer(s.view.rootURI.Filename(), s.view.gomodcache, s.view.Options()) - newMetadata := make(map[PackageID]*KnownMetadata) + newMetadata := make(map[PackageID]*Metadata) for _, pkg := range pkgs { // The Go command returns synthetic list results for module queries that // encountered module errors. @@ -222,10 +222,10 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc // // TODO(rfindley): perform a sanity check that metadata matches here. If not, // we have an invalidation bug elsewhere. - updates := make(map[PackageID]*KnownMetadata) + updates := make(map[PackageID]*Metadata) var updatedIDs []PackageID for _, m := range newMetadata { - if existing := s.meta.metadata[m.ID]; existing == nil || !existing.Valid { + if existing := s.meta.metadata[m.ID]; existing == nil { updates[m.ID] = m updatedIDs = append(updatedIDs, m.ID) delete(s.shouldLoad, m.ID) @@ -238,7 +238,7 @@ func (s *snapshot) load(ctx context.Context, allowNetwork bool, scopes ...loadSc // // Note that the original metadata is being invalidated here, so we use the // original metadata graph to compute the reverse closure. - invalidatedPackages := s.meta.reverseTransitiveClosure(true, updatedIDs...) + invalidatedPackages := s.meta.reverseTransitiveClosure(updatedIDs...) s.meta = s.meta.Clone(updates) s.resetIsActivePackageLocked() @@ -475,7 +475,7 @@ func makeWorkspaceDir(ctx context.Context, workspace *workspace, fs source.FileS // buildMetadata populates the updates map with metadata updates to // apply, based on the given pkg. It recurs through pkg.Imports to ensure that // metadata exists for all dependencies. -func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*KnownMetadata, path []PackageID) error { +func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Config, query []string, updates map[PackageID]*Metadata, path []PackageID) error { // Allow for multiple ad-hoc packages in the workspace (see #47584). pkgPath := PackagePath(pkg.PkgPath) id := PackageID(pkg.ID) @@ -507,18 +507,15 @@ func buildMetadata(ctx context.Context, pkg *packages.Package, cfg *packages.Con } // Recreate the metadata rather than reusing it to avoid locking. - m := &KnownMetadata{ - Metadata: &Metadata{ - ID: id, - PkgPath: pkgPath, - Name: PackageName(pkg.Name), - ForTest: PackagePath(packagesinternal.GetForTest(pkg)), - TypesSizes: pkg.TypesSizes, - Config: cfg, - Module: pkg.Module, - depsErrors: packagesinternal.GetDepsErrors(pkg), - }, - Valid: true, + m := &Metadata{ + ID: id, + PkgPath: pkgPath, + Name: PackageName(pkg.Name), + ForTest: PackagePath(packagesinternal.GetForTest(pkg)), + TypesSizes: pkg.TypesSizes, + Config: cfg, + Module: pkg.Module, + depsErrors: packagesinternal.GetDepsErrors(pkg), } updates[id] = m @@ -650,7 +647,7 @@ func containsPackageLocked(s *snapshot, m *Metadata) bool { // the snapshot s. // // s.mu must be held while calling this function. -func containsOpenFileLocked(s *snapshot, m *KnownMetadata) bool { +func containsOpenFileLocked(s *snapshot, m *Metadata) bool { uris := map[span.URI]struct{}{} for _, uri := range m.CompiledGoFiles { uris[uri] = struct{}{} @@ -700,14 +697,7 @@ func containsFileInWorkspaceLocked(s *snapshot, m *Metadata) bool { func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[PackageID]PackagePath { workspacePackages := make(map[PackageID]PackagePath) for _, m := range meta.metadata { - // Don't consider invalid packages to be workspace packages. Doing so can - // result in type-checking and diagnosing packages that no longer exist, - // which can lead to memory leaks and confusing errors. - if !m.Valid { - continue - } - - if !containsPackageLocked(s, m.Metadata) { + if !containsPackageLocked(s, m) { continue } @@ -748,12 +738,12 @@ func computeWorkspacePackagesLocked(s *snapshot, meta *metadataGraph) map[Packag // function returns false. // // If m is not a command-line-arguments package, this is trivially true. -func allFilesHaveRealPackages(g *metadataGraph, m *KnownMetadata) bool { +func allFilesHaveRealPackages(g *metadataGraph, m *Metadata) bool { n := len(m.CompiledGoFiles) checkURIs: for _, uri := range append(m.CompiledGoFiles[0:n:n], m.GoFiles...) { for _, id := range g.ids[uri] { - if !source.IsCommandLineArguments(id) && (g.metadata[id].Valid || !m.Valid) { + if !source.IsCommandLineArguments(id) { continue checkURIs } } diff --git a/gopls/internal/lsp/cache/metadata.go b/gopls/internal/lsp/cache/metadata.go index 03037f0b09..135623fc53 100644 --- a/gopls/internal/lsp/cache/metadata.go +++ b/gopls/internal/lsp/cache/metadata.go @@ -86,12 +86,3 @@ func (m *Metadata) IsIntermediateTestVariant() bool { func (m *Metadata) ModuleInfo() *packages.Module { return m.Module } - -// KnownMetadata is a wrapper around metadata that tracks its validity. -type KnownMetadata struct { - *Metadata - - // Valid is true if the given metadata is Valid. - // Invalid metadata can still be used if a metadata reload fails. - Valid bool -} diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go index cc604c1c86..fc97ee6b25 100644 --- a/gopls/internal/lsp/cache/mod_tidy.go +++ b/gopls/internal/lsp/cache/mod_tidy.go @@ -196,7 +196,7 @@ func modTidyDiagnostics(ctx context.Context, snapshot *snapshot, pm *source.Pars } // Read both lists of files of this package, in parallel. - goFiles, compiledGoFiles, err := readGoFiles(ctx, snapshot, m.Metadata) + goFiles, compiledGoFiles, err := readGoFiles(ctx, snapshot, m) if err != nil { return nil, err } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index 70c26f253c..656d08fb11 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -729,8 +729,6 @@ func (s *snapshot) packageHandlesForFile(ctx context.Context, uri span.URI, mode // If experimentalUseInvalidMetadata is set, this function may return package // IDs with invalid metadata. func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]PackageID, error) { - useInvalidMetadata := s.useInvalidMetadata() - s.mu.Lock() // Start with the set of package associations derived from the last load. @@ -741,7 +739,7 @@ func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]Pack for _, id := range ids { // TODO(rfindley): remove the defensiveness here. s.meta.metadata[id] must // exist. - if m, ok := s.meta.metadata[id]; ok && m.Valid { + if _, ok := s.meta.metadata[id]; ok { hasValidID = true } if len(s.shouldLoad[id]) > 0 { @@ -757,16 +755,6 @@ func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]Pack s.mu.Unlock() - // Special case: if experimentalUseInvalidMetadata is set and we have any - // ids, just return them. - // - // This is arguably wrong: if the metadata is invalid we should try reloading - // it. However, this was the pre-existing behavior, and - // experimentalUseInvalidMetadata will be removed in a future release. - if !shouldLoad && useInvalidMetadata && len(ids) > 0 { - return ids, nil - } - // Reload if loading is likely to improve the package associations for uri: // - uri is not contained in any valid packages // - ...or one of the packages containing uri is marked 'shouldLoad' @@ -800,28 +788,19 @@ func (s *snapshot) getOrLoadIDsForURI(ctx context.Context, uri span.URI) ([]Pack s.mu.Lock() ids = s.meta.ids[uri] - if !useInvalidMetadata { - var validIDs []PackageID - for _, id := range ids { - // TODO(rfindley): remove the defensiveness here as well. - if m, ok := s.meta.metadata[id]; ok && m.Valid { - validIDs = append(validIDs, id) - } + var validIDs []PackageID + for _, id := range ids { + // TODO(rfindley): remove the defensiveness here as well. + if _, ok := s.meta.metadata[id]; ok { + validIDs = append(validIDs, id) } - ids = validIDs } + ids = validIDs s.mu.Unlock() return ids, nil } -// Only use invalid metadata for Go versions >= 1.13. Go 1.12 and below has -// issues with overlays that will cause confusing error messages if we reuse -// old metadata. -func (s *snapshot) useInvalidMetadata() bool { - return s.view.goversion >= 13 && s.view.Options().ExperimentalUseInvalidMetadata -} - func (s *snapshot) GetReverseDependencies(ctx context.Context, id PackageID) ([]source.Package, error) { if err := s.awaitLoaded(ctx); err != nil { return nil, err @@ -829,7 +808,7 @@ func (s *snapshot) GetReverseDependencies(ctx context.Context, id PackageID) ([] s.mu.Lock() meta := s.meta s.mu.Unlock() - ids := meta.reverseTransitiveClosure(s.useInvalidMetadata(), id) + ids := meta.reverseTransitiveClosure(id) // Make sure to delete the original package ID from the map. delete(ids, id) @@ -1216,9 +1195,7 @@ func (s *snapshot) AllValidMetadata(ctx context.Context) ([]source.Metadata, err var meta []source.Metadata for _, m := range s.meta.metadata { - if m.Valid { - meta = append(meta, m) - } + meta = append(meta, m) } return meta, nil } @@ -1273,7 +1250,7 @@ func moduleForURI(modFiles map[span.URI]struct{}, uri span.URI) span.URI { return match } -func (s *snapshot) getMetadata(id PackageID) *KnownMetadata { +func (s *snapshot) getMetadata(id PackageID) *Metadata { s.mu.Lock() defer s.mu.Unlock() @@ -1319,7 +1296,7 @@ func (s *snapshot) noValidMetadataForURILocked(uri span.URI) bool { return true } for _, id := range ids { - if m, ok := s.meta.metadata[id]; ok && m.Valid { + if _, ok := s.meta.metadata[id]; ok { return false } } @@ -1409,19 +1386,8 @@ func isFileOpen(fh source.VersionedFileHandle) bool { func (s *snapshot) awaitLoaded(ctx context.Context) error { loadErr := s.awaitLoadedAllErrors(ctx) - s.mu.Lock() - defer s.mu.Unlock() - - // If we still have absolutely no metadata, check if the view failed to - // initialize and return any errors. - if s.useInvalidMetadata() && len(s.meta.metadata) > 0 { - return nil - } - for _, m := range s.meta.metadata { - if m.Valid { - return nil - } - } + // TODO(rfindley): eliminate this function as part of simplifying + // CriticalErrors. if loadErr != nil { return loadErr.MainError } @@ -1968,27 +1934,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC result.shouldLoad[k] = v } - // TODO(rfindley): consolidate the this workspace mode detection with - // workspace invalidation. - workspaceModeChanged := s.workspaceMode() != result.workspaceMode() - - // We delete invalid metadata in the following cases: - // - If we are forcing a reload of metadata. - // - If the workspace mode has changed, as stale metadata may produce - // confusing or incorrect diagnostics. - // - // TODO(rfindley): we should probably also clear metadata if we are - // reinitializing the workspace, as otherwise we could leave around a bunch - // of irrelevant and duplicate metadata (for example, if the module path - // changed). However, this breaks the "experimentalUseInvalidMetadata" - // feature, which relies on stale metadata when, for example, a go.mod file - // is broken via invalid syntax. - deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged - // Compute which metadata updates are required. We only need to invalidate // packages directly containing the affected file, and only if it changed in // a relevant way. - metadataUpdates := make(map[PackageID]*KnownMetadata) + metadataUpdates := make(map[PackageID]*Metadata) for k, v := range s.meta.metadata { invalidateMetadata := idsToInvalidate[k] @@ -2012,20 +1961,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC } // Check whether the metadata should be deleted. - if skipID[k] || (invalidateMetadata && deleteInvalidMetadata) { + if skipID[k] || invalidateMetadata { metadataUpdates[k] = nil continue } - - // Check if the metadata has changed. - valid := v.Valid && !invalidateMetadata - if valid != v.Valid { - // Mark invalidated metadata rather than deleting it outright. - metadataUpdates[k] = &KnownMetadata{ - Metadata: v.Metadata, - Valid: valid, - } - } } // Update metadata, if necessary. @@ -2042,6 +1981,10 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // Don't bother copying the importedBy graph, // as it changes each time we update metadata. + // TODO(rfindley): consolidate the this workspace mode detection with + // workspace invalidation. + workspaceModeChanged := s.workspaceMode() != result.workspaceMode() + // If the snapshot's workspace mode has changed, the packages loaded using // the previous mode are no longer relevant, so clear them out. if workspaceModeChanged { diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go index 762054d10b..e283c3b35c 100755 --- a/gopls/internal/lsp/source/api_json.go +++ b/gopls/internal/lsp/source/api_json.go @@ -88,14 +88,6 @@ var GeneratedAPIJSON = &APIJSON{ Status: "experimental", Hierarchy: "build", }, - { - Name: "experimentalUseInvalidMetadata", - Type: "bool", - Doc: "experimentalUseInvalidMetadata enables gopls to fall back on outdated\npackage metadata to provide editor features if the go command fails to\nload packages for some reason (like an invalid go.mod file).\n\nDeprecated: this setting is deprecated and will be removed in a future\nversion of gopls (https://go.dev/issue/55333).\n", - Default: "false", - Status: "experimental", - Hierarchy: "build", - }, { Name: "standaloneTags", Type: "[]string", diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go index 23d795ef0e..20c8f85ed6 100644 --- a/gopls/internal/lsp/source/options.go +++ b/gopls/internal/lsp/source/options.go @@ -292,14 +292,6 @@ type BuildOptions struct { // be removed. AllowImplicitNetworkAccess bool `status:"experimental"` - // ExperimentalUseInvalidMetadata enables gopls to fall back on outdated - // package metadata to provide editor features if the go command fails to - // load packages for some reason (like an invalid go.mod file). - // - // Deprecated: this setting is deprecated and will be removed in a future - // version of gopls (https://go.dev/issue/55333). - ExperimentalUseInvalidMetadata bool `status:"experimental"` - // StandaloneTags specifies a set of build constraints that identify // individual Go source files that make up the entire main package of an // executable. @@ -1128,10 +1120,7 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) result.setBool(&o.AllowImplicitNetworkAccess) case "experimentalUseInvalidMetadata": - const msg = "experimentalUseInvalidMetadata is deprecated, and will be removed " + - "in a future version of gopls (https://go.dev/issue/55333)" - result.softErrorf(msg) - result.setBool(&o.ExperimentalUseInvalidMetadata) + result.deprecated("") case "standaloneTags": result.setStringSlice(&o.StandaloneTags) diff --git a/gopls/internal/regtest/diagnostics/diagnostics_test.go b/gopls/internal/regtest/diagnostics/diagnostics_test.go index 18c022cb2a..45bf60267d 100644 --- a/gopls/internal/regtest/diagnostics/diagnostics_test.go +++ b/gopls/internal/regtest/diagnostics/diagnostics_test.go @@ -1901,159 +1901,6 @@ func main() {} }) } -func TestUseOfInvalidMetadata(t *testing.T) { - testenv.NeedsGo1Point(t, 13) - - const mod = ` --- go.mod -- -module mod.com - -go 1.12 --- main.go -- -package main - -import ( - "mod.com/a" - //"os" -) - -func _() { - a.Hello() - os.Getenv("") - //var x int -} --- a/a.go -- -package a - -func Hello() {} -` - WithOptions( - Settings{"experimentalUseInvalidMetadata": true}, - Modes(Default), - ).Run(t, mod, func(t *testing.T, env *Env) { - env.OpenFile("go.mod") - env.RegexpReplace("go.mod", "module mod.com", "modul mod.com") // break the go.mod file - env.SaveBufferWithoutActions("go.mod") - env.Await( - env.DiagnosticAtRegexp("go.mod", "modul"), - ) - // Confirm that language features work with invalid metadata. - env.OpenFile("main.go") - file, pos := env.GoToDefinition("main.go", env.RegexpSearch("main.go", "Hello")) - wantPos := env.RegexpSearch("a/a.go", "Hello") - if file != "a/a.go" && pos != wantPos { - t.Fatalf("expected a/a.go:%s, got %s:%s", wantPos, file, pos) - } - // Confirm that new diagnostics appear with invalid metadata by adding - // an unused variable to the body of the function. - env.RegexpReplace("main.go", "//var x int", "var x int") - env.Await( - env.DiagnosticAtRegexp("main.go", "x"), - ) - // Add an import and confirm that we get a diagnostic for it, since the - // metadata will not have been updated. - env.RegexpReplace("main.go", "//\"os\"", "\"os\"") - env.Await( - env.DiagnosticAtRegexp("main.go", `"os"`), - ) - // Fix the go.mod file and expect the diagnostic to resolve itself. - env.RegexpReplace("go.mod", "modul mod.com", "module mod.com") - env.SaveBuffer("go.mod") - env.Await( - env.DiagnosticAtRegexp("main.go", "x"), - env.NoDiagnosticAtRegexp("main.go", `"os"`), - EmptyDiagnostics("go.mod"), - ) - }) -} - -func TestReloadInvalidMetadata(t *testing.T) { - // We only use invalid metadata for Go versions > 1.12. - testenv.NeedsGo1Point(t, 13) - - const mod = ` --- go.mod -- -module mod.com - -go 1.12 --- main.go -- -package main - -func _() {} -` - WithOptions( - Settings{"experimentalUseInvalidMetadata": true}, - // ExperimentalWorkspaceModule has a different failure mode for this - // case. - Modes(Default), - ).Run(t, mod, func(t *testing.T, env *Env) { - env.Await( - OnceMet( - InitialWorkspaceLoad, - CompletedWork("Load", 1, false), - ), - ) - - // Break the go.mod file on disk, expecting a reload. - env.WriteWorkspaceFile("go.mod", `modul mod.com - -go 1.12 -`) - env.Await( - OnceMet( - env.DoneWithChangeWatchedFiles(), - env.DiagnosticAtRegexp("go.mod", "modul"), - CompletedWork("Load", 1, false), - ), - ) - - env.OpenFile("main.go") - env.Await(env.DoneWithOpen()) - // The first edit after the go.mod file invalidation should cause a reload. - // Any subsequent simple edits should not. - content := `package main - -func main() { - _ = 1 -} -` - env.EditBuffer("main.go", fake.NewEdit(0, 0, 3, 0, content)) - env.Await( - OnceMet( - env.DoneWithChange(), - CompletedWork("Load", 2, false), - NoLogMatching(protocol.Error, "error loading file"), - ), - ) - env.RegexpReplace("main.go", "_ = 1", "_ = 2") - env.Await( - OnceMet( - env.DoneWithChange(), - CompletedWork("Load", 2, false), - NoLogMatching(protocol.Error, "error loading file"), - ), - ) - // Add an import to the main.go file and confirm that it does get - // reloaded, but the reload fails, so we see a diagnostic on the new - // "fmt" import. - env.EditBuffer("main.go", fake.NewEdit(0, 0, 5, 0, `package main - -import "fmt" - -func main() { - fmt.Println("") -} -`)) - env.Await( - OnceMet( - env.DoneWithChange(), - env.DiagnosticAtRegexp("main.go", `"fmt"`), - CompletedWork("Load", 3, false), - ), - ) - }) -} - func TestLangVersion(t *testing.T) { testenv.NeedsGo1Point(t, 18) // Requires types.Config.GoVersion, new in 1.18. const files = `