tfdiags: Contextual diagnostics

The usual usage of diagnostics requires us to pass around source location
information to everywhere that might generate a diagnostic, and that is
always the best way to get the most precise diagnostic source locations.

However, it's impractical to require source location information to be
retained in every Terraform subsystem, and so this new idea of "contextual
diagnostics" allows us to separate the generation of a diagnostic from
the resolution of its source location, instead resolving the location
information as a post-processing step once the call stack unwinds to a
place where there is enough context to find it.

This is necessarily a less precise approach than reading the source ranges
directly from the configuration AST, but gives us an alternative to no
diagnostics at all in portions of Terraform where full location
information is not available.

This is a best-effort sort of thing which will get as precise as it can
but may return a range in a parent block if the precise location of a
particular attribute cannot be found. Diagnostics that rely on this
mechanism should include some other contextual information in the detail
message to make up for any loss of precision that results.

tfdiags/contextual.go

@@ -0,0 +1,263 @@
+package tfdiags
+
+import (
+	"github.com/hashicorp/hcl2/hcl"
+	"github.com/zclconf/go-cty/cty"
+	"github.com/zclconf/go-cty/cty/gocty"
+)
+
+// The "contextual" family of diagnostics are designed to allow separating
+// the detection of a problem from placing that problem in context. For
+// example, some code that is validating an object extracted from configuration
+// may not have access to the configuration that generated it, but can still
+// report problems within that object which the caller can then place in
+// context by calling IsConfigBody on the returned diagnostics.
+//
+// When contextual diagnostics are used, the documentation for a method must
+// be very explicit about what context is implied for any diagnostics returned,
+// to help ensure the expected result.
+
+// contextualFromConfig is an interface type implemented by diagnostic types
+// that can elaborate themselves when given information about the configuration
+// body they are embedded in.
+//
+// Usually this entails extracting source location information in order to
+// populate the "Subject" range.
+type contextualFromConfigBody interface {
+	ElaborateFromConfigBody(hcl.Body) Diagnostic
+}
+
+// InConfigBody returns a copy of the receiver with any config-contextual
+// diagnostics elaborated in the context of the given body.
+func (d Diagnostics) InConfigBody(body hcl.Body) Diagnostics {
+	if len(d) == 0 {
+		return nil
+	}
+
+	ret := make(Diagnostics, len(d))
+	for i, srcDiag := range d {
+		if cd, isCD := srcDiag.(contextualFromConfigBody); isCD {
+			ret[i] = cd.ElaborateFromConfigBody(body)
+		} else {
+			ret[i] = srcDiag
+		}
+	}
+
+	return ret
+}
+
+// AttributeValue returns a diagnostic about an attribute value in an implied current
+// configuration context. This should be returned only from functions whose
+// interface specifies a clear configuration context that this will be
+// resolved in.
+//
+// The given path is relative to the implied configuration context. To describe
+// a top-level attribute, it should be a single-element cty.Path with a
+// cty.GetAttrStep. It's assumed that the path is returning into a structure
+// that would be produced by our conventions in the configschema package; it
+// may return unexpected results for structures that can't be represented by
+// configschema.
+//
+// Since mapping attribute paths back onto configuration is an imprecise
+// operation (e.g. dynamic block generation may cause the same block to be
+// evaluated multiple times) the diagnostic detail should include the attribute
+// name and other context required to help the user understand what is being
+// referenced in case the identified source range is not unique.
+//
+// The returned attribute will not have source location information until
+// context is applied to the containing diagnostics using diags.InConfigBody.
+// After context is applied, the source location is the value assigned to the
+// named attribute, or the containing body's "missing item range" if no
+// value is present.
+func AttributeValue(severity Severity, summary, detail string, attrPath cty.Path) Diagnostic {
+	return &attributeDiagnostic{
+		diagnosticBase: diagnosticBase{
+			severity: severity,
+			summary:  summary,
+			detail:   detail,
+		},
+		attrPath: attrPath,
+	}
+}
+
+type attributeDiagnostic struct {
+	diagnosticBase
+	attrPath cty.Path
+	subject  *SourceRange // populated only after ElaborateFromConfigBody
+}
+
+// ElaborateFromConfigBody finds the most accurate possible source location
+// for a diagnostic's attribute path within the given body.
+//
+// Backing out from a path back to a source location is not always entirely
+// possible because we lose some information in the decoding process, so
+// if an exact position cannot be found then the returned diagnostic will
+// refer to a position somewhere within the containing body, which is assumed
+// to be better than no location at all.
+//
+// If possible it is generally better to report an error at a layer where
+// source location information is still available, for more accuracy. This
+// is not always possible due to system architecture, so this serves as a
+// "best effort" fallback behavior for such situations.
+func (d *attributeDiagnostic) ElaborateFromConfigBody(body hcl.Body) Diagnostic {
+	if len(d.attrPath) < 1 {
+		// Should never happen, but we'll allow it rather than crashing.
+		return d
+	}
+
+	if d.subject != nil {
+		// Don't modify an already-elaborated diagnostic.
+		return d
+	}
+
+	ret := *d
+
+	// This function will often end up re-decoding values that were already
+	// decoded by an earlier step. This is non-ideal but is architecturally
+	// more convenient than arranging for source location information to be
+	// propagated to every place in Terraform, and this happens only in the
+	// presence of errors where performance isn't a concern.
+
+	traverse := d.attrPath[:len(d.attrPath)-1]
+	final := d.attrPath[len(d.attrPath)-1]
+
+	// If we have more than one step then we'll first try to traverse to
+	// a child body corresponding to the requested path.
+	for i := 0; i < len(traverse); i++ {
+		step := traverse[i]
+
+		switch tStep := step.(type) {
+		case cty.GetAttrStep:
+
+			var next cty.PathStep
+			if i < (len(traverse) - 1) {
+				next = traverse[i+1]
+			}
+
+			// Will be indexing into our result here?
+			var indexType cty.Type
+			var indexVal cty.Value
+			if nextIndex, ok := next.(cty.IndexStep); ok {
+				indexVal = nextIndex.Key
+				indexType = indexVal.Type()
+				i++ // skip over the index on subsequent iterations
+			}
+
+			var blockLabelNames []string
+			if indexType == cty.String {
+				// Map traversal means we expect one label for the key.
+				blockLabelNames = []string{"key"}
+			}
+
+			// For intermediate steps we expect to be referring to a child
+			// block, so we'll attempt decoding under that assumption.
+			content, _, contentDiags := body.PartialContent(&hcl.BodySchema{
+				Blocks: []hcl.BlockHeaderSchema{
+					{
+						Type:       tStep.Name,
+						LabelNames: blockLabelNames,
+					},
+				},
+			})
+			if contentDiags.HasErrors() {
+				subject := SourceRangeFromHCL(body.MissingItemRange())
+				ret.subject = &subject
+				return &ret
+			}
+			filtered := make([]*hcl.Block, 0, len(content.Blocks))
+			for _, block := range content.Blocks {
+				if block.Type == tStep.Name {
+					filtered = append(filtered, block)
+				}
+			}
+			if len(filtered) == 0 {
+			}
+
+			switch indexType {
+			case cty.NilType: // no index at all
+				if len(filtered) != 1 {
+					subject := SourceRangeFromHCL(body.MissingItemRange())
+					ret.subject = &subject
+					return &ret
+				}
+				body = filtered[0].Body
+			case cty.Number:
+				var idx int
+				err := gocty.FromCtyValue(indexVal, &idx)
+				if err != nil || idx >= len(filtered) {
+					subject := SourceRangeFromHCL(body.MissingItemRange())
+					ret.subject = &subject
+					return &ret
+				}
+				body = filtered[idx].Body
+			case cty.String:
+				key := indexVal.AsString()
+				var block *hcl.Block
+				for _, candidate := range filtered {
+					if candidate.Labels[0] == key {
+						block = candidate
+						break
+					}
+				}
+				if block == nil {
+					// No block with this key, so we'll just indicate a
+					// missing item in the containing block.
+					subject := SourceRangeFromHCL(body.MissingItemRange())
+					ret.subject = &subject
+					return &ret
+				}
+				body = block.Body
+			default:
+				// Should never happen, because only string and numeric indices
+				// are supported by cty collections.
+				subject := SourceRangeFromHCL(body.MissingItemRange())
+				ret.subject = &subject
+				return &ret
+			}
+
+		default:
+			// For any other kind of step, we'll just return our current body
+			// as the subject and accept that this is a little inaccurate.
+			subject := SourceRangeFromHCL(body.MissingItemRange())
+			ret.subject = &subject
+			return &ret
+		}
+	}
+
+	// Default is to indicate a missing item in the deepest body we reached
+	// while traversing.
+	subject := SourceRangeFromHCL(body.MissingItemRange())
+	ret.subject = &subject
+
+	// Once we get here, "final" should be a GetAttr step that maps to an
+	// attribute in our current body.
+	finalStep, isAttr := final.(cty.GetAttrStep)
+	if !isAttr {
+		return &ret
+	}
+
+	content, _, contentDiags := body.PartialContent(&hcl.BodySchema{
+		Attributes: []hcl.AttributeSchema{
+			{
+				Name:     finalStep.Name,
+				Required: true,
+			},
+		},
+	})
+	if contentDiags.HasErrors() {
+		return &ret
+	}
+
+	if attr, ok := content.Attributes[finalStep.Name]; ok {
+		subject = SourceRangeFromHCL(attr.Expr.Range())
+		ret.subject = &subject
+	}
+
+	return &ret
+}
+
+func (d *attributeDiagnostic) Source() Source {
+	return Source{
+		Subject: d.subject,
+	}
+}

tfdiags/contextual_test.go

@@ -0,0 +1,139 @@
+package tfdiags
+
+import (
+	"testing"
+
+	"github.com/go-test/deep"
+	"github.com/hashicorp/hcl2/hcl"
+	"github.com/hashicorp/hcl2/hcl/hclsyntax"
+	"github.com/zclconf/go-cty/cty"
+)
+
+func TestAttributeValue(t *testing.T) {
+	testConfig := `
+foo {
+  bar = "hi"
+}
+foo {
+  bar = "bar"
+}
+bar {
+  bar = "woot"
+}
+baz "a" {
+  bar = "beep"
+}
+baz "b" {
+  bar = "boop"
+}
+`
+	f, parseDiags := hclsyntax.ParseConfig([]byte(testConfig), "test.tf", hcl.Pos{Line: 1, Column: 1})
+	if len(parseDiags) != 0 {
+		t.Fatal(parseDiags)
+	}
+
+	body := f.Body
+	var diags Diagnostics
+	diags = diags.Append(AttributeValue(
+		Error,
+		"foo[0].bar",
+		"detail",
+		cty.Path{
+			cty.GetAttrStep{Name: "foo"},
+			cty.IndexStep{Key: cty.NumberIntVal(0)},
+			cty.GetAttrStep{Name: "bar"},
+		},
+	))
+	diags = diags.Append(AttributeValue(
+		Error,
+		"foo[1].bar",
+		"detail",
+		cty.Path{
+			cty.GetAttrStep{Name: "foo"},
+			cty.IndexStep{Key: cty.NumberIntVal(1)},
+			cty.GetAttrStep{Name: "bar"},
+		},
+	))
+	diags = diags.Append(AttributeValue(
+		Error,
+		"bar.bar",
+		"detail",
+		cty.Path{
+			cty.GetAttrStep{Name: "bar"},
+			cty.GetAttrStep{Name: "bar"},
+		},
+	))
+	diags = diags.Append(AttributeValue(
+		Error,
+		`baz["a"].bar`,
+		"detail",
+		cty.Path{
+			cty.GetAttrStep{Name: "baz"},
+			cty.IndexStep{Key: cty.StringVal("a")},
+			cty.GetAttrStep{Name: "bar"},
+		},
+	))
+	diags = diags.Append(AttributeValue(
+		Error,
+		`baz["b"].bar`,
+		"detail",
+		cty.Path{
+			cty.GetAttrStep{Name: "baz"},
+			cty.IndexStep{Key: cty.StringVal("b")},
+			cty.GetAttrStep{Name: "bar"},
+		},
+	))
+	// Attribute value with subject already populated should not be disturbed.
+	// (in a real case, this might've been passed through from a deeper function
+	// in the call stack, for example.)
+	diags = diags.Append(&attributeDiagnostic{
+		diagnosticBase: diagnosticBase{
+			summary: "preexisting",
+			detail:  "detail",
+		},
+		subject: &SourceRange{
+			Filename: "somewhere_else.tf",
+		},
+	})
+
+	gotDiags := diags.InConfigBody(body)
+
+	wantRanges := map[string]*SourceRange{
+		`foo[0].bar`: {
+			Filename: "test.tf",
+			Start:    SourcePos{Line: 3, Column: 9, Byte: 15},
+			End:      SourcePos{Line: 3, Column: 13, Byte: 19},
+		},
+		`foo[1].bar`: {
+			Filename: "test.tf",
+			Start:    SourcePos{Line: 6, Column: 9, Byte: 36},
+			End:      SourcePos{Line: 6, Column: 14, Byte: 41},
+		},
+		`bar.bar`: {
+			Filename: "test.tf",
+			Start:    SourcePos{Line: 9, Column: 9, Byte: 58},
+			End:      SourcePos{Line: 9, Column: 15, Byte: 64},
+		},
+		`baz["a"].bar`: {
+			Filename: "test.tf",
+			Start:    SourcePos{Line: 12, Column: 9, Byte: 85},
+			End:      SourcePos{Line: 12, Column: 15, Byte: 91},
+		},
+		`baz["b"].bar`: {
+			Filename: "test.tf",
+			Start:    SourcePos{Line: 15, Column: 9, Byte: 112},
+			End:      SourcePos{Line: 15, Column: 15, Byte: 118},
+		},
+		`preexisting`: {
+			Filename: "somewhere_else.tf",
+		},
+	}
+	gotRanges := make(map[string]*SourceRange)
+	for _, diag := range gotDiags {
+		gotRanges[diag.Description().Summary] = diag.Source().Subject
+	}
+
+	for _, problem := range deep.Equal(gotRanges, wantRanges) {
+		t.Error(problem)
+	}
+}

tfdiags/diagnostic_base.go

@@ -0,0 +1,27 @@
+package tfdiags
+
+// diagnosticBase can be embedded in other diagnostic structs to get
+// default implementations of Severity and Description. This type also
+// has a default implementation of Source that returns no source location
+// information, so embedders should generally override that method to
+// return more useful results.
+type diagnosticBase struct {
+	severity Severity
+	summary  string
+	detail   string
+}
+
+func (d diagnosticBase) Severity() Severity {
+	return d.severity
+}
+
+func (d diagnosticBase) Description() Description {
+	return Description{
+		Summary: d.summary,
+		Detail:  d.detail,
+	}
+}
+
+func (d diagnosticBase) Source() Source {
+	return Source{}
+}