From f6ea009c5f495984cd59a7db8271d5c46dd36137 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Tue, 17 Jan 2023 17:27:37 -0500 Subject: [PATCH] gopls/internal/lsp: remove the experimentalWatchedFileDelay setting See the associated issue for more details. Fixes golang/go#55332 Change-Id: I062423c090838c87dbb0c2d21ab448f6c72399c6 Reviewed-on: https://go-review.googlesource.com/c/tools/+/462496 Run-TryBot: Robert Findley gopls-CI: kokoro TryBot-Result: Gopher Robot Reviewed-by: Alan Donovan --- gopls/doc/settings.md | 17 ------- gopls/internal/lsp/server.go | 11 ----- gopls/internal/lsp/source/api_json.go | 8 --- gopls/internal/lsp/source/options.go | 17 +------ gopls/internal/lsp/text_synchronization.go | 57 +--------------------- 5 files changed, 2 insertions(+), 108 deletions(-) diff --git a/gopls/doc/settings.md b/gopls/doc/settings.md index 3df086cf82b..1b8f6d779a0 100644 --- a/gopls/doc/settings.md +++ b/gopls/doc/settings.md @@ -358,23 +358,6 @@ This option must be set to a valid duration string, for example `"250ms"`. Default: `"250ms"`. -##### **experimentalWatchedFileDelay** *time.Duration* - -**This setting is experimental and may be deleted.** - -experimentalWatchedFileDelay controls the amount of time that gopls waits -for additional workspace/didChangeWatchedFiles notifications to arrive, -before processing all such notifications in a single batch. This is -intended for use by LSP clients that don't support their own batching of -file system notifications. - -This option must be set to a valid duration string, for example `"100ms"`. - -Deprecated: this setting is deprecated and will be removed in a future -version of gopls (https://go.dev/issue/55332) - -Default: `"0s"`. - #### Documentation ##### **hoverKind** *enum* diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go index 7e70d0a76ab..82d90dcf4f9 100644 --- a/gopls/internal/lsp/server.go +++ b/gopls/internal/lsp/server.go @@ -33,7 +33,6 @@ func NewServer(session *cache.Session, client protocol.ClientCloser) *Server { diagnosticsSema: make(chan struct{}, concurrentAnalyses), progress: progress.NewTracker(client), diagDebouncer: newDebouncer(), - watchedFileDebouncer: newDebouncer(), } } @@ -106,22 +105,12 @@ type Server struct { // diagDebouncer is used for debouncing diagnostics. diagDebouncer *debouncer - // watchedFileDebouncer is used for batching didChangeWatchedFiles notifications. - watchedFileDebouncer *debouncer - fileChangeMu sync.Mutex - pendingOnDiskChanges []*pendingModificationSet - // When the workspace fails to load, we show its status through a progress // report with an error message. criticalErrorStatusMu sync.Mutex criticalErrorStatus *progress.WorkDone } -type pendingModificationSet struct { - diagnoseDone chan struct{} - changes []source.FileModification -} - func (s *Server) workDoneProgressCancel(ctx context.Context, params *protocol.WorkDoneProgressCancelParams) error { return s.progress.Cancel(params.Token) } diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go index 11347e7bf80..0d24ececa1e 100755 --- a/gopls/internal/lsp/source/api_json.go +++ b/gopls/internal/lsp/source/api_json.go @@ -516,14 +516,6 @@ var GeneratedAPIJSON = &APIJSON{ Status: "advanced", Hierarchy: "ui.diagnostic", }, - { - Name: "experimentalWatchedFileDelay", - Type: "time.Duration", - Doc: "experimentalWatchedFileDelay controls the amount of time that gopls waits\nfor additional workspace/didChangeWatchedFiles notifications to arrive,\nbefore processing all such notifications in a single batch. This is\nintended for use by LSP clients that don't support their own batching of\nfile system notifications.\n\nThis option must be set to a valid duration string, for example `\"100ms\"`.\n\nDeprecated: this setting is deprecated and will be removed in a future\nversion of gopls (https://go.dev/issue/55332)\n", - Default: "\"0s\"", - Status: "experimental", - Hierarchy: "ui.diagnostic", - }, { Name: "hints", Type: "map[string]bool", diff --git a/gopls/internal/lsp/source/options.go b/gopls/internal/lsp/source/options.go index ca3441a693b..01feb210d7a 100644 --- a/gopls/internal/lsp/source/options.go +++ b/gopls/internal/lsp/source/options.go @@ -434,18 +434,6 @@ type DiagnosticOptions struct { // // This option must be set to a valid duration string, for example `"250ms"`. DiagnosticsDelay time.Duration `status:"advanced"` - - // ExperimentalWatchedFileDelay controls the amount of time that gopls waits - // for additional workspace/didChangeWatchedFiles notifications to arrive, - // before processing all such notifications in a single batch. This is - // intended for use by LSP clients that don't support their own batching of - // file system notifications. - // - // This option must be set to a valid duration string, for example `"100ms"`. - // - // Deprecated: this setting is deprecated and will be removed in a future - // version of gopls (https://go.dev/issue/55332) - ExperimentalWatchedFileDelay time.Duration `status:"experimental"` } type InlayHintOptions struct { @@ -1120,10 +1108,7 @@ func (o *Options) set(name string, value interface{}, seen map[string]struct{}) result.setDuration(&o.DiagnosticsDelay) case "experimentalWatchedFileDelay": - const msg = "experimentalWatchedFileDelay is deprecated, and will " + - "be removed in a future version of gopls (https://go.dev/issue/55332)" - result.softErrorf(msg) - result.setDuration(&o.ExperimentalWatchedFileDelay) + result.deprecated("") case "experimentalPackageCacheKey": result.setBool(&o.ExperimentalPackageCacheKey) diff --git a/gopls/internal/lsp/text_synchronization.go b/gopls/internal/lsp/text_synchronization.go index e70bf9eb5b6..77e081a8a08 100644 --- a/gopls/internal/lsp/text_synchronization.go +++ b/gopls/internal/lsp/text_synchronization.go @@ -10,14 +10,11 @@ import ( "errors" "fmt" "path/filepath" - "time" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/span" - "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/jsonrpc2" - "golang.org/x/tools/internal/xcontext" ) // ModificationSource identifies the originating cause of a file modification. @@ -220,59 +217,7 @@ func (s *Server) didModifyFiles(ctx context.Context, modifications []source.File } onDisk := cause == FromDidChangeWatchedFiles - delay := s.session.Options().ExperimentalWatchedFileDelay - s.fileChangeMu.Lock() - defer s.fileChangeMu.Unlock() - if !onDisk || delay == 0 { - // No delay: process the modifications immediately. - return s.processModifications(ctx, modifications, onDisk, diagnoseDone) - } - // Debounce and batch up pending modifications from watched files. - pending := &pendingModificationSet{ - diagnoseDone: diagnoseDone, - changes: modifications, - } - // Invariant: changes appended to s.pendingOnDiskChanges are eventually - // handled in the order they arrive. This guarantee is only partially - // enforced here. Specifically: - // 1. s.fileChangesMu ensures that the append below happens in the order - // notifications were received, so that the changes within each batch are - // ordered properly. - // 2. The debounced func below holds s.fileChangesMu while processing all - // changes in s.pendingOnDiskChanges, ensuring that no batches are - // processed out of order. - // 3. Session.ExpandModificationsToDirectories and Session.DidModifyFiles - // process changes in order. - s.pendingOnDiskChanges = append(s.pendingOnDiskChanges, pending) - ctx = xcontext.Detach(ctx) - okc := s.watchedFileDebouncer.debounce("", 0, time.After(delay)) - go func() { - if ok := <-okc; !ok { - return - } - s.fileChangeMu.Lock() - var allChanges []source.FileModification - // For accurate progress notifications, we must notify all goroutines - // waiting for the diagnose pass following a didChangeWatchedFiles - // notification. This is necessary for regtest assertions. - var dones []chan struct{} - for _, pending := range s.pendingOnDiskChanges { - allChanges = append(allChanges, pending.changes...) - dones = append(dones, pending.diagnoseDone) - } - - allDone := make(chan struct{}) - if err := s.processModifications(ctx, allChanges, onDisk, allDone); err != nil { - event.Error(ctx, "processing delayed file changes", err) - } - s.pendingOnDiskChanges = nil - s.fileChangeMu.Unlock() - <-allDone - for _, done := range dones { - close(done) - } - }() - return nil + return s.processModifications(ctx, modifications, onDisk, diagnoseDone) } // processModifications update server state to reflect file changes, and