Skip to content

Commit

Permalink
Introduce a new job for running early validation (#1346)
Browse files Browse the repository at this point in the history
* Implement unreferenced variable validation (#1357)

Add the ability to use the collected origin and target references in early validation by providing a hook for validation funcs. This also adds a validator for unreferenced variables.

Validation funcs will be provided by terraform-ls for now, but may be moved into hcl-lang in the future.

* Introduce ValidationDiagnostics field to module

* Publish early validation diagnostics

* Include validation diagnotics in changes check

* Introduce early validation job

* Check ValidationDiagnosticsState when running validation

* Run early validation job after collection jobs

* Bump hcl-lang to `b6a3f8`

* Update internal/terraform/module/module_ops.go

Co-authored-by: Radek Simko <radek.simko@gmail.com>

---------

Co-authored-by: James Pogran <jpogran@outlook.com>
Co-authored-by: Radek Simko <radek.simko@gmail.com>
  • Loading branch information
3 people committed Sep 26, 2023
1 parent 9a3c45b commit 086576c
Show file tree
Hide file tree
Showing 11 changed files with 312 additions and 12 deletions.
7 changes: 7 additions & 0 deletions internal/decoder/decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@ import (
"context"

"github.com/hashicorp/hcl-lang/decoder"
"github.com/hashicorp/hcl-lang/lang"
"github.com/hashicorp/hcl-lang/reference"
"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/terraform-ls/internal/codelens"
"github.com/hashicorp/terraform-ls/internal/decoder/validations"
ilsp "github.com/hashicorp/terraform-ls/internal/lsp"
lsp "github.com/hashicorp/terraform-ls/internal/protocol"
"github.com/hashicorp/terraform-ls/internal/state"
Expand Down Expand Up @@ -91,5 +93,10 @@ func DecoderContext(ctx context.Context) decoder.DecoderContext {
}
}

validations := []lang.ValidationFunc{
validations.UnreferencedOrigins,
}
dCtx.Validations = append(dCtx.Validations, validations...)

return dCtx
}
60 changes: 60 additions & 0 deletions internal/decoder/validations/unreferenced_origin.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package validations

import (
"context"
"fmt"

"github.com/hashicorp/hcl-lang/decoder"
"github.com/hashicorp/hcl-lang/lang"
"github.com/hashicorp/hcl-lang/reference"
"github.com/hashicorp/hcl/v2"
)

func UnreferencedOrigins(ctx context.Context) lang.DiagnosticsMap {
diagsMap := make(lang.DiagnosticsMap)

pathCtx, err := decoder.PathCtx(ctx)
if err != nil {
return diagsMap
}

for _, origin := range pathCtx.ReferenceOrigins {
matchableOrigin, ok := origin.(reference.MatchableOrigin)
if !ok {
// we don't report on other origins to avoid complexity for now
// other origins would need to be matched against other
// modules/directories and we cannot be sure the targets are
// available within the workspace or were parsed/decoded/collected
// at the time this event occurs
continue
}

// we only initially validate variables
// resources and data sources can have unknown schema
// and will be researched at a later point
firstStep := matchableOrigin.Address()[0]
if firstStep.String() != "var" {
continue
}

_, ok = pathCtx.ReferenceTargets.Match(matchableOrigin)
if !ok {
// target not found
fileName := origin.OriginRange().Filename
d := &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: fmt.Sprintf("No declaration found for %q", matchableOrigin.Address()),
Subject: origin.OriginRange().Ptr(),
}
diagsMap[fileName] = diagsMap[fileName].Append(d)

continue
}

}

return diagsMap
}
120 changes: 120 additions & 0 deletions internal/decoder/validations/unreferenced_origin_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
// Copyright (c) HashiCorp, Inc.
// SPDX-License-Identifier: MPL-2.0

package validations

import (
"context"
"fmt"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/hashicorp/hcl-lang/decoder"
"github.com/hashicorp/hcl-lang/lang"
"github.com/hashicorp/hcl-lang/reference"
"github.com/hashicorp/hcl/v2"
)

func TestUnreferencedOrigins(t *testing.T) {
tests := []struct {
name string
origins reference.Origins
want lang.DiagnosticsMap
}{
{
name: "undeclared variable",
origins: reference.Origins{
reference.LocalOrigin{
Range: hcl.Range{
Filename: "test.tf",
Start: hcl.Pos{},
End: hcl.Pos{},
},
Addr: lang.Address{
lang.RootStep{Name: "var"},
lang.AttrStep{Name: "foo"},
},
},
},
want: lang.DiagnosticsMap{
"test.tf": hcl.Diagnostics{
&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "No declaration found for \"var.foo\"",
Subject: &hcl.Range{
Filename: "test.tf",
Start: hcl.Pos{},
End: hcl.Pos{},
},
},
},
},
},
{
name: "many undeclared variables",
origins: reference.Origins{
reference.LocalOrigin{
Range: hcl.Range{
Filename: "test.tf",
Start: hcl.Pos{Line: 1, Column: 1, Byte: 0},
End: hcl.Pos{Line: 1, Column: 10, Byte: 10},
},
Addr: lang.Address{
lang.RootStep{Name: "var"},
lang.AttrStep{Name: "foo"},
},
},
reference.LocalOrigin{
Range: hcl.Range{
Filename: "test.tf",
Start: hcl.Pos{Line: 2, Column: 1, Byte: 0},
End: hcl.Pos{Line: 2, Column: 10, Byte: 10},
},
Addr: lang.Address{
lang.RootStep{Name: "var"},
lang.AttrStep{Name: "wakka"},
},
},
},
want: lang.DiagnosticsMap{
"test.tf": hcl.Diagnostics{
&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "No declaration found for \"var.foo\"",
Subject: &hcl.Range{
Filename: "test.tf",
Start: hcl.Pos{Line: 1, Column: 1, Byte: 0},
End: hcl.Pos{Line: 1, Column: 10, Byte: 10},
},
},
&hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "No declaration found for \"var.wakka\"",
Subject: &hcl.Range{
Filename: "test.tf",
Start: hcl.Pos{Line: 2, Column: 1, Byte: 0},
End: hcl.Pos{Line: 2, Column: 10, Byte: 10},
},
},
},
},
},
}

for i, tt := range tests {
t.Run(fmt.Sprintf("%2d-%s", i, tt.name), func(t *testing.T) {
ctx := context.Background()

pathCtx := &decoder.PathContext{
ReferenceOrigins: tt.origins,
}

ctx = decoder.WithPathContext(ctx, pathCtx)

diags := UnreferencedOrigins(ctx)
if diff := cmp.Diff(tt.want["test.tf"], diags["test.tf"]); diff != "" {
t.Fatalf("unexpected diagnostics: %s", diff)
}
})
}
}
13 changes: 13 additions & 0 deletions internal/indexer/document_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,19 @@ func (idx *Indexer) decodeModule(ctx context.Context, modHandle document.DirHand
}
ids = append(ids, refTargetsId)

_, err = idx.jobStore.EnqueueJob(ctx, job.Job{
Dir: modHandle,
Func: func(ctx context.Context) error {
return module.EarlyValidation(ctx, idx.modStore, idx.schemaStore, modHandle.Path())
},
Type: op.OpTypeEarlyValidation.String(),
DependsOn: job.IDs{metaId, refTargetsId},
IgnoreState: ignoreState,
})
if err != nil {
return ids, err
}

// This job may make an HTTP request, and we schedule it in
// the low-priority queue, so we don't want to wait for it.
_, err = idx.jobStore.EnqueueJob(ctx, job.Job{
Expand Down
1 change: 1 addition & 0 deletions internal/langserver/handlers/command/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ func (h *CmdHandler) TerraformValidateHandler(ctx context.Context, args cmd.Comm
validateDiags := diagnostics.HCLDiagsFromJSON(jsonDiags)
diags.EmptyRootDiagnostic()
diags.Append("terraform validate", validateDiags)
diags.Append("early validation", mod.ValidationDiagnostics)
diags.Append("HCL", mod.ModuleDiagnostics.AutoloadedOnly().AsMap())
diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap())

Expand Down
1 change: 1 addition & 0 deletions internal/langserver/handlers/hooks_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ func updateDiagnostics(dNotifier *diagnostics.Notifier) notifier.Hook {
defer dNotifier.PublishHCLDiags(ctx, mod.Path, diags)

if mod != nil {
diags.Append("early validation", mod.ValidationDiagnostics)
diags.Append("HCL", mod.ModuleDiagnostics.AutoloadedOnly().AsMap())
diags.Append("HCL", mod.VarsDiagnostics.AutoloadedOnly().AsMap())
}
Expand Down
73 changes: 65 additions & 8 deletions internal/state/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/hashicorp/go-memdb"
"github.com/hashicorp/go-version"
"github.com/hashicorp/hcl-lang/lang"
"github.com/hashicorp/hcl-lang/reference"
"github.com/hashicorp/hcl/v2"
tfaddr "github.com/hashicorp/terraform-registry-address"
Expand Down Expand Up @@ -135,6 +136,9 @@ type Module struct {

ModuleDiagnostics ast.ModDiags
VarsDiagnostics ast.VarsDiags

ValidationDiagnostics lang.DiagnosticsMap
ValidationDiagnosticsState op.OpState
}

func (m *Module) Copy() *Module {
Expand Down Expand Up @@ -178,6 +182,8 @@ func (m *Module) Copy() *Module {
ModuleParsingState: m.ModuleParsingState,
VarsParsingState: m.VarsParsingState,

ValidationDiagnosticsState: m.ValidationDiagnosticsState,

Meta: m.Meta.Copy(),
MetaErr: m.MetaErr,
MetaState: m.MetaState,
Expand Down Expand Up @@ -211,21 +217,23 @@ func (m *Module) Copy() *Module {
newMod.ModuleDiagnostics = make(ast.ModDiags, len(m.ModuleDiagnostics))
for name, diags := range m.ModuleDiagnostics {
newMod.ModuleDiagnostics[name] = make(hcl.Diagnostics, len(diags))
for i, diag := range diags {
// hcl.Diagnostic is practically immutable once it comes out of parser
newMod.ModuleDiagnostics[name][i] = diag
}
copy(newMod.ModuleDiagnostics[name], diags)
}
}

if m.ValidationDiagnostics != nil {
newMod.ValidationDiagnostics = make(lang.DiagnosticsMap, len(m.ValidationDiagnostics))
for name, diags := range m.ValidationDiagnostics {
newMod.ValidationDiagnostics[name] = make(hcl.Diagnostics, len(diags))
copy(newMod.ValidationDiagnostics[name], diags)
}
}

if m.VarsDiagnostics != nil {
newMod.VarsDiagnostics = make(ast.VarsDiags, len(m.VarsDiagnostics))
for name, diags := range m.VarsDiagnostics {
newMod.VarsDiagnostics[name] = make(hcl.Diagnostics, len(diags))
for i, diag := range diags {
// hcl.Diagnostic is practically immutable once it comes out of parser
newMod.VarsDiagnostics[name][i] = diag
}
copy(newMod.VarsDiagnostics[name], diags)
}
}

Expand All @@ -243,6 +251,7 @@ func newModule(modPath string) *Module {
RefTargetsState: op.OpStateUnknown,
ModuleParsingState: op.OpStateUnknown,
MetaState: op.OpStateUnknown,
ValidationDiagnosticsState: op.OpStateUnknown,
}
}

Expand Down Expand Up @@ -988,6 +997,54 @@ func (s *ModuleStore) UpdateModuleDiagnostics(path string, diags ast.ModDiags) e
return nil
}

func (s *ModuleStore) SetValidationDiagnosticsState(path string, state op.OpState) error {
txn := s.db.Txn(true)
defer txn.Abort()

mod, err := moduleCopyByPath(txn, path)
if err != nil {
return err
}

mod.ValidationDiagnosticsState = state
err = txn.Insert(s.tableName, mod)
if err != nil {
return err
}

txn.Commit()
return nil
}

func (s *ModuleStore) UpdateValidateDiagnostics(path string, diags lang.DiagnosticsMap) error {
txn := s.db.Txn(true)
txn.Defer(func() {
s.SetValidationDiagnosticsState(path, op.OpStateLoaded)
})
defer txn.Abort()

oldMod, err := moduleByPath(txn, path)
if err != nil {
return err
}

mod := oldMod.Copy()
mod.ValidationDiagnostics = diags

err = txn.Insert(s.tableName, mod)
if err != nil {
return err
}

err = s.queueModuleChange(txn, oldMod, mod)
if err != nil {
return err
}

txn.Commit()
return nil
}

func (s *ModuleStore) UpdateVarsDiagnostics(path string, diags ast.VarsDiags) error {
txn := s.db.Txn(true)
defer txn.Abort()
Expand Down
4 changes: 2 additions & 2 deletions internal/state/module_changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,10 +142,10 @@ func (s *ModuleStore) queueModuleChange(txn *memdb.Txn, oldMod, newMod *Module)

oldDiags, newDiags := 0, 0
if oldMod != nil {
oldDiags = oldMod.ModuleDiagnostics.Count() + oldMod.VarsDiagnostics.Count()
oldDiags = oldMod.ModuleDiagnostics.Count() + oldMod.VarsDiagnostics.Count() + oldMod.ValidationDiagnostics.Count()
}
if newMod != nil {
newDiags = newMod.ModuleDiagnostics.Count() + newMod.VarsDiagnostics.Count()
newDiags = newMod.ModuleDiagnostics.Count() + newMod.VarsDiagnostics.Count() + newMod.ValidationDiagnostics.Count()
}
// Comparing diagnostics accurately could be expensive
// so we just treat any non-empty diags as a change
Expand Down
Loading

0 comments on commit 086576c

Please sign in to comment.