From bc5d16d423e84b66e3db6c89cec0b4c661fb1873 Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Wed, 17 Nov 2021 11:22:55 +0100 Subject: [PATCH] decouple diagnostics from handlers into a hook --- .../langserver/diagnostics/diagnostics.go | 19 +++++++------ .../diagnostics/diagnostics_test.go | 24 +++++++++------- internal/langserver/handlers/did_change.go | 17 ----------- internal/langserver/handlers/did_close.go | 17 ----------- internal/langserver/handlers/did_open.go | 18 ------------ internal/langserver/handlers/hooks_module.go | 28 +++++++++++++++++++ internal/langserver/handlers/initialize.go | 4 ++- internal/langserver/handlers/service.go | 14 +++++----- internal/langserver/session/types.go | 9 ++++++ internal/state/module.go | 14 ++++++++-- 10 files changed, 84 insertions(+), 80 deletions(-) diff --git a/internal/langserver/diagnostics/diagnostics.go b/internal/langserver/diagnostics/diagnostics.go index 795b6e3b0..7c2eef357 100644 --- a/internal/langserver/diagnostics/diagnostics.go +++ b/internal/langserver/diagnostics/diagnostics.go @@ -6,7 +6,6 @@ import ( "path/filepath" "sync" - "github.com/creachadair/jrpc2" "github.com/hashicorp/hcl/v2" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" @@ -21,20 +20,24 @@ type diagContext struct { type DiagnosticSource string +type ClientNotifier interface { + Notify(ctx context.Context, method string, params interface{}) error +} + // Notifier is a type responsible for queueing HCL diagnostics to be converted // and sent to the client type Notifier struct { logger *log.Logger - sessCtx context.Context diags chan diagContext + clientNotifier ClientNotifier closeDiagsOnce sync.Once } -func NewNotifier(sessCtx context.Context, logger *log.Logger) *Notifier { +func NewNotifier(clientNotifier ClientNotifier, logger *log.Logger) *Notifier { n := &Notifier{ - logger: logger, - sessCtx: sessCtx, - diags: make(chan diagContext, 50), + logger: logger, + diags: make(chan diagContext, 50), + clientNotifier: clientNotifier, } go n.notify() return n @@ -44,7 +47,7 @@ func NewNotifier(sessCtx context.Context, logger *log.Logger) *Notifier { // A dir path is passed which is joined with the filename keys of the map, to form a file URI. func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags Diagnostics) { select { - case <-n.sessCtx.Done(): + case <-ctx.Done(): n.closeDiagsOnce.Do(func() { close(n.diags) }) @@ -68,7 +71,7 @@ func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags Di func (n *Notifier) notify() { for d := range n.diags { - if err := jrpc2.ServerFromContext(d.ctx).Notify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ + if err := n.clientNotifier.Notify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ URI: d.uri, Diagnostics: d.diags, }); err != nil { diff --git a/internal/langserver/diagnostics/diagnostics_test.go b/internal/langserver/diagnostics/diagnostics_test.go index 1d5beffdd..33dfb46f4 100644 --- a/internal/langserver/diagnostics/diagnostics_test.go +++ b/internal/langserver/diagnostics/diagnostics_test.go @@ -13,10 +13,7 @@ import ( var discardLogger = log.New(ioutil.Discard, "", 0) func TestDiags_Closes(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - n := NewNotifier(ctx, discardLogger) - - cancel() + n := NewNotifier(noopNotifier{}, discardLogger) diags := NewDiagnostics() diags.Append("test", map[string]hcl.Diagnostics{ @@ -27,7 +24,9 @@ func TestDiags_Closes(t *testing.T) { }, }) - n.PublishHCLDiags(context.Background(), t.TempDir(), diags) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + n.PublishHCLDiags(ctx, t.TempDir(), diags) if _, open := <-n.diags; open { t.Fatal("channel should be closed") @@ -41,10 +40,7 @@ func TestPublish_DoesNotSendAfterClose(t *testing.T) { } }() - ctx, cancel := context.WithCancel(context.Background()) - n := NewNotifier(ctx, discardLogger) - - cancel() + n := NewNotifier(noopNotifier{}, discardLogger) diags := NewDiagnostics() diags.Append("test", map[string]hcl.Diagnostics{ @@ -55,7 +51,9 @@ func TestPublish_DoesNotSendAfterClose(t *testing.T) { }, }) - n.PublishHCLDiags(context.Background(), t.TempDir(), diags) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + n.PublishHCLDiags(ctx, t.TempDir(), diags) } func TestDiagnostics_Append(t *testing.T) { @@ -133,3 +131,9 @@ func TestDiagnostics_Append(t *testing.T) { t.Fatalf("diagnostics mismatch: %s", diff) } } + +type noopNotifier struct{} + +func (noopNotifier) Notify(ctx context.Context, method string, params interface{}) error { + return nil +} diff --git a/internal/langserver/handlers/did_change.go b/internal/langserver/handlers/did_change.go index 8a6e4384c..495ce4a1b 100644 --- a/internal/langserver/handlers/did_change.go +++ b/internal/langserver/handlers/did_change.go @@ -5,10 +5,8 @@ import ( "fmt" lsctx "github.com/hashicorp/terraform-ls/internal/context" - "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" - "github.com/hashicorp/terraform-ls/internal/terraform/ast" op "github.com/hashicorp/terraform-ls/internal/terraform/module/operation" ) @@ -82,20 +80,5 @@ func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocument return err } - // TODO - notifier, err := lsctx.DiagnosticsNotifier(ctx) - if err != nil { - return err - } - - diags := diagnostics.NewDiagnostics() - diags.EmptyRootDiagnostic() - diags.Append("HCL", mod.ModuleDiagnostics.AsMap()) - diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap()) - if vf, ok := ast.NewVarsFilename(f.Filename()); ok && !vf.IsAutoloaded() { - diags.Append("HCL", mod.VarsDiagnostics.ForFile(vf).AsMap()) - } - notifier.PublishHCLDiags(ctx, mod.Path, diags) - return nil } diff --git a/internal/langserver/handlers/did_close.go b/internal/langserver/handlers/did_close.go index 019fd35cb..8b4bb52d9 100644 --- a/internal/langserver/handlers/did_close.go +++ b/internal/langserver/handlers/did_close.go @@ -3,12 +3,9 @@ package handlers import ( "context" - "github.com/hashicorp/hcl/v2" lsctx "github.com/hashicorp/terraform-ls/internal/context" - "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" - "github.com/hashicorp/terraform-ls/internal/terraform/ast" ) func TextDocumentDidClose(ctx context.Context, params lsp.DidCloseTextDocumentParams) error { @@ -23,19 +20,5 @@ func TextDocumentDidClose(ctx context.Context, params lsp.DidCloseTextDocumentPa return err } - if vf, ok := ast.NewVarsFilename(fh.Filename()); ok && !vf.IsAutoloaded() { - notifier, err := lsctx.DiagnosticsNotifier(ctx) - if err != nil { - return err - } - - diags := diagnostics.NewDiagnostics() - diags.EmptyRootDiagnostic() - diags.Append("HCL", map[string]hcl.Diagnostics{ - fh.Filename(): {}, - }) - notifier.PublishHCLDiags(ctx, fh.Dir(), diags) - } - return nil } diff --git a/internal/langserver/handlers/did_open.go b/internal/langserver/handlers/did_open.go index 0af8b4046..a324d6f64 100644 --- a/internal/langserver/handlers/did_open.go +++ b/internal/langserver/handlers/did_open.go @@ -4,10 +4,8 @@ import ( "context" lsctx "github.com/hashicorp/terraform-ls/internal/context" - "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" lsp "github.com/hashicorp/terraform-ls/internal/protocol" - "github.com/hashicorp/terraform-ls/internal/terraform/ast" "github.com/hashicorp/terraform-ls/internal/terraform/module" op "github.com/hashicorp/terraform-ls/internal/terraform/module/operation" ) @@ -70,21 +68,5 @@ func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpe } } - // TODO: move - notifier, err := lsctx.DiagnosticsNotifier(ctx) - if err != nil { - return err - } - - diags := diagnostics.NewDiagnostics() - diags.EmptyRootDiagnostic() - diags.Append("HCL", mod.ModuleDiagnostics.AsMap()) - diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap()) - if vf, ok := ast.NewVarsFilename(f.Filename()); ok && !vf.IsAutoloaded() { - diags.Append("HCL", mod.VarsDiagnostics.ForFile(vf).AsMap()) - } - - notifier.PublishHCLDiags(ctx, mod.Path, diags) - return nil } diff --git a/internal/langserver/handlers/hooks_module.go b/internal/langserver/handlers/hooks_module.go index b0ae8c4d0..4e61b3d18 100644 --- a/internal/langserver/handlers/hooks_module.go +++ b/internal/langserver/handlers/hooks_module.go @@ -4,6 +4,7 @@ import ( "context" "fmt" + "github.com/hashicorp/terraform-ls/internal/langserver/diagnostics" "github.com/hashicorp/terraform-ls/internal/state" "github.com/hashicorp/terraform-ls/internal/telemetry" "github.com/hashicorp/terraform-schema/backend" @@ -91,3 +92,30 @@ func sendModuleTelemetry(ctx context.Context, store *state.StateStore, telemetry telemetrySender.SendEvent(ctx, "moduleData", properties) } } + +func updateDiagnostics(ctx context.Context, notifier *diagnostics.Notifier) state.ModuleChangeHook { + return func(oldMod, newMod *state.Module) { + // TODO: check if diagnostics have actually changed + oldDiags, newDiags := 0, 0 + if oldMod != nil { + oldDiags = len(oldMod.ModuleDiagnostics) + len(oldMod.VarsDiagnostics) + } + if newMod != nil { + newDiags = len(newMod.ModuleDiagnostics) + len(newMod.VarsDiagnostics) + } + + diags := diagnostics.NewDiagnostics() + diags.EmptyRootDiagnostic() + + defer notifier.PublishHCLDiags(ctx, newMod.Path, diags) + + if oldDiags == 0 && newDiags == 0 { + return + } + + if newMod != nil { + diags.Append("HCL", newMod.ModuleDiagnostics.AsMap()) + diags.Append("HCL", newMod.VarsDiagnostics.AutoloadedOnly().AsMap()) + } + } +} diff --git a/internal/langserver/handlers/initialize.go b/internal/langserver/handlers/initialize.go index 74520d17b..a08166b60 100644 --- a/internal/langserver/handlers/initialize.go +++ b/internal/langserver/handlers/initialize.go @@ -70,9 +70,11 @@ func (svc *service) Initialize(ctx context.Context, params lsp.InitializeParams) expClientCaps := lsp.ExperimentalClientCapabilities(clientCaps.Experimental) + svc.server = jrpc2.ServerFromContext(ctx) + if tv, ok := expClientCaps.TelemetryVersion(); ok { svc.logger.Printf("enabling telemetry (version: %d)", tv) - err := svc.setupTelemetry(tv, jrpc2.ServerFromContext(ctx)) + err := svc.setupTelemetry(tv, svc.server) if err != nil { svc.logger.Printf("failed to setup telemetry: %s", err) } diff --git a/internal/langserver/handlers/service.go b/internal/langserver/handlers/service.go index 33eaa5211..606a61320 100644 --- a/internal/langserver/handlers/service.go +++ b/internal/langserver/handlers/service.go @@ -52,6 +52,8 @@ type service struct { telemetry telemetry.Sender decoder *decoder.Decoder stateStore *state.StateStore + server session.Server + diagsNotifier *diagnostics.Notifier additionalHandlers map[string]rpch.Func } @@ -100,8 +102,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { lh := LogHandler(svc.logger) cc := &lsp.ClientCapabilities{} - notifier := diagnostics.NewNotifier(svc.sessCtx, svc.logger) - rootDir := "" commandPrefix := "" clientName := "" @@ -140,7 +140,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { 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) @@ -150,7 +149,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) ctx = lsctx.WithDocumentStorage(ctx, svc.fs) ctx = lsctx.WithModuleManager(ctx, svc.modMgr) ctx = lsctx.WithWatcher(ctx, svc.watcher) @@ -161,7 +159,6 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) ctx = lsctx.WithDocumentStorage(ctx, svc.fs) return handle(ctx, req, TextDocumentDidClose) }, @@ -287,7 +284,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { return nil, err } - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) + ctx = lsctx.WithDiagnosticsNotifier(ctx, svc.diagsNotifier) ctx = lsctx.WithExperimentalFeatures(ctx, &expFeatures) ctx = lsctx.WithModuleFinder(ctx, svc.modMgr) ctx = exec.WithExecutorOpts(ctx, svc.tfExecOpts) @@ -328,7 +325,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { ctx = lsctx.WithModuleWalker(ctx, svc.walker) ctx = lsctx.WithWatcher(ctx, svc.watcher) ctx = lsctx.WithRootDirectory(ctx, &rootDir) - ctx = lsctx.WithDiagnosticsNotifier(ctx, notifier) + ctx = lsctx.WithDiagnosticsNotifier(ctx, svc.diagsNotifier) ctx = exec.WithExecutorOpts(ctx, svc.tfExecOpts) ctx = exec.WithExecutorFactory(ctx, svc.tfExecFactory) @@ -430,6 +427,8 @@ func (svc *service) configureSessionDependencies(ctx context.Context, cfgOpts *s execOpts.Timeout = d } + svc.diagsNotifier = diagnostics.NewNotifier(svc.server, svc.logger) + svc.tfExecOpts = execOpts svc.sessCtx = exec.WithExecutorOpts(svc.sessCtx, execOpts) @@ -445,6 +444,7 @@ func (svc *service) configureSessionDependencies(ctx context.Context, cfgOpts *s svc.stateStore.SetLogger(svc.logger) svc.stateStore.Modules.ChangeHooks = state.ModuleChangeHooks{ + updateDiagnostics(svc.sessCtx, svc.diagsNotifier), sendModuleTelemetry(svc.sessCtx, svc.stateStore, svc.telemetry), } diff --git a/internal/langserver/session/types.go b/internal/langserver/session/types.go index 194cab8e2..6d129ea74 100644 --- a/internal/langserver/session/types.go +++ b/internal/langserver/session/types.go @@ -18,3 +18,12 @@ type ClientNotifier interface { } type SessionFactory func(context.Context) Session + +type ClientCaller interface { + Callback(ctx context.Context, method string, params interface{}) (*jrpc2.Response, error) +} + +type Server interface { + ClientNotifier + ClientCaller +} diff --git a/internal/state/module.go b/internal/state/module.go index ea55df570..c10fcea5b 100644 --- a/internal/state/module.go +++ b/internal/state/module.go @@ -699,11 +699,12 @@ func (s *ModuleStore) UpdateModuleDiagnostics(path string, diags ast.ModDiags) e txn := s.db.Txn(true) defer txn.Abort() - mod, err := moduleCopyByPath(txn, path) + oldMod, err := moduleByPath(txn, path) if err != nil { return err } + mod := oldMod.Copy() mod.ModuleDiagnostics = diags err = txn.Insert(s.tableName, mod) @@ -711,6 +712,10 @@ func (s *ModuleStore) UpdateModuleDiagnostics(path string, diags ast.ModDiags) e return err } + txn.Defer(func() { + go s.ChangeHooks.notifyModuleChange(oldMod, mod) + }) + txn.Commit() return nil } @@ -719,11 +724,12 @@ func (s *ModuleStore) UpdateVarsDiagnostics(path string, diags ast.VarsDiags) er txn := s.db.Txn(true) defer txn.Abort() - mod, err := moduleCopyByPath(txn, path) + oldMod, err := moduleByPath(txn, path) if err != nil { return err } + mod := oldMod.Copy() mod.VarsDiagnostics = diags err = txn.Insert(s.tableName, mod) @@ -731,6 +737,10 @@ func (s *ModuleStore) UpdateVarsDiagnostics(path string, diags ast.VarsDiags) er return err } + txn.Defer(func() { + go s.ChangeHooks.notifyModuleChange(oldMod, mod) + }) + txn.Commit() return nil }