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

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

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