Skip to content

Commit

Permalink
hcl: Allow individual diagnostics to carry extra information
Browse files Browse the repository at this point in the history
The primary goal of the diagnostics design in HCL is to return
high-quality diagnostics messages primarily for human consumption, and so
their regular structure is only machine-processable in a general sense
where we treat all diagnostics as subject to the same processing.

A few times now we've ended up wanting to carry some additional optional
contextual information along with the diagnostic, for example so that a
more advanced diagnostics renderer might optionally annotate a diagnostic
with extra notes to help the reader debug.

We got pretty far with our previous extension of hcl.Diagnostic to include
the Expression and EvalContext fields, which allow an advanced diagnostic
renderer to offer hints about what values contributed to the expression
that failed, but some context is even more specific than that, or is
defined by the application itself and therefore not appropriate to model
directly here in HCL.

As a pragmatic compromise then, here we introduce one more field Extra
to hcl.Diagnostic, which comes with a documented convention of placing
into it situation-specific values that implement particular interfaces,
and therefore a diagnostics renderer or other consumer can potentially
"sniff" this field for particular interfaces it knows about and treat them
in a special way if present.

Since there is only one field here that might end up being asked to
capture multiple extra values as the call stack unwinds, there is also a
simple predefined protocol for "unwrapping" extra values in order to find
nested implementations within.


For callers that are prepared to require Go 1.18, the helper function
hcl.DiagnosticExtra provides a type-assertion-like mechanism for sniffing
for a particular interface type while automatically respecting the nesting
protocol. For the moment that function lives behind a build constraint
so that callers which are not yet ready to use Go 1.18 can continue to
use other parts of HCL, and can implement a non-generic equivalent of
this function within their own codebase if absolutely necessary.

As an initial example to demonstrate the idea I've also implemented some
extra information for error diagnostics returned from FunctionCallExpr,
which gives the name of the function being called and, if the diagnostic
is describing an error returned by the function itself, a direct reference
to the raw error value returned from the function call. I anticipate a
diagnostic renderer sniffing for hclsyntax.FunctionCallDiagExtra to see
if a particular diagnostic is related to a function call, and if so to
include additional context about the signature of that function in the
diagnostic messages (by correlating with the function in the EvalContext
functions table). For example:
    While calling: join(separator, list)

An example application-specific "extra value" could be for Terraform to
annotate diagnostics that relate to situations where an unknown value is
invalid, or where a "sensitive" value (a Terraform-specific value mark) is
invalid, so that the diagnostic renderer can avoid distracting users with
"red herring" commentary about unknown or sensitive values unless they
seem likely to be relevant to the error being printed.
  • Loading branch information
apparentlymart committed Jun 22, 2022
1 parent 986b881 commit 88ecd13
Show file tree
Hide file tree
Showing 5 changed files with 237 additions and 2 deletions.
43 changes: 43 additions & 0 deletions diagnostic.go
Expand Up @@ -63,6 +63,28 @@ type Diagnostic struct {
// case of colliding names.
Expression Expression
EvalContext *EvalContext

// Extra is an extension point for additional machine-readable information
// about this problem.
//
// Recipients of diagnostic objects may type-assert this value with
// specific interface types they know about to discover if any additional
// information is available that is interesting for their use-case.
//
// Extra is always considered to be optional extra information and so a
// diagnostic message should still always be fully described (from the
// perspective of a human who understands the language the messages are
// written in) by the other fields in case a particular recipient.
//
// Functions that return diagnostics with Extra populated should typically
// document that they place values implementing a particular interface,
// rather than a concrete type, and define that interface such that its
// methods can dynamically indicate a lack of support at runtime even
// if the interface happens to be statically available. An Extra
// type that wraps other Extra values should additionally implement
// interface DiagnosticExtraUnwrapper to return the value they are wrapping
// so that callers can access inner values to type-assert against.
Extra interface{}
}

// Diagnostics is a list of Diagnostic instances.
Expand Down Expand Up @@ -141,3 +163,24 @@ type DiagnosticWriter interface {
WriteDiagnostic(*Diagnostic) error
WriteDiagnostics(Diagnostics) error
}

// DiagnosticExtraUnwrapper is an interface implemented by values in the
// Extra field of Diagnostic when they are wrapping another "Extra" value that
// was generated downstream.
//
// Diagnostic recipients which want to examine "Extra" values to sniff for
// particular types of extra data can either type-assert this interface
// directly and repeatedly unwrap until they recieve nil, or can use the
// helper function DiagnosticExtra.
type DiagnosticExtraUnwrapper interface {
// If the reciever is wrapping another "diagnostic extra" value, returns
// that value. Otherwise returns nil to indicate dynamically that nothing
// is wrapped.
//
// The "nothing is wrapped" condition can be signalled either by this
// method returning nil or by a type not implementing this interface at all.
//
// Implementers should never create unwrap "cycles" where a nested extra
// value returns a value that was also wrapping it.
UnwrapDiagnosticExtra() interface{}
}
39 changes: 39 additions & 0 deletions diagnostic_typeparams.go
@@ -0,0 +1,39 @@
//go:build go1.18
// +build go1.18

package hcl

// This file contains additional diagnostics-related symbols that use the
// Go 1.18 type parameters syntax and would therefore be incompatible with
// Go 1.17 and earlier.

// DiagnosticExtra attempts to retrieve an "extra value" of type T from the
// given diagnostic, if either the diag.Extra field directly contains a value
// of that type or the value implements DiagnosticExtraUnwrapper and directly
// or indirectly returns a value of that type.
//
// Type T should typically be an interface type, so that code which generates
// diagnostics can potentially return different implementations of the same
// interface dynamically as needed.
//
// If a value of type T is found, returns that value and true to indicate
// success. Otherwise, returns the zero value of T and false to indicate
// failure.
func DiagnosticExtra[T any](diag *Diagnostic) (T, bool) {
extra := diag.Extra
var zero T

for {
if ret, ok := extra.(T); ok {
return ret, true
}

if unwrap, ok := extra.(DiagnosticExtraUnwrapper); ok {
// If our "extra" implements DiagnosticExtraUnwrapper then we'll
// unwrap one level and try this again.
extra = unwrap.UnwrapDiagnosticExtra()
} else {
return zero, false
}
}
}
2 changes: 1 addition & 1 deletion go.mod
@@ -1,6 +1,6 @@
module github.com/hashicorp/hcl/v2

go 1.17
go 1.18

require (
github.com/agext/levenshtein v1.2.1
Expand Down
53 changes: 52 additions & 1 deletion hclsyntax/expression.go
Expand Up @@ -26,7 +26,7 @@ type Expression interface {
}

// Assert that Expression implements hcl.Expression
var assertExprImplExpr hcl.Expression = Expression(nil)
var _ hcl.Expression = Expression(nil)

// ParenthesesExpr represents an expression written in grouping
// parentheses.
Expand Down Expand Up @@ -270,6 +270,10 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
}
}

diagExtra := functionCallDiagExtra{
calledFunctionName: e.Name,
}

params := f.Params()
varParam := f.VarParam()

Expand Down Expand Up @@ -297,6 +301,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: expandExpr,
EvalContext: ctx,
Extra: &diagExtra,
})
return cty.DynamicVal, diags
}
Expand All @@ -311,6 +316,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: expandExpr,
EvalContext: ctx,
Extra: &diagExtra,
})
return cty.DynamicVal, diags
}
Expand Down Expand Up @@ -342,6 +348,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: expandExpr,
EvalContext: ctx,
Extra: &diagExtra,
})
return cty.DynamicVal, diags
}
Expand All @@ -365,6 +372,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: e,
EvalContext: ctx,
Extra: &diagExtra,
},
}
}
Expand All @@ -382,6 +390,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: e,
EvalContext: ctx,
Extra: &diagExtra,
},
}
}
Expand Down Expand Up @@ -426,6 +435,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: argExpr,
EvalContext: ctx,
Extra: &diagExtra,
})
}
}
Expand All @@ -442,6 +452,10 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti

resultVal, err := f.Call(argVals)
if err != nil {
// For errors in the underlying call itself we also return the raw
// call error via an extra method on our "diagnostic extra" value.
diagExtra.functionCallError = err

switch terr := err.(type) {
case function.ArgError:
i := terr.Index
Expand Down Expand Up @@ -479,6 +493,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Subject: e.Range().Ptr(),
Expression: e,
EvalContext: ctx,
Extra: &diagExtra,
})
default:
// This is the most degenerate case of all, where the
Expand All @@ -497,6 +512,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: e,
EvalContext: ctx,
Extra: &diagExtra,
})
}
} else {
Expand All @@ -515,6 +531,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: argExpr,
EvalContext: ctx,
Extra: &diagExtra,
})
}

Expand All @@ -530,6 +547,7 @@ func (e *FunctionCallExpr) Value(ctx *hcl.EvalContext) (cty.Value, hcl.Diagnosti
Context: e.Range().Ptr(),
Expression: e,
EvalContext: ctx,
Extra: &diagExtra,
})
}

Expand Down Expand Up @@ -562,6 +580,39 @@ func (e *FunctionCallExpr) ExprCall() *hcl.StaticCall {
return ret
}

// FunctionCallDiagExtra is an interface implemented by the value in the "Extra"
// field of some diagnostics returned by FunctionCallExpr.Value, giving
// cooperating callers access to some machine-readable information about the
// call that a diagnostic relates to.
type FunctionCallDiagExtra interface {
// CalledFunctionName returns the name of the function being called at
// the time the diagnostic was generated, if any. Returns an empty string
// if there is no known called function.
CalledFunctionName() string

// FunctionCallError returns the error value returned by the implementation
// of the function being called, if any. Returns nil if the diagnostic was
// not returned in response to a call error.
//
// Some errors related to calling functions are generated by HCL itself
// rather than by the underlying function, in which case this method
// will return nil.
FunctionCallError() error
}

type functionCallDiagExtra struct {
calledFunctionName string
functionCallError error
}

func (e *functionCallDiagExtra) CalledFunctionName() string {
return e.calledFunctionName
}

func (e *functionCallDiagExtra) FunctionCallError() error {
return e.functionCallError
}

type ConditionalExpr struct {
Condition Expression
TrueResult Expression
Expand Down
102 changes: 102 additions & 0 deletions hclsyntax/expression_typeparams_test.go
@@ -0,0 +1,102 @@
//go:build go1.18
// +build go1.18

package hclsyntax

import (
"fmt"
"testing"

"github.com/hashicorp/hcl/v2"
"github.com/zclconf/go-cty/cty"
"github.com/zclconf/go-cty/cty/function"
)

// This file contains some additional tests that only make sense when using
// a Go compiler which supports type parameters (Go 1.18 or later).

func TestExpressionDiagnosticExtra(t *testing.T) {
tests := []struct {
input string
ctx *hcl.EvalContext
assert func(t *testing.T, diags hcl.Diagnostics)
}{
// Error messages describing inconsistent result types for conditional expressions.
{
"boop()",
&hcl.EvalContext{
Functions: map[string]function.Function{
"boop": function.New(&function.Spec{
Type: function.StaticReturnType(cty.String),
Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) {
return cty.DynamicVal, fmt.Errorf("the expected error")
},
}),
},
},
func(t *testing.T, diags hcl.Diagnostics) {
try := func(diags hcl.Diagnostics) {
t.Helper()
for _, diag := range diags {
extra, ok := hcl.DiagnosticExtra[FunctionCallDiagExtra](diag)
if !ok {
continue
}

if got, want := extra.CalledFunctionName(), "boop"; got != want {
t.Errorf("wrong called function name %q; want %q", got, want)
}
err := extra.FunctionCallError()
if err == nil {
t.Fatal("FunctionCallError returned nil")
}
if got, want := err.Error(), "the expected error"; got != want {
t.Errorf("wrong error message\ngot: %q\nwant: %q", got, want)
}

return
}
t.Fatalf("None of the returned diagnostics implement FunctionCallDiagError\n%s", diags.Error())
}

t.Run("unwrapped", func(t *testing.T) {
try(diags)
})

// It should also work if we wrap up the "extras" in wrapper types.
for _, diag := range diags {
diag.Extra = diagnosticExtraWrapper{diag.Extra}
}
t.Run("wrapped", func(t *testing.T) {
try(diags)
})
},
},
}

for _, test := range tests {
t.Run(test.input, func(t *testing.T) {
var diags hcl.Diagnostics
expr, parseDiags := ParseExpression([]byte(test.input), "", hcl.Pos{Line: 1, Column: 1, Byte: 0})
diags = append(diags, parseDiags...)
_, valDiags := expr.Value(test.ctx)
diags = append(diags, valDiags...)

if !diags.HasErrors() {
t.Fatal("unexpected success")
}

test.assert(t, diags)
})
}
}

type diagnosticExtraWrapper struct {
wrapped interface{}
}

var _ hcl.DiagnosticExtraUnwrapper = diagnosticExtraWrapper{}

func (w diagnosticExtraWrapper) UnwrapDiagnosticExtra() interface{} {
return w.wrapped
}

0 comments on commit 88ecd13

Please sign in to comment.