fix: avoid publishing stale 'validate' diagnostics

internal/context/context.go

@@ -33,7 +33,7 @@ var (
 	ctxModuleWalker         = &contextKey{"module walker"}
 	ctxRootDir              = &contextKey{"root directory"}
 	ctxCommandPrefix        = &contextKey{"command prefix"}
-	ctxDiags                = &contextKey{"diagnostics"}
+	ctxDiagsNotifier        = &contextKey{"diagnostics notifier"}
 	ctxLsVersion            = &contextKey{"language server version"}
 	ctxProgressToken        = &contextKey{"progress token"}
 	ctxExperimentalFeatures = &contextKey{"experimental features"}
@@ -224,14 +224,14 @@ func ModuleWalker(ctx context.Context) (*module.Walker, error) {
 	return w, nil
 }
 
-func WithDiagnostics(ctx context.Context, diags *diagnostics.Notifier) context.Context {
-	return context.WithValue(ctx, ctxDiags, diags)
+func WithDiagnosticsNotifier(ctx context.Context, diags *diagnostics.Notifier) context.Context {
+	return context.WithValue(ctx, ctxDiagsNotifier, diags)
 }
 
-func Diagnostics(ctx context.Context) (*diagnostics.Notifier, error) {
-	diags, ok := ctx.Value(ctxDiags).(*diagnostics.Notifier)
+func DiagnosticsNotifier(ctx context.Context) (*diagnostics.Notifier, error) {
+	diags, ok := ctx.Value(ctxDiagsNotifier).(*diagnostics.Notifier)
 	if !ok {
-		return nil, missingContextErr(ctxDiags)
+		return nil, missingContextErr(ctxDiagsNotifier)
 	}
 
 	return diags, nil

internal/langserver/diagnostics/diagnostics.go

@@ -14,41 +14,35 @@ import (
 )
 
 type diagContext struct {
-	ctx    context.Context
-	uri    lsp.DocumentURI
-	source string
-	diags  []lsp.Diagnostic
+	ctx   context.Context
+	uri   lsp.DocumentURI
+	diags []lsp.Diagnostic
 }
 
-type diagnosticSource string
+type DiagnosticSource string
 
-type fileDiagnostics map[diagnosticSource][]lsp.Diagnostic
-
-// Notifier is a type responsible for queueing hcl diagnostics to be converted
+// 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
-	diagsCache     map[lsp.DocumentURI]fileDiagnostics
 	closeDiagsOnce sync.Once
 }
 
 func NewNotifier(sessCtx context.Context, logger *log.Logger) *Notifier {
 	n := &Notifier{
-		logger:     logger,
-		sessCtx:    sessCtx,
-		diags:      make(chan diagContext, 50),
-		diagsCache: make(map[lsp.DocumentURI]fileDiagnostics),
+		logger:  logger,
+		sessCtx: sessCtx,
+		diags:   make(chan diagContext, 50),
 	}
 	go n.notify()
 	return n
 }
 
-// PublishHCLDiags accepts a map of hcl diagnostics per file and queues them for publishing.
+// PublishHCLDiags accepts a map of HCL diagnostics per file and queues them for publishing.
 // A dir path is passed which is joined with the filename keys of the map, to form a file URI.
-// A source string is passed and set for each diagnostic, this is typically displayed in the client UI.
-func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags map[string]hcl.Diagnostics, source string) {
+func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags Diagnostics) {
 	select {
 	case <-n.sessCtx.Done():
 		n.closeDiagsOnce.Do(func() {
@@ -59,10 +53,15 @@ func (n *Notifier) PublishHCLDiags(ctx context.Context, dirPath string, diags ma
 	}
 
 	for filename, ds := range diags {
+		fileDiags := make([]lsp.Diagnostic, 0)
+		for source, diags := range ds {
+			fileDiags = append(fileDiags, ilsp.HCLDiagsToLSP(diags, string(source))...)
+		}
+
 		n.diags <- diagContext{
-			ctx: ctx, source: source,
-			diags: ilsp.HCLDiagsToLSP(ds, source),
+			ctx:   ctx,
 			uri:   lsp.DocumentURI(uri.FromPath(filepath.Join(dirPath, filename))),
+			diags: fileDiags,
 		}
 	}
 }
@@ -71,29 +70,33 @@ func (n *Notifier) notify() {
 	for d := range n.diags {
 		if err := jrpc2.ServerFromContext(d.ctx).Notify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{
 			URI:         d.uri,
-			Diagnostics: n.mergeDiags(d.uri, d.source, d.diags),
+			Diagnostics: d.diags,
 		}); err != nil {
 			n.logger.Printf("Error pushing diagnostics: %s", err)
 		}
 	}
 }
 
-// mergeDiags will return all diags from all cached sources for a given uri.
-// the passed diags overwrites the cached entry for the passed source key
-// even if empty
-func (n *Notifier) mergeDiags(uri lsp.DocumentURI, source string, diags []lsp.Diagnostic) []lsp.Diagnostic {
+type Diagnostics map[string]map[DiagnosticSource]hcl.Diagnostics
 
-	fileDiags, ok := n.diagsCache[uri]
-	if !ok {
-		fileDiags = make(fileDiagnostics)
-	}
+func NewDiagnostics() Diagnostics {
+	return make(Diagnostics, 0)
+}
 
-	fileDiags[diagnosticSource(source)] = diags
-	n.diagsCache[uri] = fileDiags
+// EmptyRootDiagnostic allows emptying any diagnostics for
+// the whole directory which were published previously.
+func (d Diagnostics) EmptyRootDiagnostic() Diagnostics {
+	d[""] = make(map[DiagnosticSource]hcl.Diagnostics, 0)
+	return d
+}
 
-	all := []lsp.Diagnostic{}
-	for _, diags := range fileDiags {
-		all = append(all, diags...)
+func (d Diagnostics) Append(src string, diagsMap map[string]hcl.Diagnostics) Diagnostics {
+	for uri, uriDiags := range diagsMap {
+		if _, ok := d[uri]; !ok {
+			d[uri] = make(map[DiagnosticSource]hcl.Diagnostics, 0)
+		}
+		d[uri][DiagnosticSource(src)] = uriDiags
 	}
-	return all
+
+	return d
 }

internal/langserver/diagnostics/diagnostics_test.go

@@ -6,8 +6,8 @@ import (
 	"log"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/hashicorp/hcl/v2"
-	lsp "github.com/hashicorp/terraform-ls/internal/protocol"
 )
 
 var discardLogger = log.New(ioutil.Discard, "", 0)
@@ -17,13 +17,17 @@ func TestDiags_Closes(t *testing.T) {
 	n := NewNotifier(ctx, discardLogger)
 
 	cancel()
-	n.PublishHCLDiags(context.Background(), "", map[string]hcl.Diagnostics{
+
+	diags := NewDiagnostics()
+	diags.Append("test", map[string]hcl.Diagnostics{
 		"test": {
 			{
 				Severity: hcl.DiagError,
 			},
 		},
-	}, "test")
+	})
+
+	n.PublishHCLDiags(context.Background(), t.TempDir(), diags)
 
 	if _, open := <-n.diags; open {
 		t.Fatal("channel should be closed")
@@ -41,81 +45,91 @@ func TestPublish_DoesNotSendAfterClose(t *testing.T) {
 	n := NewNotifier(ctx, discardLogger)
 
 	cancel()
-	n.PublishHCLDiags(context.Background(), "", map[string]hcl.Diagnostics{
+
+	diags := NewDiagnostics()
+	diags.Append("test", map[string]hcl.Diagnostics{
 		"test": {
 			{
 				Severity: hcl.DiagError,
 			},
 		},
-	}, "test")
-}
-
-func TestMergeDiags_CachesMultipleSourcesPerURI(t *testing.T) {
-	uri := lsp.DocumentURI("test.tf")
-
-	n := NewNotifier(context.Background(), discardLogger)
-
-	all := n.mergeDiags(uri, "source1", []lsp.Diagnostic{
-		{
-			Severity: lsp.SeverityError,
-			Message:  "diag1",
-		},
 	})
-	if len(all) != 1 {
-		t.Fatalf("returns diags is incorrect length: expected %d, got %d", 1, len(all))
-	}
 
-	all = n.mergeDiags(uri, "source2", []lsp.Diagnostic{
-		{
-			Severity: lsp.SeverityError,
-			Message:  "diag2",
-		},
-	})
-	if len(all) != 2 {
-		t.Fatalf("returns diags is incorrect length: expected %d, got %d", 2, len(all))
-	}
+	n.PublishHCLDiags(context.Background(), t.TempDir(), diags)
 }
 
-func TestMergeDiags_OverwritesSource_EvenWithEmptySlice(t *testing.T) {
-	uri := lsp.DocumentURI("test.tf")
-
-	n := NewNotifier(context.Background(), discardLogger)
-
-	all := n.mergeDiags(uri, "source1", []lsp.Diagnostic{
-		{
-			Severity: lsp.SeverityError,
-			Message:  "diag1",
+func TestDiagnostics_Append(t *testing.T) {
+	diags := NewDiagnostics()
+	diags.Append("foo", map[string]hcl.Diagnostics{
+		"first.tf": {
+			&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Something went wrong",
+				Detail:   "Testing detail",
+				Subject: &hcl.Range{
+					Filename: "first.tf",
+					Start:    hcl.InitialPos,
+					End: hcl.Pos{
+						Line:   3,
+						Column: 2,
+						Byte:   10,
+					},
+				},
+			},
 		},
 	})
-	if len(all) != 1 {
-		t.Fatalf("returns diags is incorrect length: expected %d, got %d", 1, len(all))
-	}
-
-	all = n.mergeDiags(uri, "source1", []lsp.Diagnostic{
-		{
-			Severity: lsp.SeverityError,
-			Message:  "diagOverwritten",
+	diags.Append("bar", map[string]hcl.Diagnostics{
+		"first.tf": {
+			&hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Something else went wrong",
+				Detail:   "Testing detail",
+			},
+		},
+		"second.tf": {
+			&hcl.Diagnostic{
+				Severity: hcl.DiagWarning,
+				Summary:  "Beware",
+			},
 		},
 	})
-	if len(all) != 1 {
-		t.Fatalf("returns diags is incorrect length: expected %d, got %d", 1, len(all))
-	}
-	if all[0].Message != "diagOverwritten" {
-		t.Fatalf("diag has incorrect message: expected %s, got %s", "diagOverwritten", all[0].Message)
-	}
 
-	all = n.mergeDiags(uri, "source2", []lsp.Diagnostic{
-		{
-			Severity: lsp.SeverityError,
-			Message:  "diag2",
+	expectedDiags := Diagnostics{
+		"first.tf": map[DiagnosticSource]hcl.Diagnostics{
+			DiagnosticSource("foo"): {
+				&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Something went wrong",
+					Detail:   "Testing detail",
+					Subject: &hcl.Range{
+						Filename: "first.tf",
+						Start:    hcl.InitialPos,
+						End: hcl.Pos{
+							Line:   3,
+							Column: 2,
+							Byte:   10,
+						},
+					},
+				},
+			},
+			DiagnosticSource("bar"): {
+				&hcl.Diagnostic{
+					Severity: hcl.DiagError,
+					Summary:  "Something else went wrong",
+					Detail:   "Testing detail",
+				},
+			},
+		},
+		"second.tf": map[DiagnosticSource]hcl.Diagnostics{
+			DiagnosticSource("bar"): {
+				&hcl.Diagnostic{
+					Severity: hcl.DiagWarning,
+					Summary:  "Beware",
+				},
+			},
 		},
-	})
-	if len(all) != 2 {
-		t.Fatalf("returns diags is incorrect length: expected %d, got %d", 2, len(all))
 	}
-
-	all = n.mergeDiags(uri, "source2", []lsp.Diagnostic{})
-	if len(all) != 1 {
-		t.Fatalf("returns diags is incorrect length: expected %d, got %d", 1, len(all))
+	if diff := cmp.Diff(expectedDiags, diags); diff != "" {
+		t.Fatalf("diagnostics mismatch: %s", diff)
 	}
 }

internal/langserver/handlers/command/validate.go

@@ -45,7 +45,7 @@ func TerraformValidateHandler(ctx context.Context, args cmd.CommandArgs) (interf
 		return nil, errors.EnrichTfExecError(err)
 	}
 
-	notifier, err := lsctx.Diagnostics(ctx)
+	notifier, err := lsctx.DiagnosticsNotifier(ctx)
 	if err != nil {
 		return nil, err
 	}
@@ -60,9 +60,14 @@ func TerraformValidateHandler(ctx context.Context, args cmd.CommandArgs) (interf
 		return nil, err
 	}
 
-	diags := diagnostics.HCLDiagsFromJSON(jsonDiags)
+	diags := diagnostics.NewDiagnostics()
+	validateDiags := diagnostics.HCLDiagsFromJSON(jsonDiags)
+	diags.EmptyRootDiagnostic()
+	diags.Append("terraform validate", validateDiags)
+	diags.Append("HCL", mod.ModuleDiagnostics)
+	diags.Append("HCL", mod.VarsDiagnostics)
 
-	notifier.PublishHCLDiags(ctx, mod.Path, diags, "terraform validate")
+	notifier.PublishHCLDiags(ctx, mod.Path, diags)
 
 	return nil, nil
 }

internal/langserver/handlers/did_change.go

@@ -5,6 +5,7 @@ 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"
 	op "github.com/hashicorp/terraform-ls/internal/terraform/module/operation"
@@ -81,7 +82,7 @@ func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocument
 		return err
 	}
 
-	diags, err := lsctx.Diagnostics(ctx)
+	notifier, err := lsctx.DiagnosticsNotifier(ctx)
 	if err != nil {
 		return err
 	}
@@ -92,8 +93,12 @@ func TextDocumentDidChange(ctx context.Context, params lsp.DidChangeTextDocument
 		return err
 	}
 
-	mergedDiags := mergeDiagnostics(mod.ModuleDiagnostics, mod.VarsDiagnostics)
-	diags.PublishHCLDiags(ctx, mod.Path, mergedDiags, "HCL")
+	diags := diagnostics.NewDiagnostics()
+	diags.EmptyRootDiagnostic()
+	diags.Append("HCL", mod.ModuleDiagnostics)
+	diags.Append("HCL", mod.VarsDiagnostics)
+
+	notifier.PublishHCLDiags(ctx, mod.Path, diags)
 
 	return nil
 }