Merge pull request #714 from hashicorp/f-mod-ops-sync

refactor: decouple diagnostics from handlers into a hook

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 {

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
+}

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"
 )
 
@@ -61,47 +59,26 @@ func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocument
  return err
  }
 
- err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseModuleConfiguration)
+ err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseModuleConfiguration, nil)
  if err != nil {
  return err
  }
- err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseVariables)
+ err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseVariables, nil)
  if err != nil {
  return err
  }
- // TODO: parallelise the operations below in a workgroup
- err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeLoadModuleMetadata)
+ err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeLoadModuleMetadata, nil)
  if err != nil {
  return err
  }
- err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceTargets)
+ err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceTargets, nil)
  if err != nil {
  return err
  }
- err = modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceOrigins)
+ err = modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceOrigins, nil)
  if err != nil {
  return err
  }
 
- notifier, err := lsctx.DiagnosticsNotifier(ctx)
- if err != nil {
- return err
- }
-
- // obtain fresh module state after the above operations finished
- mod, err = modMgr.ModuleByPath(fh.Dir())
- 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
 }

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
 }

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"
 )
@@ -48,11 +46,11 @@ func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpe
  // We reparse because the file being opened may not match
  // (originally parsed) content on the disk
  // TODO: Do this only if we can verify the file differs?
- modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseModuleConfiguration)
- modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeParseVariables)
- modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeLoadModuleMetadata)
- modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceTargets)
- modMgr.EnqueueModuleOpWait(mod.Path, op.OpTypeDecodeReferenceOrigins)
+ modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseModuleConfiguration, nil)
+ modMgr.EnqueueModuleOp(mod.Path, op.OpTypeParseVariables, nil)
+ modMgr.EnqueueModuleOp(mod.Path, op.OpTypeLoadModuleMetadata, nil)
+ modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceTargets, nil)
+ modMgr.EnqueueModuleOp(mod.Path, op.OpTypeDecodeReferenceOrigins, nil)
 
  if mod.TerraformVersionState == op.OpStateUnknown {
  modMgr.EnqueueModuleOp(mod.Path, op.OpTypeGetTerraformVersion, nil)
@@ -70,20 +68,5 @@ func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpe
  }
  }
 
- 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
 }

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())
+ }
+ }
+}

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)
  }

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),
  }
 

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
+}