Support workspace/didChangeWatchedFiles

[radeksimko]

Background

LSP method textDocument/didChange (which is already implemented) can be used to find out about changes made through the IDE, but sometimes files are changed outside of the IDE.

These can be detected through workspace/didChangeWatchedFiles which the LS doesn't implement yet.

Clients may choose to send pretty much arbitrary updates to the server and it is up to the server to filter them. Ideally though, the server should document what clients should watch so it doesn't have to filter garbage unnecessary updates.

Server may also add to the list of watched globs, which is a feature described under #867

UX Impact

Users will see completion, go-to-definition or go-to-references etc. involving indexed but unopened modules to be accurate even when these modules change outside of the editor - e.g. when git branch is switched or files are changed in another editor.

Proposal

• Implement/register new handler for workspace/didChangeWatchedFiles
• Filter files based on appropriate criteria
  • *.tf and *.tfvars files
  • directory is already tracked as a module, i.e. needs updating

    terraform-ls/internal/langserver/handlers/did_change.go

    Lines 49 to 53 in c277ec6

    	// check existence
    	_, err = svc.modStore.ModuleByPath(dh.Dir.Path())
    	if err != nil {
    	return err
    	}

• Re-parse the directory in which relevant files were changed/added/delete

  terraform-ls/internal/langserver/handlers/did_change.go

  Lines 55 to 58 in c277ec6

  	jobIds, err := svc.parseAndDecodeModule(dh.Dir)
  	if err != nil {
  	return err
  	}

• Document what files does the server expect to be watched by default in https://github.com/hashicorp/terraform-ls/blob/main/docs/language-clients.md

[radeksimko]

@danishprakash This might be relatively simple to resolve - or at least it doesn't have any further upstream dependencies, if you want to tackle a new issue.

[danishprakash]

Thanks @radeksimko, I'll start looking into this.

[danishprakash]

Hey @radeksimko, I've a few questions while going through this issue, namely:

1. The spec mentions that the file events can include folders as well. In this case, do we use "modules" or do we walk through the directory and update each file there. I'm still not very clear as to what a module is in the server's context (is it the initialized directory generated by terraform init?), wasn't able to find much to read around this in the code.
2. Regarding the registration of the watchers, how are we handling registration for other methods? For instance, how is the registration for DidChangeTextDocument currently handled? Didn't find any references around this in the codebase at least(I might be missing something though). Otoh, I also noticed that gopls for instance, has some of these glob patterns hard-coded in the server. But at least from the spec and your comment, it seems like it's the client who should define glob patterns for the files to be watched.

Apologies if these are rather trivial questions, I thought It better to clear them before starting with the implementation.

[danishprakash]

@radeksimko are these^ valid questions here? just checking in.

[radeksimko]

Sorry for the delay @danishprakash

The spec mentions that the file events can include folders as well. In this case, do we use "modules" or do we walk through the directory and update each file there.

I think whenever a folder changes, that should be reflected in the virtual filesystem layer, the filesystem shouldn't need to know whether a path belongs to a registered module or whether it even contains any Terraform code.

I'm still not very clear as to what a module is in the server's context (is it the initialized directory generated by terraform init?)

Currently a "module" as we track it in memdb in state storage is generally initialized directory of *.tf files, each of which can get there in a few different ways:

• initial indexing discovers initialized module (i.e. basically directory which contains .terraform) anywhere in the workspace
• user opens a Terraform file within unindexed module - either because it was not indexed yet (i.e. indexer is still indexing) or because the module is not initialized

We have plans to index uninitialized modules too - which practically means any folder with *.tf files (which also aligns better with how "module" is generally understood in Terraform docs), but we don't have an issue tracking this work yet and no work has started yet.

Regarding the registration of the watchers, how are we handling registration for other methods? For instance, how is the registration for DidChangeTextDocument currently handled? Didn't find any references around this in the codebase at least(I might be missing something though).

Are you possibly looking for these places in the codebase?

terraform-ls/internal/langserver/handlers/service.go

Lines 132 to 141 in 132e82d

	"textDocument/didChange": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
	err := session.CheckInitializationIsConfirmed()
	if err != nil {
	return nil, err
	}
	ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier)
	ctx = lsctx.WithDocumentStorage(ctx, svc.fs)
	ctx = lsctx.WithModuleManager(ctx, svc.modMgr)
	return handle(ctx, req, TextDocumentDidChange)
	},

terraform-ls/internal/langserver/handlers/did_change.go

Line 15 in 132e82d

func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocumentParams) error {

Otoh, I also noticed that gopls for instance, has some of these glob patterns hard-coded in the server. But at least from the spec and your comment, it seems like it's the client who should define glob patterns for the files to be watched.

Do you mind linking to the relevant part of gopls codebase, for context?

FWIW our reliance on the locally sourced schema and hence the role of the internal server-side file watcher is no longer as critical since we prebuild the schema since #454 (released in April 2021 in 0.16.0) so we can be a little more flexible in theory. i.e. if the client misconfigures what files are supposed to be watched, it may still affect the UX, but it is not as big of a deal anymore.

Relatedly I think that a first iteration of this feature doesn't have to involve all filetypes, we can start just *.tf, *.tfvars, *.tf.json and *.tfvars.json and later in a later iteration figure out what to do about any other files.

[danishprakash]

Sorry about the delay on this @radeksimko.

Thanks for the thorough response to my questions. I'm still trying to understand the whole thing and from what I can gather till now, we're essentially handling the notifications here. When I mentioned "registration", I actually meant the server registration to the client for a specific event for e.g. this is how gopls registers for the didChangeWatchedFiles event from the server (to the client) and I'm assuming or missing that we're not handling the registration for our handlers. Please correct me here.

With that being said, I'm thinking of moving ahead with this outline. I can go ahead and raise a WIP PR where you can also track the progress if that looks like the right direction.

[radeksimko]

I actually meant the server registration to the client for a specific event for e.g. this is how gopls registers for the didChangeWatchedFiles event from the server (to the client) and I'm assuming or missing that we're not handling the registration for our handlers.

That's correct - dynamic capability registration is an AFAIK optional part of LSP which our server currently doesn't support. This means that all capability negotiation has to happen early during initialize request/response and nothing can change afterwards without restarting the server.

There is no reason for not supporting dynamic (un)registration, other than just "there are other tasks with higher priority currently".

I'm thinking of moving ahead with this outline. I can go ahead and raise a WIP PR where you can also track the progress if that looks like the right direction.

That looks like a reasonable start, feel free to raise a draft PR.

[radeksimko]

FYI @danishprakash I have clarified the scope and added some more notes to the initial comment here + I also created hashicorp/vscode-terraform#1068 which may be an easy way to test this.

Let me know if you need any further help with #790 or if you'd prefer for anyone else to take it over. We'll likely need to prioritize it in the upcoming weeks as part of larger list tracked in hashicorp/vscode-terraform#715

[danishprakash]

@radeksimko thanks for the heads up, I looked into it last week and I'll share an update by EOD today.

[github-actions]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.