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

x/tools/gopls: support workspace/didChangeWatchedFile #31553

Open
ianthehat opened this issue Apr 18, 2019 · 16 comments

Comments

@ianthehat
Copy link

@ianthehat ianthehat commented Apr 18, 2019

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.

@myitcv

This comment has been minimized.

Copy link
Member

@myitcv 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

This comment has been minimized.

Copy link
Author

@ianthehat ianthehat commented Apr 18, 2019

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

This comment has been minimized.

Copy link
Member

@myitcv 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

This comment has been minimized.

Copy link
Member

@myitcv 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

This comment has been minimized.

Copy link
Author

@ianthehat ianthehat commented Apr 29, 2019

  • 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
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
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
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
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
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
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.
@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

This comment has been minimized.

Copy link
Contributor

@suzmue suzmue commented Jul 3, 2019

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

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jul 10, 2019

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

@stamblerre stamblerre removed the Suggested label Jul 11, 2019
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Jul 11, 2019

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

@suzmue

This comment has been minimized.

Copy link
Contributor

@suzmue suzmue commented Jul 12, 2019

@muirrn if you want to get started on this, I am not going to get to it right away

@muirdm

This comment has been minimized.

Copy link

@muirdm muirdm commented Jul 14, 2019

Ok, I am working on this one.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 14, 2019

Change https://golang.org/cl/186097 mentions this issue: internal/lsp: fix watched file protocol constants

gopherbot pushed a commit to golang/tools that referenced this issue Jul 31, 2019
The "Create" and "Delete" WatchKind values were missing from the
generated code because their names were colliding with other
constants. Add "WatchKind" to go.ts prefix map to disambiguate.

Updates golang/go#31553

Change-Id: I60269969831c0822896e87b3f2332ded71748f42
GitHub-Last-Rev: 6d85cf9
GitHub-Pull-Request: #136
Reviewed-on: https://go-review.googlesource.com/c/tools/+/186097
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre changed the title x/tools/gopls: watch changed files x/tools/gopls: implement workspace/didChangeWatchedFile Aug 5, 2019
@stamblerre stamblerre changed the title x/tools/gopls: implement workspace/didChangeWatchedFile x/tools/gopls: support workspace/didChangeWatchedFile Aug 6, 2019
muirdm added a commit to muirdm/tools that referenced this issue Aug 19, 2019
Now we register for and handle didChangeWatchedFiles "change"
events. We don't handle "create" or "delete" yet.

When a file changes on disk, there are two basic cases. If the editor
has the file open, we want to ignore the change since we need to
respect the file contents in the editor. If the file isn't open in the
editor then we need to re-type check (and re-diagnose) any packages it
belongs to.

We will need special handling of go.mod changes, but start with
just *.go files for now.

I'm putting the new behavior behind an initialization flag while it is
under development.

Updates golang/go#31553

Change-Id: I81a767ebe12f5f82657752dcdfb069c5820cbaa0
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 19, 2019

Change https://golang.org/cl/190857 mentions this issue: internal/lsp: start handling watched file change events

muirdm added a commit to muirdm/tools that referenced this issue Aug 19, 2019
Now we register for and handle didChangeWatchedFiles "change"
events. We don't handle "create" or "delete" yet.

When a file changes on disk, there are two basic cases. If the editor
has the file open, we want to ignore the change since we need to
respect the file contents in the editor. If the file isn't open in the
editor then we need to re-type check (and re-diagnose) any packages it
belongs to.

We will need special handling of go.mod changes, but start with
just *.go files for now.

I'm putting the new behavior behind an initialization flag while it is
under development.

Updates golang/go#31553

Change-Id: I81a767ebe12f5f82657752dcdfb069c5820cbaa0
@stamblerre stamblerre modified the milestones: Unreleased, gopls v1.0 Aug 26, 2019
gopherbot pushed a commit to golang/tools that referenced this issue Aug 26, 2019
Now we register for and handle didChangeWatchedFiles "change"
events. We don't handle "create" or "delete" yet.

When a file changes on disk, there are two basic cases. If the editor
has the file open, we want to ignore the change since we need to
respect the file contents in the editor. If the file isn't open in the
editor then we need to re-type check (and re-diagnose) any packages it
belongs to.

We will need special handling of go.mod changes, but start with
just *.go files for now.

I'm putting the new behavior behind an initialization flag while it is
under development.

Updates golang/go#31553

Change-Id: I81a767ebe12f5f82657752dcdfb069c5820cbaa0
Reviewed-on: https://go-review.googlesource.com/c/tools/+/190857
Reviewed-by: Ian Cottrell <iancottrell@google.com>
Run-TryBot: Ian Cottrell <iancottrell@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@Helcaraxan

This comment has been minimized.

Copy link
Contributor

@Helcaraxan Helcaraxan commented Aug 27, 2019

As discussed on the golang-tools call. If we are counting on the clients sending the workspace/didChangeWatchedFile event we need to make sure that the major clients actually handle this well.

There have been extensively documented and long-running issues in VSCode with file-watching. These are not Go-specific but rather with the editor itself. Despite an "experimental" new file-watcher being implemented this is still something that hasn't been addressed. Regardless of the non-Go-specificity this has notable knock-on effects on Go projects part of larger code-bases where gopls would be relying on the editor to keep track of changed files.

One entry-point to the nebula of GH issues related to this is microsoft/vscode#59679.

@suzmue suzmue removed their assignment Aug 30, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 4, 2019

Change https://golang.org/cl/193121 mentions this issue: internal/lsp: start handling watched file deletes

gopherbot pushed a commit to golang/tools that referenced this issue Sep 10, 2019
Now when a file is deleted we force the file's packages to refresh
go/packages metadata, and kick off diagnostics.

I made a couple other changes to watched file handling:
- Kick off diagnostics in a goroutine to match how DidChange works.
  This will allow us to work through big sets of file changes faster,
  and will save duplicated work once type checking can be canceled.
- Don't assume a watched file is only part of one view.

Two interesting cases we don't handle yet:
- If the deleted file was the only file in the package, we don't
  currently update diagnostics for dependent packages. This requires
  rejiggering how diagnostics are invoked a bit.
- If the deleted file is still open in the editor and then later
  closed, we don't trigger metadata/diagnostics refresh on DidClose.

Updates golang/go#31553

Change-Id: I65768614c24d9800ffea149ccdbdbd3cb7b2f3d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193121
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
eduvim added a commit to eduvim/tools that referenced this issue Sep 11, 2019
Now when a file is deleted we force the file's packages to refresh
go/packages metadata, and kick off diagnostics.

I made a couple other changes to watched file handling:
- Kick off diagnostics in a goroutine to match how DidChange works.
  This will allow us to work through big sets of file changes faster,
  and will save duplicated work once type checking can be canceled.
- Don't assume a watched file is only part of one view.

Two interesting cases we don't handle yet:
- If the deleted file was the only file in the package, we don't
  currently update diagnostics for dependent packages. This requires
  rejiggering how diagnostics are invoked a bit.
- If the deleted file is still open in the editor and then later
  closed, we don't trigger metadata/diagnostics refresh on DidClose.

Updates golang/go#31553

Change-Id: I65768614c24d9800ffea149ccdbdbd3cb7b2f3d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193121
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 16, 2019

Change https://golang.org/cl/195537 mentions this issue: internal/lsp: start handling watched file creates

clintjedwards added a commit to clintjedwards/tools that referenced this issue Sep 19, 2019
Now when a file is deleted we force the file's packages to refresh
go/packages metadata, and kick off diagnostics.

I made a couple other changes to watched file handling:
- Kick off diagnostics in a goroutine to match how DidChange works.
  This will allow us to work through big sets of file changes faster,
  and will save duplicated work once type checking can be canceled.
- Don't assume a watched file is only part of one view.

Two interesting cases we don't handle yet:
- If the deleted file was the only file in the package, we don't
  currently update diagnostics for dependent packages. This requires
  rejiggering how diagnostics are invoked a bit.
- If the deleted file is still open in the editor and then later
  closed, we don't trigger metadata/diagnostics refresh on DidClose.

Updates golang/go#31553

Change-Id: I65768614c24d9800ffea149ccdbdbd3cb7b2f3d8
Reviewed-on: https://go-review.googlesource.com/c/tools/+/193121
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@stamblerre

This comment has been minimized.

Copy link
Contributor

@stamblerre stamblerre commented Oct 18, 2019

A reminder for an optimization that needs to be added: We should cache a mapping of directories to file URIs to speed up file creation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.