From eac85b506bb0f5143ae7000babd53bcf2b973a58 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 17 Nov 2020 15:30:59 -0800 Subject: [PATCH] configs: Specialized warning for single-interpolation object keys We have an existing warning message to encourage moving away from the old 0.11-and-earlier style of redundantly wrapping standalone expressions in templates, but due to the special rules for object keys the warning message was giving misleading advice in that context: a user following the advice as given would then encounter an error about the object key being ambiguous. To account for that, this introduces a special alternative version of the warning just for that particular position, directing the user to replace the template interpolation markers with parenthesis instead. That will then get the same result as the former interpolation sequence, rather than producing the ambiguity error. --- configs/compat_shim.go | 95 ++++++++++++++++++++++++++++++------- configs/compat_shim_test.go | 61 ++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 18 deletions(-) create mode 100644 configs/compat_shim_test.go diff --git a/configs/compat_shim.go b/configs/compat_shim.go index 4c6c1b75e..47f512b50 100644 --- a/configs/compat_shim.go +++ b/configs/compat_shim.go @@ -147,22 +147,81 @@ func warnForDeprecatedInterpolationsInExpr(expr hcl.Expression) hcl.Diagnostics return nil } - return hclsyntax.VisitAll(node, func(n hclsyntax.Node) hcl.Diagnostics { - e, ok := n.(*hclsyntax.TemplateWrapExpr) - if !ok { - // We're only interested in TemplateWrapExpr, because that's how - // the HCL native syntax parser represents the case of a template - // that consists entirely of a single interpolation expression, which - // is therefore subject to the special case of passing through the - // inner value without conversion to string. - return nil - } - - return hcl.Diagnostics{&hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Interpolation-only expressions are deprecated", - Detail: "Terraform 0.11 and earlier required all non-constant expressions to be provided via interpolation syntax, but this pattern is now deprecated. To silence this warning, remove the \"${ sequence from the start and the }\" sequence from the end of this expression, leaving just the inner expression.\n\nTemplate interpolation syntax is still used to construct strings from expressions when the template includes multiple interpolation sequences or a mixture of literal strings and interpolations. This deprecation applies only to templates that consist entirely of a single interpolation sequence.", - Subject: e.Range().Ptr(), - }} - }) + walker := warnForDeprecatedInterpolationsWalker{ + // create some capacity so that we can deal with simple expressions + // without any further allocation during our walk. + contextStack: make([]warnForDeprecatedInterpolationsContext, 0, 16), + } + return hclsyntax.Walk(node, &walker) +} + +// warnForDeprecatedInterpolationsWalker is an implementation of +// hclsyntax.Walker that we use to generate deprecation warnings for template +// expressions that consist entirely of a single interpolation directive. +// That's always redundant in Terraform v0.12 and later, but tends to show up +// when people work from examples written for Terraform v0.11 or earlier. +type warnForDeprecatedInterpolationsWalker struct { + contextStack []warnForDeprecatedInterpolationsContext +} + +var _ hclsyntax.Walker = (*warnForDeprecatedInterpolationsWalker)(nil) + +type warnForDeprecatedInterpolationsContext int + +const ( + warnForDeprecatedInterpolationsNormal warnForDeprecatedInterpolationsContext = 0 + warnForDeprecatedInterpolationsObjKey warnForDeprecatedInterpolationsContext = 1 +) + +func (w *warnForDeprecatedInterpolationsWalker) Enter(node hclsyntax.Node) hcl.Diagnostics { + var diags hcl.Diagnostics + + context := warnForDeprecatedInterpolationsNormal + switch node := node.(type) { + case *hclsyntax.ObjectConsKeyExpr: + context = warnForDeprecatedInterpolationsObjKey + case *hclsyntax.TemplateWrapExpr: + // hclsyntax.TemplateWrapExpr is a special node type used by HCL only + // for the situation where a template is just a single interpolation, + // so we don't need to do anything further to distinguish that + // situation. ("normal" templates are *hclsyntax.TemplateExpr.) + + const summary = "Interpolation-only expressions are deprecated" + switch w.currentContext() { + case warnForDeprecatedInterpolationsObjKey: + // This case requires a different resolution in order to retain + // the same meaning, so we have a different detail message for + // it. + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: summary, + Detail: "Terraform 0.11 and earlier required all non-constant expressions to be provided via interpolation syntax, but this pattern is now deprecated.\n\nTo silence this warning, replace the \"${ opening sequence and the }\" closing sequence with opening and closing parentheses respectively. Parentheses are needed here to mark this as an expression to be evaluated, rather than as a literal string key.\n\nTemplate interpolation syntax is still used to construct strings from expressions when the template includes multiple interpolation sequences or a mixture of literal strings and interpolations. This deprecation applies only to templates that consist entirely of a single interpolation sequence.", + Subject: node.Range().Ptr(), + }) + default: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: summary, + Detail: "Terraform 0.11 and earlier required all non-constant expressions to be provided via interpolation syntax, but this pattern is now deprecated. To silence this warning, remove the \"${ sequence from the start and the }\" sequence from the end of this expression, leaving just the inner expression.\n\nTemplate interpolation syntax is still used to construct strings from expressions when the template includes multiple interpolation sequences or a mixture of literal strings and interpolations. This deprecation applies only to templates that consist entirely of a single interpolation sequence.", + Subject: node.Range().Ptr(), + }) + } + } + + // Note the context of the current node for when we potentially visit + // child nodes. + w.contextStack = append(w.contextStack, context) + return diags +} + +func (w *warnForDeprecatedInterpolationsWalker) Exit(node hclsyntax.Node) hcl.Diagnostics { + w.contextStack = w.contextStack[:len(w.contextStack)-1] + return nil +} + +func (w *warnForDeprecatedInterpolationsWalker) currentContext() warnForDeprecatedInterpolationsContext { + if len(w.contextStack) == 0 { + return warnForDeprecatedInterpolationsNormal + } + return w.contextStack[len(w.contextStack)-1] } diff --git a/configs/compat_shim_test.go b/configs/compat_shim_test.go new file mode 100644 index 000000000..f6068bce9 --- /dev/null +++ b/configs/compat_shim_test.go @@ -0,0 +1,61 @@ +package configs + +import ( + "strings" + "testing" + + "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/hclsyntax" +) + +func TestWarnForDeprecatedInterpolationsInExpr(t *testing.T) { + tests := []struct { + Expr string + WantSubstr string + }{ + { + `"${foo}"`, + "leaving just the inner expression", + }, + { + `{"${foo}" = 1}`, + // Special message for object key expressions, because just + // removing the interpolation markers would change the meaning + // in that context. + "opening and closing parentheses respectively", + }, + { + `{upper("${foo}") = 1}`, + // But no special message if the template is just descended from an + // object key, because the special interpretation applies only to + // a naked reference in te object key position. + "leaving just the inner expression", + }, + } + + for _, test := range tests { + t.Run(test.Expr, func(t *testing.T) { + expr, diags := hclsyntax.ParseExpression([]byte(test.Expr), "", hcl.InitialPos) + if diags.HasErrors() { + t.Fatalf("parse error: %s", diags.Error()) + } + + diags = warnForDeprecatedInterpolationsInExpr(expr) + if !diagWarningsContainSubstring(diags, test.WantSubstr) { + t.Errorf("wrong warning message\nwant detail substring: %s\ngot: %s", test.WantSubstr, diags.Error()) + } + }) + } +} + +func diagWarningsContainSubstring(diags hcl.Diagnostics, want string) bool { + for _, diag := range diags { + if diag.Severity != hcl.DiagWarning { + continue + } + if strings.Contains(diag.Detail, want) { + return true + } + } + return false +}