Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate On Save #340

Merged
merged 4 commits into from
Jan 5, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/SETTINGS.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ Or if left empty
This setting should be deprecated once the language server supports multiple workspaces,
as this arises in VS code because a server instance is started per VS Code workspace.

## `experimentalFeatures`

This setting contains inner settings used to opt into experimental features not yet ready to be on by default.

### `experimentalFeatures.validateOnSave`

Enabling this feature will run terraform validate within the folder of the file saved. This comes with some user experience caveats.
- Validation is not run on file open, only once it's saved.
- When editing a module file, validation is not run due to not knowing which "rootmodule" to run validation from (there could be multiple). This creates an awkward workflow where when saving a file in a rootmodule, a diagnostic is raised in a module file. Editing the module file will not clear the diagnostic for the reason mentioned above, it will only clear once a file is saved back in the original "rootmodule". We will continue to attempt improve this user experience.

## How to pass settings

The server expects static settings to be passed as part of LSP `initialize` call,
Expand Down
58 changes: 41 additions & 17 deletions internal/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/hashicorp/terraform-ls/internal/filesystem"
"github.com/hashicorp/terraform-ls/internal/langserver/diagnostics"
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
"github.com/hashicorp/terraform-ls/internal/settings"
"github.com/hashicorp/terraform-ls/internal/terraform/rootmodule"
"github.com/hashicorp/terraform-ls/internal/watcher"
)
Expand All @@ -20,23 +21,24 @@ func (k *contextKey) String() string {
}

var (
ctxDs = &contextKey{"document storage"}
ctxClientCapsSetter = &contextKey{"client capabilities setter"}
ctxClientCaps = &contextKey{"client capabilities"}
ctxTfExecPath = &contextKey{"terraform executable path"}
ctxTfExecLogPath = &contextKey{"terraform executor log path"}
ctxTfExecTimeout = &contextKey{"terraform execution timeout"}
ctxWatcher = &contextKey{"watcher"}
ctxRootModuleMngr = &contextKey{"root module manager"}
ctxTfFormatterFinder = &contextKey{"terraform formatter finder"}
ctxRootModuleCaFi = &contextKey{"root module candidate finder"}
ctxRootModuleWalker = &contextKey{"root module walker"}
ctxRootModuleLoader = &contextKey{"root module loader"}
ctxRootDir = &contextKey{"root directory"}
ctxCommandPrefix = &contextKey{"command prefix"}
ctxDiags = &contextKey{"diagnostics"}
ctxLsVersion = &contextKey{"language server version"}
ctxProgressToken = &contextKey{"progress token"}
ctxDs = &contextKey{"document storage"}
ctxClientCapsSetter = &contextKey{"client capabilities setter"}
ctxClientCaps = &contextKey{"client capabilities"}
ctxTfExecPath = &contextKey{"terraform executable path"}
ctxTfExecLogPath = &contextKey{"terraform executor log path"}
ctxTfExecTimeout = &contextKey{"terraform execution timeout"}
ctxWatcher = &contextKey{"watcher"}
ctxRootModuleMngr = &contextKey{"root module manager"}
ctxTfFormatterFinder = &contextKey{"terraform formatter finder"}
ctxRootModuleCaFi = &contextKey{"root module candidate finder"}
ctxRootModuleWalker = &contextKey{"root module walker"}
ctxRootModuleLoader = &contextKey{"root module loader"}
ctxRootDir = &contextKey{"root directory"}
ctxCommandPrefix = &contextKey{"command prefix"}
ctxDiags = &contextKey{"diagnostics"}
ctxLsVersion = &contextKey{"language server version"}
ctxProgressToken = &contextKey{"progress token"}
ctxExperimentalFeatures = &contextKey{"experimental features"}
)

func missingContextErr(ctxKey *contextKey) *MissingContextErr {
Expand Down Expand Up @@ -262,3 +264,25 @@ func ProgressToken(ctx context.Context) (lsp.ProgressToken, bool) {
}
return pt, true
}

func WithExperimentalFeatures(ctx context.Context, expFeatures *settings.ExperimentalFeatures) context.Context {
return context.WithValue(ctx, ctxExperimentalFeatures, expFeatures)
}

func SetExperimentalFeatures(ctx context.Context, expFeatures settings.ExperimentalFeatures) error {
e, ok := ctx.Value(ctxExperimentalFeatures).(*settings.ExperimentalFeatures)
if !ok {
return missingContextErr(ctxExperimentalFeatures)
}

*e = expFeatures
return nil
}

func ExperimentalFeatures(ctx context.Context) (settings.ExperimentalFeatures, error) {
expFeatures, ok := ctx.Value(ctxExperimentalFeatures).(*settings.ExperimentalFeatures)
if !ok {
return settings.ExperimentalFeatures{}, missingContextErr(ctxExperimentalFeatures)
}
return *expFeatures, nil
}
9 changes: 5 additions & 4 deletions internal/langserver/handlers/command/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/creachadair/jrpc2/code"
lsctx "github.com/hashicorp/terraform-ls/internal/context"
"github.com/hashicorp/terraform-ls/internal/langserver/cmd"
"github.com/hashicorp/terraform-ls/internal/langserver/progress"
ilsp "github.com/hashicorp/terraform-ls/internal/lsp"
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
)
Expand All @@ -29,18 +30,18 @@ func TerraformInitHandler(ctx context.Context, args cmd.CommandArgs) (interface{
return nil, err
}

progressBegin(ctx, "Initializing")
progress.Begin(ctx, "Initializing")
defer func() {
progressEnd(ctx, "Finished")
progress.End(ctx, "Finished")
}()

progressReport(ctx, "Running terraform init ...")
progress.Report(ctx, "Running terraform init ...")
err = rm.ExecuteTerraformInit(ctx)
if err != nil {
return nil, err
}

progressReport(ctx, "Detecting paths to watch ...")
progress.Report(ctx, "Detecting paths to watch ...")
paths := rm.PathsToWatch()

w, err := lsctx.Watcher(ctx)
Expand Down
7 changes: 4 additions & 3 deletions internal/langserver/handlers/command/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/creachadair/jrpc2/code"
lsctx "github.com/hashicorp/terraform-ls/internal/context"
"github.com/hashicorp/terraform-ls/internal/langserver/cmd"
"github.com/hashicorp/terraform-ls/internal/langserver/progress"
ilsp "github.com/hashicorp/terraform-ls/internal/lsp"
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
)
Expand Down Expand Up @@ -42,11 +43,11 @@ func TerraformValidateHandler(ctx context.Context, args cmd.CommandArgs) (interf
return nil, err
}

progressBegin(ctx, "Validating")
progress.Begin(ctx, "Validating")
defer func() {
progressEnd(ctx, "Finished")
progress.End(ctx, "Finished")
}()
progressReport(ctx, "Running terraform validate ...")
progress.Report(ctx, "Running terraform validate ...")
hclDiags, err := rm.ExecuteTerraformValidate(ctx)
if err != nil {
return nil, err
Expand Down
30 changes: 30 additions & 0 deletions internal/langserver/handlers/did_save.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package handlers

import (
"context"

lsctx "github.com/hashicorp/terraform-ls/internal/context"
"github.com/hashicorp/terraform-ls/internal/langserver/cmd"
"github.com/hashicorp/terraform-ls/internal/langserver/handlers/command"
ilsp "github.com/hashicorp/terraform-ls/internal/lsp"
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
)

func (lh *logHandler) TextDocumentDidSave(ctx context.Context, params lsp.DidSaveTextDocumentParams) error {
expFeatures, err := lsctx.ExperimentalFeatures(ctx)
if err != nil {
return err
}
if !expFeatures.ValidateOnSave {
return nil
}

fh := ilsp.FileHandlerFromDocumentURI(params.TextDocument.URI)
dh := ilsp.FileHandlerFromDirPath(fh.Dir())

_, err = command.TerraformValidateHandler(ctx, cmd.CommandArgs{
"uri": dh.URI(),
})

return err
}
3 changes: 3 additions & 0 deletions internal/langserver/handlers/initialize.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,9 @@ func (lh *logHandler) Initialize(ctx context.Context, params lsp.InitializeParam
},
}

// set experimental feature flags
lsctx.SetExperimentalFeatures(ctx, out.Options.ExperimentalFeatures)

if len(out.UnusedKeys) > 0 {
jrpc2.PushNotify(ctx, "window/showMessage", &lsp.ShowMessageParams{
Type: lsp.Warning,
Expand Down
15 changes: 15 additions & 0 deletions internal/langserver/handlers/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/hashicorp/terraform-ls/internal/langserver/diagnostics"
"github.com/hashicorp/terraform-ls/internal/langserver/session"
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
"github.com/hashicorp/terraform-ls/internal/settings"
"github.com/hashicorp/terraform-ls/internal/terraform/rootmodule"
"github.com/hashicorp/terraform-ls/internal/watcher"
)
Expand Down Expand Up @@ -150,6 +151,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) {

rootDir := ""
commandPrefix := ""
var expFeatures settings.ExperimentalFeatures

m := map[string]rpch.Func{
"initialize": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
Expand All @@ -165,6 +167,7 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) {
ctx = lsctx.WithCommandPrefix(ctx, &commandPrefix)
ctx = lsctx.WithRootModuleManager(ctx, svc.modMgr)
ctx = lsctx.WithRootModuleLoader(ctx, rmLoader)
ctx = lsctx.WithExperimentalFeatures(ctx, &expFeatures)

version, ok := lsctx.LanguageServerVersion(svc.srvCtx)
if ok {
Expand Down Expand Up @@ -272,6 +275,18 @@ func (svc *service) Assigner() (jrpc2.Assigner, error) {

return handle(ctx, req, lh.TextDocumentSemanticTokensFull)
},
"textDocument/didSave": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
err := session.CheckInitializationIsConfirmed()
if err != nil {
return nil, err
}

ctx = lsctx.WithDiagnostics(ctx, diags)
ctx = lsctx.WithExperimentalFeatures(ctx, &expFeatures)
ctx = lsctx.WithRootModuleFinder(ctx, svc.modMgr)

return handle(ctx, req, lh.TextDocumentDidSave)
},
"workspace/executeCommand": func(ctx context.Context, req *jrpc2.Request) (interface{}, error) {
err := session.CheckInitializationIsConfirmed()
if err != nil {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package command
package progress

import (
"context"
Expand All @@ -8,7 +8,7 @@ import (
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
)

func progressBegin(ctx context.Context, title string) error {
func Begin(ctx context.Context, title string) error {
token, ok := lsctx.ProgressToken(ctx)
if !ok {
return nil
Expand All @@ -23,7 +23,7 @@ func progressBegin(ctx context.Context, title string) error {
})
}

func progressReport(ctx context.Context, message string) error {
func Report(ctx context.Context, message string) error {
token, ok := lsctx.ProgressToken(ctx)
if !ok {
return nil
Expand All @@ -38,7 +38,7 @@ func progressReport(ctx context.Context, message string) error {
})
}

func progressEnd(ctx context.Context, message string) error {
func End(ctx context.Context, message string) error {
token, ok := lsctx.ProgressToken(ctx)
if !ok {
return nil
Expand Down
7 changes: 7 additions & 0 deletions internal/settings/settings.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,19 @@ import (
"github.com/mitchellh/mapstructure"
)

type ExperimentalFeatures struct {
ValidateOnSave bool `mapstructure:"validateOnSave"`
}

type Options struct {
// RootModulePaths describes a list of absolute paths to root modules
RootModulePaths []string `mapstructure:"rootModulePaths"`
ExcludeModulePaths []string `mapstructure:"excludeModulePaths"`
CommandPrefix string `mapstructure:"commandPrefix"`

// ExperimentalFeatures encapsulates experimental features users can opt into.
ExperimentalFeatures ExperimentalFeatures `mapstructure:"experimentalFeatures"`

// TODO: Need to check for conflict with CLI flags
// TerraformExecPath string
// TerraformExecTimeout time.Duration
Expand Down
37 changes: 36 additions & 1 deletion internal/terraform/rootmodule/root_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -394,9 +394,44 @@ func (rm *rootModule) ExecuteTerraformValidate(ctx context.Context) (map[string]
}

// an entry for each file should exist, even if there are no diags
for filename := range rm.pFilesMap {
for filename := range rm.parsedFiles() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! Looks like this was previously prone to race conditions if we were parsing files (and hence writing into the map) while also reading from that map.

diagsMap[filename] = make(hcl.Diagnostics, 0)
}
// since validation applies to linked modules, create an entry for all
// files of linked modules
for _, m := range rm.moduleManifest.Records {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason for reading the module manifest here? Could we not create the entries "lazily" automatically below? e.g. something like

diags, ok := diagsMap[d.Range.Filename]
if !ok {
    diagsMap[d.Range.Filename] = make(hcl.Diagnostics, 0)
    diags = diagsMap[d.Range.Filename]
}

I understand there is a need for filtering out submodule diags, but couldn't this be done via something like strings.HasPrefix(d.Range.Filename, rm.Path())?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I don't believe so no, the point of this is to create an entry in the map for every possible file, so that clearing diagnostics over the protocol works correctly. I also authored this a while though so I can't describe it in detail, but we can revisit/refactor in follow ups

if m.IsRoot() {
// skip root modules
continue
}
if m.IsExternal() {
// skip external modules
continue
}

absPath := filepath.Join(rm.moduleManifest.rootDir, m.Dir)
infos, err := rm.filesystem.ReadDir(absPath)
if err != nil {
return diagsMap, fmt.Errorf("failed to read module at %q: %w", absPath, err)
}

for _, info := range infos {
if info.IsDir() {
// We only care about files
continue
}

name := info.Name()
if !strings.HasSuffix(name, ".tf") || IsIgnoredFile(name) {
continue
}

// map entries are relative to the parent module path
filename := filepath.Join(m.Dir, name)

diagsMap[filename] = make(hcl.Diagnostics, 0)
}
}

validationDiags, err := rm.tfExec.Validate(ctx)
if err != nil {
Expand Down