From 6513c0138f22b5e02d0af24d183c6c1c7b37ccbc Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 21 Jun 2022 12:05:38 -0700 Subject: [PATCH] hcl: Allow individual diagnostics to carry extra information 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. --- diagnostic.go | 43 ++++++++++ diagnostic_typeparams.go | 39 +++++++++ go.mod | 2 +- hclsyntax/expression.go | 53 +++++++++++- hclsyntax/expression_typeparams_test.go | 102 ++++++++++++++++++++++++ 5 files changed, 237 insertions(+), 2 deletions(-) create mode 100644 diagnostic_typeparams.go create mode 100644 hclsyntax/expression_typeparams_test.go diff --git a/diagnostic.go b/diagnostic.go index c80535b7..bcf4eb39 100644 --- a/diagnostic.go +++ b/diagnostic.go @@ -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. @@ -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{} +} diff --git a/diagnostic_typeparams.go b/diagnostic_typeparams.go new file mode 100644 index 00000000..6994e233 --- /dev/null +++ b/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 + } + } +} diff --git a/go.mod b/go.mod index cf503a04..8f7c79fc 100644 --- a/go.mod +++ b/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 diff --git a/hclsyntax/expression.go b/hclsyntax/expression.go index 2706998f..358fd5d5 100644 --- a/hclsyntax/expression.go +++ b/hclsyntax/expression.go @@ -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. @@ -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() @@ -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 } @@ -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 } @@ -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 } @@ -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, }, } } @@ -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, }, } } @@ -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, }) } } @@ -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 @@ -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 @@ -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 { @@ -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, }) } @@ -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, }) } @@ -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 diff --git a/hclsyntax/expression_typeparams_test.go b/hclsyntax/expression_typeparams_test.go new file mode 100644 index 00000000..f88e6a34 --- /dev/null +++ b/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 +}