Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/tools/gopls: watch file changes on disk #31553

Closed
ianthehat opened this issue Apr 18, 2019 · 26 comments
Closed

x/tools/gopls: watch file changes on disk #31553

ianthehat opened this issue Apr 18, 2019 · 26 comments
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done.
Milestone
gopls/v0.3.0

Comments

@ianthehat
Copy link

We cache a lot of information like the AST and typechecking data per file.
If someone changes a file outside the editor, often by doing things with their vcs (update/branch switch is the most common thing mentioned) we currently do not attempt to re-build that information unless it is invalidated by an editor change.
We should be using the LSP handling for workspace/didChangeWatchedFiles to watch all the files we are caching and invalidating them.
It is not clear if we can/should do anything if the client does not support this capability.
@ianthehat ianthehat added FeatureRequest gopls Issues related to the Go language server, gopls. labels Apr 18, 2019
@ianthehat ianthehat self-assigned this Apr 18, 2019
@myitcv
Copy link
Member

myitcv commented Apr 18, 2019

Does this not contradict what you said the other day in terms of this being the editor's responsibility?

@ianthehat
Copy link
Author

No, workspace/didChangeWatchedFile, s is the mechanism by which the server can ask the client to tell it when a file changes, which makes it the editors responsibility

@myitcv
Copy link
Member

myitcv commented Apr 18, 2019

Got you. I misconstrued the title of the issue: it's actually that gopls watches these files through the client. Thanks

@stamblerre stamblerre changed the title gopls needs to watch changed files x/tools/internal/lsp: watch changed files Apr 18, 2019
@gopherbot gopherbot added this to the Unreleased milestone Apr 18, 2019
@myitcv myitcv mentioned this issue Apr 29, 2019
Closed
@myitcv
Copy link
Member

myitcv commented Apr 29, 2019

Just jotting down a couple of notes/questions:

  • This presumably includes watching the main module go.mod file itself too? i.e. it covers the situation where the user adds/drops/changes a requirement
  • Will the server ask the client to watch files in the read-only module cache?
  • Will the server ask the client to watch files in directory-replace-d modules which are writable? Hopefully yes 😉. Main use case here being gohack esque workflows

@ianthehat
Copy link
Author

  • Watching go.mod - yes
  • Watching module cache - undecided, not doing so is more complicated, and possibly fragile, it would have to demonstrate a significant benefit, and we would have to decide if it caused issues when people clear the cache
  • Watching replaced files- yes

myitcv added a commit to govim/govim that referenced this issue Apr 30, 2019
@myitcv
cmd/govim: temporary implementation of file watching
5b4f6a0 
For now use textDocument/didOpen and textDocument/didChange to deal with
generated files changing, whilst we await a proper implementation in
gopls via golang/go#31553.
myitcv added a commit to govim/govim that referenced this issue Apr 30, 2019
@myitcv
cmd/govim: temporary implementation of file watching
eb72aaa 
For now use textDocument/didOpen and textDocument/didChange to deal with
generated files changing, whilst we await a proper implementation in
gopls via golang/go#31553.
@myitcv myitcv mentioned this issue Apr 30, 2019
Merged
myitcv added a commit to govim/govim that referenced this issue Apr 30, 2019
@myitcv
cmd/govim: temporary implementation of file watching
832ffd5 
For now use textDocument/didOpen and textDocument/didChange to deal with
generated files changing, whilst we await a proper implementation in
gopls via golang/go#31553.
myitcv added a commit to govim/govim that referenced this issue Apr 30, 2019
@myitcv
cmd/govim: temporary implementation of file watching
a222541 
For now use textDocument/didOpen and textDocument/didChange to deal with
generated files changing, whilst we await a proper implementation in
gopls via golang/go#31553.
myitcv added a commit to govim/govim that referenced this issue Apr 30, 2019
@myitcv
cmd/govim: temporary implementation of file watching
261cbde 
For now use textDocument/didOpen and textDocument/didChange to deal with
generated files changing, whilst we await a proper implementation in
gopls via golang/go#31553.
myitcv added a commit to govim/govim that referenced this issue Apr 30, 2019
@myitcv
cmd/govim: temporary implementation of file watching (#189)
af25fed 
For now use textDocument/didOpen and textDocument/didChange to deal with
generated files changing, whilst we await a proper implementation in
gopls via golang/go#31553.
@fhs fhs mentioned this issue May 28, 2019
Open
@stamblerre stamblerre mentioned this issue Jun 5, 2019
Closed
@stamblerre stamblerre added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 5, 2019
@stamblerre stamblerre mentioned this issue Jun 12, 2019
Closed
@Asarew Asarew mentioned this issue Jun 19, 2019
Closed
@stamblerre stamblerre changed the title x/tools/internal/lsp: watch changed files x/tools/gopls: watch changed files Jul 2, 2019
@suzmue suzmue self-assigned this Jul 3, 2019
@suzmue
Copy link
Contributor

suzmue commented Jul 3, 2019

To update this thread, I am planning to begin work on implementing workspace/didChangeWatchedFile in gopls

@stamblerre stamblerre added the Suggested Issues that may be good for new contributors looking for work to do. label Jul 10, 2019
@muirdm
Copy link

muirdm commented Jul 10, 2019

Is this one available or does @suzmue still have dibs?

@stamblerre stamblerre removed the Suggested Issues that may be good for new contributors looking for work to do. label Jul 11, 2019
@stamblerre
Copy link
Contributor

I'm sorry, I mistakenly added the Suggested label here. I believe @suzmue is still working on this.

@stamblerre stamblerre mentioned this issue Nov 4, 2019
Closed
@stamblerre stamblerre assigned stamblerre and unassigned ianthehat Nov 29, 2019
@stamblerre stamblerre mentioned this issue Dec 4, 2019
Closed
@stamblerre stamblerre changed the title x/tools/gopls: support workspace/didChangeWatchedFile x/tools/gopls: watch file changes on disk Dec 4, 2019
@stamblerre stamblerre mentioned this issue Dec 4, 2019
Closed
@myitcv
Copy link
Member

myitcv commented Jan 8, 2020

@stamblerre what's the latest status on file watching support in gopls?

I'm wondering if we can remove the workarounds we have in govim.

Thanks

@stamblerre
Copy link
Contributor

It should be coming soon, but not in time for the next gopls release. I've been doing a bit of refactoring to get it to fit in with the rest of the codebase, but the work for this is not yet complete. I will update here when it is.

@muirdm
Copy link

muirdm commented Jan 8, 2020

Not sure if it was intentional, but the options refactoring ended up enabling file watching be default in gopls.

@stamblerre
Copy link
Contributor

Yes, that actually was (I know it doesn't make much sense since it doesn't work yet). I just want to eliminate that option altogether, since I think it will become very difficult to investigate issues if we have multiple modes of updating files. I'm aiming to get the final few CLs out in the coming days.

@gopherbot
Copy link

Change https://golang.org/cl/214277 mentions this issue: internal/lsp: fix support for watching changed files

gopherbot pushed a commit to golang/tools that referenced this issue Jan 13, 2020
@stamblerre
internal/lsp: fix support for watching changed files
86d412b 
This is the beginning of the CLs to refactor the file watching code with
the normal text synchronization code. This hasn't yet been tested other
than with some minimal local testing, so follow-up CLs will be needed.

Updates golang/go#31553

Change-Id: Id081ecc59dd2903057164171bd95f0dc07baa5f1
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214277
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@stamblerre stamblerre modified the milestones: gopls v1.0, gopls/v0.3.0 Jan 15, 2020
@stamblerre
Copy link
Contributor

File watching now works with gopls at master. The remaining task here is to implement batching at the snapshot level, so that a single snapshot can be invalidated by multiple files instead of a single file.

@stamblerre stamblerre added NeedsFix The path to resolution is known, but the work has not been done. and removed FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 16, 2020
@gopherbot
Copy link

Change https://golang.org/cl/215902 mentions this issue: internal/lsp: refactor (*snapshot).clone to handle multiple URIs

gopherbot pushed a commit to golang/tools that referenced this issue Jan 23, 2020
@stamblerre
internal/lsp: refactor (*snapshot).clone to handle multiple URIs
18389cb 
This is the first in a series of changes to batch file changes. First,
we have to support invalidating a snaphot with multiple files.

Updates golang/go#31553

Change-Id: I7cd83d9280e3274549a72393bb9010c47eb5dd1e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215902
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/215906 mentions this issue: internal/lsp: support multiple URIs in (*view).invalidateContent

@gopherbot
Copy link

Change https://golang.org/cl/215907 mentions this issue: internal/lsp: support batched on-disk changes in source.DidModifyFiles

gopherbot pushed a commit to golang/tools that referenced this issue Jan 23, 2020
@stamblerre
internal/lsp: support multiple URIs in (*view).invalidateContent
a356fb7 
Caught a number of unused parameters along the way. Hopefully we can
eliminate the containsFileSave boolean soon, since it's a bit annoying
to have to send that through.

Updates golang/go#31553.

Change-Id: I94319d902d329c84cb1c0676322ac04541ad53a0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215906
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jan 23, 2020
@stamblerre
internal/lsp: support batched on-disk changes in source.DidModifyFiles
e54d0ed 
We don't yet propagate these batched changes in text_synchronization.go,
but this is the next step in moving towards a batched approach.

Updates golang/go#31553

Change-Id: Id6496af9d5422cc50ccb995f81c71ec1886f965a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/215907
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/215908 mentions this issue: internal/lsp: batch file changes in didChangeWatchedFiles

@bhcleek bhcleek mentioned this issue Jan 30, 2020
Closed
@golang golang locked and limited conversation to collaborators Jan 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
gopls/v0.3.0
Development

No branches or pull requests
7 participants
@ianthehat @Helcaraxan @myitcv @stamblerre @suzmue @gopherbot @muirdm