From 3bb0bc909f2570b41bb0e5a5a9eb5e740e776c25 Mon Sep 17 00:00:00 2001 From: Alex Pilon Date: Tue, 29 Sep 2020 20:34:14 -0400 Subject: [PATCH 1/4] Implement hcl parse diagnostics --- internal/context/context.go | 15 +++++ langserver/diagnostics/diagnostics.go | 91 +++++++++++++++++++++++++++ langserver/handlers/did_change.go | 10 +++ langserver/handlers/did_open.go | 7 +++ langserver/handlers/service.go | 4 ++ 5 files changed, 127 insertions(+) create mode 100644 langserver/diagnostics/diagnostics.go diff --git a/internal/context/context.go b/internal/context/context.go index 07c20d8c0..8e08f8644 100644 --- a/internal/context/context.go +++ b/internal/context/context.go @@ -7,6 +7,7 @@ import ( "github.com/hashicorp/terraform-ls/internal/filesystem" "github.com/hashicorp/terraform-ls/internal/terraform/rootmodule" "github.com/hashicorp/terraform-ls/internal/watcher" + "github.com/hashicorp/terraform-ls/langserver/diagnostics" "github.com/sourcegraph/go-lsp" ) @@ -33,6 +34,7 @@ var ( ctxRootModuleWalker = &contextKey{"root module walker"} ctxRootModuleLoader = &contextKey{"root module loader"} ctxRootDir = &contextKey{"root directory"} + ctxDiags = &contextKey{"diagnostics"} ) func missingContextErr(ctxKey *contextKey) *MissingContextErr { @@ -211,3 +213,16 @@ func RootModuleLoader(ctx context.Context) (rootmodule.RootModuleLoader, error) } return w, nil } + +func WithDiagnostics(ctx context.Context, diags *diagnostics.Notifier) context.Context { + return context.WithValue(ctx, ctxDiags, diags) +} + +func Diagnostics(ctx context.Context) (*diagnostics.Notifier, error) { + diags, ok := ctx.Value(ctxDiags).(*diagnostics.Notifier) + if !ok { + return nil, missingContextErr(ctxDiags) + } + + return diags, nil +} diff --git a/langserver/diagnostics/diagnostics.go b/langserver/diagnostics/diagnostics.go new file mode 100644 index 000000000..b2ec109b1 --- /dev/null +++ b/langserver/diagnostics/diagnostics.go @@ -0,0 +1,91 @@ +package diagnostics + +import ( + "context" + "sync" + + "github.com/creachadair/jrpc2" + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclparse" + ilsp "github.com/hashicorp/terraform-ls/internal/lsp" + "github.com/sourcegraph/go-lsp" +) + +type documentContext struct { + ctx context.Context + uri lsp.DocumentURI + text []byte +} + +type Notifier struct { + sessCtx context.Context + hclDocs chan<- documentContext + closeHclDocsOnce sync.Once +} + +func NewNotifier(sessCtx context.Context) *Notifier { + hclDocs := make(chan documentContext, 10) + go hclDiags(hclDocs) + return &Notifier{hclDocs: hclDocs, sessCtx: sessCtx} +} + +func (n *Notifier) DiagnoseHCL(ctx context.Context, uri lsp.DocumentURI, text []byte) { + select { + case <-n.sessCtx.Done(): + n.closeHclDocsOnce.Do(func() { + close(n.hclDocs) + }) + return + case n.hclDocs <- documentContext{ctx: ctx, uri: uri, text: text}: + } +} + +func hclDiags(docs <-chan documentContext) { + for d := range docs { + diags := []lsp.Diagnostic{} + + _, hclDiags := hclparse.NewParser().ParseHCL(d.text, string(d.uri)) + for _, hclDiag := range hclDiags { + if hclDiag.Subject != nil { + diags = append(diags, lsp.Diagnostic{ + Range: ilsp.HCLRangeToLSP(*hclDiag.Subject), + Severity: hclSeverityToLSP(hclDiag.Severity), + Source: "HCL", + Message: lspMessage(hclDiag), + }) + } + } + + if err := jrpc2.PushNotify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ + URI: d.uri, + Diagnostics: diags, + }); !acceptableError(err) { + panic(err) + } + } +} + +func hclSeverityToLSP(severity hcl.DiagnosticSeverity) lsp.DiagnosticSeverity { + var sev lsp.DiagnosticSeverity + switch severity { + case hcl.DiagError: + sev = lsp.Error + case hcl.DiagWarning: + sev = lsp.Warning + case hcl.DiagInvalid: + panic("invalid diagnostic") + } + return sev +} + +func lspMessage(diag *hcl.Diagnostic) string { + m := diag.Summary + if diag.Detail != "" { + m += ": " + diag.Detail + } + return m +} + +func acceptableError(err error) bool { + return true +} diff --git a/langserver/handlers/did_change.go b/langserver/handlers/did_change.go index daf10466e..3804cc89b 100644 --- a/langserver/handlers/did_change.go +++ b/langserver/handlers/did_change.go @@ -49,6 +49,16 @@ func TextDocumentDidChange(ctx context.Context, params DidChangeTextDocumentPara return err } + diags, err := lsctx.Diagnostics(ctx) + if err != nil { + return err + } + text, err := f.Text() + if err != nil { + return err + } + diags.DiagnoseHCL(ctx, params.TextDocument.URI, text) + cf, err := lsctx.RootModuleCandidateFinder(ctx) if err != nil { return err diff --git a/langserver/handlers/did_open.go b/langserver/handlers/did_open.go index d7fd4029a..12ce06410 100644 --- a/langserver/handlers/did_open.go +++ b/langserver/handlers/did_open.go @@ -14,6 +14,13 @@ import ( ) func (lh *logHandler) TextDocumentDidOpen(ctx context.Context, params lsp.DidOpenTextDocumentParams) error { + + diags, err := lsctx.Diagnostics(ctx) + if err != nil { + return err + } + diags.DiagnoseHCL(ctx, params.TextDocument.URI, []byte(params.TextDocument.Text)) + fs, err := lsctx.DocumentStorage(ctx) if err != nil { return err diff --git a/langserver/handlers/service.go b/langserver/handlers/service.go index f5c7e2130..bc8dfc1d1 100644 --- a/langserver/handlers/service.go +++ b/langserver/handlers/service.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/terraform-ls/internal/filesystem" "github.com/hashicorp/terraform-ls/internal/terraform/rootmodule" "github.com/hashicorp/terraform-ls/internal/watcher" + "github.com/hashicorp/terraform-ls/langserver/diagnostics" "github.com/hashicorp/terraform-ls/langserver/session" "github.com/sourcegraph/go-lsp" ) @@ -141,6 +142,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { } rmLoader := rootmodule.NewRootModuleLoader(svc.sessCtx, svc.modMgr) + diags := diagnostics.NewNotifier(svc.sessCtx) rootDir := "" @@ -173,6 +175,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } + ctx = lsctx.WithDiagnostics(ctx, diags) ctx = lsctx.WithDocumentStorage(ctx, fs) ctx = lsctx.WithRootModuleCandidateFinder(ctx, svc.modMgr) return handle(ctx, req, TextDocumentDidChange) @@ -182,6 +185,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) { if err != nil { return nil, err } + ctx = lsctx.WithDiagnostics(ctx, diags) ctx = lsctx.WithDocumentStorage(ctx, fs) ctx = lsctx.WithRootDirectory(ctx, &rootDir) ctx = lsctx.WithRootModuleCandidateFinder(ctx, svc.modMgr) From 7b1df0c046d007bd4ab1615b54d1e4435eb78919 Mon Sep 17 00:00:00 2001 From: Alex Pilon Date: Thu, 1 Oct 2020 23:27:42 -0400 Subject: [PATCH 2/4] Refactor and tests --- internal/lsp/convert.go | 50 ++++++++++++ .../{file_change_test.go => convert_test.go} | 0 internal/lsp/file_change.go | 31 -------- langserver/diagnostics/diagnostics.go | 79 ++++++++----------- langserver/diagnostics/diagnostics_test.go | 51 ++++++++++++ langserver/handlers/did_change.go | 1 + 6 files changed, 137 insertions(+), 75 deletions(-) create mode 100644 internal/lsp/convert.go rename internal/lsp/{file_change_test.go => convert_test.go} (100%) create mode 100644 langserver/diagnostics/diagnostics_test.go diff --git a/internal/lsp/convert.go b/internal/lsp/convert.go new file mode 100644 index 000000000..30067dda5 --- /dev/null +++ b/internal/lsp/convert.go @@ -0,0 +1,50 @@ +package lsp + +import ( + "github.com/hashicorp/hcl/v2" + lsp "github.com/sourcegraph/go-lsp" +) + +func HCLRangeToLSP(hclRng hcl.Range) lsp.Range { + return lsp.Range{ + Start: lsp.Position{ + Character: hclRng.Start.Column - 1, + Line: hclRng.Start.Line - 1, + }, + End: lsp.Position{ + Character: hclRng.End.Column - 1, + Line: hclRng.End.Line - 1, + }, + } +} + +func lspRangeToHCL(lspRng lsp.Range, f File) (*hcl.Range, error) { + startPos, err := lspPositionToHCL(f.Lines(), lspRng.Start) + if err != nil { + return nil, err + } + + endPos, err := lspPositionToHCL(f.Lines(), lspRng.End) + if err != nil { + return nil, err + } + + return &hcl.Range{ + Filename: f.Filename(), + Start: startPos, + End: endPos, + }, nil +} + +func HCLSeverityToLSP(severity hcl.DiagnosticSeverity) lsp.DiagnosticSeverity { + var sev lsp.DiagnosticSeverity + switch severity { + case hcl.DiagError: + sev = lsp.Error + case hcl.DiagWarning: + sev = lsp.Warning + case hcl.DiagInvalid: + panic("invalid diagnostic") + } + return sev +} diff --git a/internal/lsp/file_change_test.go b/internal/lsp/convert_test.go similarity index 100% rename from internal/lsp/file_change_test.go rename to internal/lsp/convert_test.go diff --git a/internal/lsp/file_change.go b/internal/lsp/file_change.go index 34e60a0ac..78468332d 100644 --- a/internal/lsp/file_change.go +++ b/internal/lsp/file_change.go @@ -54,37 +54,6 @@ func TextEdits(changes filesystem.DocumentChanges) []lsp.TextEdit { return edits } -func HCLRangeToLSP(hclRng hcl.Range) lsp.Range { - return lsp.Range{ - Start: lsp.Position{ - Character: hclRng.Start.Column - 1, - Line: hclRng.Start.Line - 1, - }, - End: lsp.Position{ - Character: hclRng.End.Column - 1, - Line: hclRng.End.Line - 1, - }, - } -} - -func lspRangeToHCL(lspRng lsp.Range, f File) (*hcl.Range, error) { - startPos, err := lspPositionToHCL(f.Lines(), lspRng.Start) - if err != nil { - return nil, err - } - - endPos, err := lspPositionToHCL(f.Lines(), lspRng.End) - if err != nil { - return nil, err - } - - return &hcl.Range{ - Filename: f.Filename(), - Start: startPos, - End: endPos, - }, nil -} - func (fc *contentChange) Text() string { return fc.text } diff --git a/langserver/diagnostics/diagnostics.go b/langserver/diagnostics/diagnostics.go index b2ec109b1..67c0113c3 100644 --- a/langserver/diagnostics/diagnostics.go +++ b/langserver/diagnostics/diagnostics.go @@ -5,21 +5,22 @@ import ( "sync" "github.com/creachadair/jrpc2" - "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/hclparse" ilsp "github.com/hashicorp/terraform-ls/internal/lsp" "github.com/sourcegraph/go-lsp" ) +// documentContext encapsulates the data needed to diagnose the file and push diagnostics to the client type documentContext struct { ctx context.Context uri lsp.DocumentURI text []byte } +// Notifier is a type responsible for processing documents and pushing diagnostics to the client type Notifier struct { sessCtx context.Context - hclDocs chan<- documentContext + hclDocs chan documentContext closeHclDocsOnce sync.Once } @@ -29,6 +30,9 @@ func NewNotifier(sessCtx context.Context) *Notifier { return &Notifier{hclDocs: hclDocs, sessCtx: sessCtx} } +// DiagnoseHCL enqueues the document for HCL parsing. Documents will be parsed and notifications delivered in order that +// they are enqueued. Files that are actively changing should be enqueued in order, so that diagnostics remain insync with +// the current content of the file. This is the responsibility of the caller. func (n *Notifier) DiagnoseHCL(ctx context.Context, uri lsp.DocumentURI, text []byte) { select { case <-n.sessCtx.Done(): @@ -36,56 +40,43 @@ func (n *Notifier) DiagnoseHCL(ctx context.Context, uri lsp.DocumentURI, text [] close(n.hclDocs) }) return - case n.hclDocs <- documentContext{ctx: ctx, uri: uri, text: text}: + default: } + n.hclDocs <- documentContext{ctx: ctx, uri: uri, text: text} } -func hclDiags(docs <-chan documentContext) { - for d := range docs { - diags := []lsp.Diagnostic{} +func hclParse(doc documentContext) []lsp.Diagnostic { + diags := []lsp.Diagnostic{} - _, hclDiags := hclparse.NewParser().ParseHCL(d.text, string(d.uri)) - for _, hclDiag := range hclDiags { - if hclDiag.Subject != nil { - diags = append(diags, lsp.Diagnostic{ - Range: ilsp.HCLRangeToLSP(*hclDiag.Subject), - Severity: hclSeverityToLSP(hclDiag.Severity), - Source: "HCL", - Message: lspMessage(hclDiag), - }) + _, hclDiags := hclparse.NewParser().ParseHCL(doc.text, string(doc.uri)) + for _, hclDiag := range hclDiags { + // only process diagnostics with an attributable spot in the code + if hclDiag.Subject != nil { + msg := hclDiag.Summary + if hclDiag.Detail != "" { + msg += ": " + hclDiag.Detail } + diags = append(diags, lsp.Diagnostic{ + Range: ilsp.HCLRangeToLSP(*hclDiag.Subject), + Severity: ilsp.HCLSeverityToLSP(hclDiag.Severity), + Source: "HCL", + Message: msg, + }) } - - if err := jrpc2.PushNotify(d.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ - URI: d.uri, - Diagnostics: diags, - }); !acceptableError(err) { - panic(err) - } - } -} - -func hclSeverityToLSP(severity hcl.DiagnosticSeverity) lsp.DiagnosticSeverity { - var sev lsp.DiagnosticSeverity - switch severity { - case hcl.DiagError: - sev = lsp.Error - case hcl.DiagWarning: - sev = lsp.Warning - case hcl.DiagInvalid: - panic("invalid diagnostic") } - return sev + return diags } -func lspMessage(diag *hcl.Diagnostic) string { - m := diag.Summary - if diag.Detail != "" { - m += ": " + diag.Detail +func hclDiags(docs <-chan documentContext) { + for doc := range docs { + // always push diagnostics, even if the slice is empty, this is how previous diagnostics are cleared + // any push error will result in a panic since this is executing in its own thread and we can't bubble + // an error to a jrpc response + if err := jrpc2.PushNotify(doc.ctx, "textDocument/publishDiagnostics", lsp.PublishDiagnosticsParams{ + URI: doc.uri, + Diagnostics: hclParse(doc), + }); err != nil { + panic(err) + } } - return m -} - -func acceptableError(err error) bool { - return true } diff --git a/langserver/diagnostics/diagnostics_test.go b/langserver/diagnostics/diagnostics_test.go new file mode 100644 index 000000000..c776d9695 --- /dev/null +++ b/langserver/diagnostics/diagnostics_test.go @@ -0,0 +1,51 @@ +package diagnostics + +import ( + "testing" + + "golang.org/x/net/context" +) + +func TestDiagnoseHCL_Closes(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + n := NewNotifier(ctx) + cancel() + n.DiagnoseHCL(context.Background(), "", []byte{}) + if _, open := <-n.hclDocs; open { + t.Fatal("documents channel should be closed") + } +} + +func TestDiagnoseHCL_DoesNotSendAfterClose(t *testing.T) { + defer func() { + if err := recover(); err != nil { + t.Fatal(err) + } + }() + ctx, cancel := context.WithCancel(context.Background()) + n := NewNotifier(ctx) + cancel() + n.DiagnoseHCL(context.Background(), "", []byte{}) + n.DiagnoseHCL(context.Background(), "", []byte{}) +} + +func TestHCLParse_ReturnsEmptySliceWhenValid(t *testing.T) { + diags := hclParse(documentContext{ctx: context.Background(), uri: "test", text: hcl(`provider "test" {}`)}) + if diags == nil { + t.Fatal("slice needs to be initialized") + } + if len(diags) > 0 { + t.Fatalf("valid hcl should return an empty slice: %v", diags) + } +} + +func TestHCLParse_ReturnsDiagsWhenInvalid(t *testing.T) { + diags := hclParse(documentContext{ctx: context.Background(), uri: "test", text: hcl(`provider test" {}`)}) + if len(diags) == 0 { + t.Fatal("invalid hcl should return diags") + } +} + +func hcl(text string) []byte { + return append([]byte(text), '\n') +} diff --git a/langserver/handlers/did_change.go b/langserver/handlers/did_change.go index 3804cc89b..6309e885e 100644 --- a/langserver/handlers/did_change.go +++ b/langserver/handlers/did_change.go @@ -49,6 +49,7 @@ func TextDocumentDidChange(ctx context.Context, params DidChangeTextDocumentPara return err } + // now that the file content has been sync'd run diagnostics diags, err := lsctx.Diagnostics(ctx) if err != nil { return err From f830285351473e79d7711caeaa8493e2364fca7a Mon Sep 17 00:00:00 2001 From: Alex Pilon Date: Thu, 1 Oct 2020 23:42:06 -0400 Subject: [PATCH 3/4] Fix import --- langserver/diagnostics/diagnostics_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/langserver/diagnostics/diagnostics_test.go b/langserver/diagnostics/diagnostics_test.go index c776d9695..d377cd14a 100644 --- a/langserver/diagnostics/diagnostics_test.go +++ b/langserver/diagnostics/diagnostics_test.go @@ -1,9 +1,8 @@ package diagnostics import ( + "context" "testing" - - "golang.org/x/net/context" ) func TestDiagnoseHCL_Closes(t *testing.T) { From d89840dcdd37af806bdbb156fb834cd07b449402 Mon Sep 17 00:00:00 2001 From: Alex Pilon Date: Fri, 2 Oct 2020 11:12:19 -0400 Subject: [PATCH 4/4] Rename files --- internal/lsp/diagnostics.go | 19 +++++++++++++++++++ internal/lsp/{convert.go => range.go} | 13 ------------- .../lsp/{convert_test.go => range_test.go} | 0 3 files changed, 19 insertions(+), 13 deletions(-) create mode 100644 internal/lsp/diagnostics.go rename internal/lsp/{convert.go => range.go} (71%) rename internal/lsp/{convert_test.go => range_test.go} (100%) diff --git a/internal/lsp/diagnostics.go b/internal/lsp/diagnostics.go new file mode 100644 index 000000000..7eff360d9 --- /dev/null +++ b/internal/lsp/diagnostics.go @@ -0,0 +1,19 @@ +package lsp + +import ( + "github.com/hashicorp/hcl/v2" + lsp "github.com/sourcegraph/go-lsp" +) + +func HCLSeverityToLSP(severity hcl.DiagnosticSeverity) lsp.DiagnosticSeverity { + var sev lsp.DiagnosticSeverity + switch severity { + case hcl.DiagError: + sev = lsp.Error + case hcl.DiagWarning: + sev = lsp.Warning + case hcl.DiagInvalid: + panic("invalid diagnostic") + } + return sev +} diff --git a/internal/lsp/convert.go b/internal/lsp/range.go similarity index 71% rename from internal/lsp/convert.go rename to internal/lsp/range.go index 30067dda5..0615f124e 100644 --- a/internal/lsp/convert.go +++ b/internal/lsp/range.go @@ -35,16 +35,3 @@ func lspRangeToHCL(lspRng lsp.Range, f File) (*hcl.Range, error) { End: endPos, }, nil } - -func HCLSeverityToLSP(severity hcl.DiagnosticSeverity) lsp.DiagnosticSeverity { - var sev lsp.DiagnosticSeverity - switch severity { - case hcl.DiagError: - sev = lsp.Error - case hcl.DiagWarning: - sev = lsp.Warning - case hcl.DiagInvalid: - panic("invalid diagnostic") - } - return sev -} diff --git a/internal/lsp/convert_test.go b/internal/lsp/range_test.go similarity index 100% rename from internal/lsp/convert_test.go rename to internal/lsp/range_test.go