configs: quoted keywords/references are warnings, not errors

In our new loader we are changing certain values in configuration to be
naked keywords or references rather than quoted strings as before. Since
many of these have been shown in books, tutorials, and our own
documentation we will make the old forms generate deprecation warnings
rather than errors so that newcomers starting from older documentation
can be eased into the new syntax, rather than getting blocked.

This will also avoid creating a hard compatibility wall for reusable
modules that are already published, allowing them to still be used in
spite of these warnings and then fixed when the maintainer is able.

configs/compat_shim.go

@@ -0,0 +1,81 @@
+package configs
+
+import (
+	"github.com/hashicorp/hcl2/hcl"
+	"github.com/hashicorp/hcl2/hcl/hclsyntax"
+)
+
+// -------------------------------------------------------------------------
+// Functions in this file are compatibility shims intended to ease conversion
+// from the old configuration loader. Any use of these functions that makes
+// a change should generate a deprecation warning explaining to the user how
+// to update their code for new patterns.
+//
+// Shims are particularly important for any patterns that have been widely
+// documented in books, tutorials, etc. Users will still be starting from
+// these examples and we want to help them adopt the latest patterns rather
+// than leave them stranded.
+// -------------------------------------------------------------------------
+
+// shimTraversalInString takes any arbitrary expression and checks if it is
+// a quoted string in the native syntax. If it _is_, then it is parsed as a
+// traversal and re-wrapped into a synthetic traversal expression and a
+// warning is generated. Otherwise, the given expression is just returned
+// verbatim.
+//
+// This function has no effect on expressions from the JSON syntax, since
+// traversals in strings are the required pattern in that syntax.
+//
+// If wantKeyword is set, the generated warning diagnostic will talk about
+// keywords rather than references. The behavior is otherwise unchanged, and
+// the caller remains responsible for checking that the result is indeed
+// a keyword, e.g. using hcl.ExprAsKeyword.
+func shimTraversalInString(expr hcl.Expression, wantKeyword bool) (hcl.Expression, hcl.Diagnostics) {
+	if !exprIsNativeQuotedString(expr) {
+		return expr, nil
+	}
+
+	strVal, diags := expr.Value(nil)
+	if diags.HasErrors() || strVal.IsNull() || !strVal.IsKnown() {
+		// Since we're not even able to attempt a shim here, we'll discard
+		// the diagnostics we saw so far and let the caller's own error
+		// handling take care of reporting the invalid expression.
+		return expr, nil
+	}
+
+	// The position handling here isn't _quite_ right because it won't
+	// take into account any escape sequences in the literal string, but
+	// it should be close enough for any error reporting to make sense.
+	srcRange := expr.Range()
+	startPos := srcRange.Start // copy
+	startPos.Column++          // skip initial quote
+	startPos.Byte++            // skip initial quote
+
+	traversal, tDiags := hclsyntax.ParseTraversalAbs(
+		[]byte(strVal.AsString()),
+		srcRange.Filename,
+		startPos,
+	)
+	diags = append(diags, tDiags...)
+
+	if wantKeyword {
+		diags = append(diags, &hcl.Diagnostic{
+			Severity: hcl.DiagWarning,
+			Summary:  "Quoted keywords are deprecated",
+			Detail:   "In this context, keywords are expected literally rather than in quotes. Previous versions of Terraform required quotes, but that usage is now deprecated. Remove the quotes surrounding this keyword to silence this warning.",
+			Subject:  &srcRange,
+		})
+	} else {
+		diags = append(diags, &hcl.Diagnostic{
+			Severity: hcl.DiagWarning,
+			Summary:  "Quoted references are deprecated",
+			Detail:   "In this context, references are expected literally rather than in quotes. Previous versions of Terraform required quotes, but that usage is now deprecated. Remove the quotes surrounding this reference to silence this warning.",
+			Subject:  &srcRange,
+		})
+	}
+
+	return &hclsyntax.ScopeTraversalExpr{
+		Traversal: traversal,
+		SrcRange:  srcRange,
+	}, diags
+}

configs/depends_on.go

@@ -1,8 +1,6 @@
 package configs
 
 import (
-	"fmt"
-
 	"github.com/hashicorp/hcl2/hcl"
 )
 
@@ -11,20 +9,8 @@ func decodeDependsOn(attr *hcl.Attribute) ([]hcl.Traversal, hcl.Diagnostics) {
 	exprs, diags := hcl.ExprList(attr.Expr)
 
 	for _, expr := range exprs {
-		// A dependency reference was given as a string literal in the legacy
-		// configuration language and there are lots of examples out there
-		// showing that usage, so we'll sniff for that situation here and
-		// produce a specialized error message for it to help users find
-		// the new correct form.
-		if exprIsNativeQuotedString(expr) {
-			diags = append(diags, &hcl.Diagnostic{
-				Severity: hcl.DiagError,
-				Summary:  "Invalid explicit dependency reference",
-				Detail:   fmt.Sprintf("%s elements must not be given in quotes.", attr.Name),
-				Subject:  attr.Expr.Range().Ptr(),
-			})
-			continue
-		}
+		expr, shimDiags := shimTraversalInString(expr, false)
+		diags = append(diags, shimDiags...)
 
 		traversal, travDiags := hcl.AbsTraversalForExpr(expr)
 		diags = append(diags, travDiags...)

configs/named_values.go

@@ -68,7 +68,10 @@ func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) {
 	}
 
 	if attr, exists := content.Attributes["type"]; exists {
-		switch hcl.ExprAsKeyword(attr.Expr) {
+		expr, shimDiags := shimTraversalInString(attr.Expr, true)
+		diags = append(diags, shimDiags...)
+
+		switch hcl.ExprAsKeyword(expr) {
 		case "string":
 			v.TypeHint = TypeHintString
 		case "list":
@@ -76,25 +79,12 @@ func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) {
 		case "map":
 			v.TypeHint = TypeHintMap
 		default:
-			// In our legacy configuration format these keywords would've been
-			// provided as quoted strings, so we'll generate a special error
-			// message for that to help those who find outdated examples and
-			// would otherwise be confused.
-			if exprIsNativeQuotedString(attr.Expr) {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid variable type hint",
-					Detail:   "The type hint keyword must not be given in quotes.",
-					Subject:  attr.Expr.Range().Ptr(),
-				})
-			} else {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid variable type hint",
-					Detail:   "The type argument requires one of the following keywords: string, list, or map.",
-					Subject:  attr.Expr.Range().Ptr(),
-				})
-			}
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid variable type hint",
+				Detail:   "The type argument requires one of the following keywords: string, list, or map.",
+				Subject:  expr.Range().Ptr(),
+			})
 		}
 	}
 

configs/parser_config_dir_test.go

@@ -60,8 +60,8 @@ func TestParserLoadConfigDirSuccess(t *testing.T) {
 			})
 
 			_, diags := parser.LoadConfigDir("mod")
-			if len(diags) != 0 {
-				t.Errorf("unexpected diagnostics")
+			if diags.HasErrors() {
+				t.Errorf("unexpected error diagnostics")
 				for _, diag := range diags {
 					t.Logf("- %s", diag)
 				}

configs/parser_config_test.go

@@ -34,8 +34,8 @@ func TestParserLoadConfigFileSuccess(t *testing.T) {
 			})
 
 			_, diags := parser.LoadConfigFile(name)
-			if len(diags) != 0 {
-				t.Errorf("unexpected diagnostics")
+			if diags.HasErrors() {
+				t.Errorf("unexpected error diagnostics")
 				for _, diag := range diags {
 					t.Logf("- %s", diag)
 				}
@@ -85,38 +85,65 @@ func TestParserLoadConfigFileFailure(t *testing.T) {
 // file produces the expected diagnostic summary.
 func TestParserLoadConfigFileFailureMessages(t *testing.T) {
 	tests := []struct {
-		Filename  string
-		WantError string
+		Filename     string
+		WantSeverity hcl.DiagnosticSeverity
+		WantDiag     string
 	}{
 		{
-			"data-resource-lifecycle.tf",
+			"invalid-files/data-resource-lifecycle.tf",
+			hcl.DiagError,
 			"Unsupported lifecycle block",
 		},
 		{
-			"variable-type-unknown.tf",
+			"invalid-files/variable-type-unknown.tf",
+			hcl.DiagError,
 			"Invalid variable type hint",
 		},
 		{
-			"variable-type-quoted.tf",
-			"Invalid variable type hint",
+			"valid-files/variable-type-quoted.tf",
+			hcl.DiagWarning,
+			"Quoted keywords are deprecated",
 		},
 		{
-			"unexpected-attr.tf",
+			"invalid-files/unexpected-attr.tf",
+			hcl.DiagError,
 			"Unsupported attribute",
 		},
 		{
-			"unexpected-block.tf",
+			"invalid-files/unexpected-block.tf",
+			hcl.DiagError,
 			"Unsupported block type",
 		},
 		{
-			"resource-lifecycle-badbool.tf",
+			"invalid-files/resource-lifecycle-badbool.tf",
+			hcl.DiagError,
 			"Unsuitable value type",
 		},
+		{
+			"valid-files/resources-dependson-quoted.tf",
+			hcl.DiagWarning,
+			"Quoted references are deprecated",
+		},
+		{
+			"valid-files/resources-ignorechanges-quoted.tf",
+			hcl.DiagWarning,
+			"Quoted references are deprecated",
+		},
+		{
+			"valid-files/resources-provisioner-when-quoted.tf",
+			hcl.DiagWarning,
+			"Quoted keywords are deprecated",
+		},
+		{
+			"valid-files/resources-provisioner-onfailure-quoted.tf",
+			hcl.DiagWarning,
+			"Quoted keywords are deprecated",
+		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.Filename, func(t *testing.T) {
-			src, err := ioutil.ReadFile(filepath.Join("test-fixtures/invalid-files", test.Filename))
+			src, err := ioutil.ReadFile(filepath.Join("test-fixtures", test.Filename))
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -133,11 +160,11 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) {
 				}
 				return
 			}
-			if diags[0].Severity != hcl.DiagError {
-				t.Errorf("Wrong diagnostic severity %s; want %s", diags[0].Severity, hcl.DiagError)
+			if diags[0].Severity != test.WantSeverity {
+				t.Errorf("Wrong diagnostic severity %#v; want %#v", diags[0].Severity, test.WantSeverity)
 			}
-			if diags[0].Summary != test.WantError {
-				t.Errorf("Wrong diagnostic summary\ngot:  %s\nwant: %s", diags[0].Summary, test.WantError)
+			if diags[0].Summary != test.WantDiag {
+				t.Errorf("Wrong diagnostic summary\ngot:  %s\nwant: %s", diags[0].Summary, test.WantDiag)
 			}
 		})
 	}

configs/provisioner.go

@@ -33,52 +33,40 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) {
 	pv.Config = config
 
 	if attr, exists := content.Attributes["when"]; exists {
-		switch hcl.ExprAsKeyword(attr.Expr) {
+		expr, shimDiags := shimTraversalInString(attr.Expr, true)
+		diags = append(diags, shimDiags...)
+
+		switch hcl.ExprAsKeyword(expr) {
 		case "create":
 			pv.When = ProvisionerWhenCreate
 		case "destroy":
 			pv.When = ProvisionerWhenDestroy
 		default:
-			if exprIsNativeQuotedString(attr.Expr) {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid \"when\" keyword",
-					Detail:   "The \"when\" argument keyword must not be given in quotes.",
-					Subject:  attr.Expr.Range().Ptr(),
-				})
-			} else {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid \"when\" keyword",
-					Detail:   "The \"when\" argument requires one of the following keywords: create or destroy.",
-					Subject:  attr.Expr.Range().Ptr(),
-				})
-			}
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid \"when\" keyword",
+				Detail:   "The \"when\" argument requires one of the following keywords: create or destroy.",
+				Subject:  expr.Range().Ptr(),
+			})
 		}
 	}
 
 	if attr, exists := content.Attributes["on_failure"]; exists {
-		switch hcl.ExprAsKeyword(attr.Expr) {
+		expr, shimDiags := shimTraversalInString(attr.Expr, true)
+		diags = append(diags, shimDiags...)
+
+		switch hcl.ExprAsKeyword(expr) {
 		case "continue":
 			pv.OnFailure = ProvisionerOnFailureContinue
 		case "fail":
 			pv.OnFailure = ProvisionerOnFailureFail
 		default:
-			if exprIsNativeQuotedString(attr.Expr) {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid \"on_failure\" keyword",
-					Detail:   "The \"on_failure\" argument keyword must not be given in quotes.",
-					Subject:  attr.Expr.Range().Ptr(),
-				})
-			} else {
-				diags = append(diags, &hcl.Diagnostic{
-					Severity: hcl.DiagError,
-					Summary:  "Invalid \"on_failure\" keyword",
-					Detail:   "The \"on_failure\" argument requires one of the following keywords: continue or fail.",
-					Subject:  attr.Expr.Range().Ptr(),
-				})
-			}
+			diags = append(diags, &hcl.Diagnostic{
+				Severity: hcl.DiagError,
+				Summary:  "Invalid \"on_failure\" keyword",
+				Detail:   "The \"on_failure\" argument requires one of the following keywords: continue or fail.",
+				Subject:  attr.Expr.Range().Ptr(),
+			})
 		}
 	}
 

configs/resource.go

@@ -122,6 +122,9 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) {
 				diags = append(diags, listDiags...)
 
 				for _, expr := range exprs {
+					expr, shimDiags := shimTraversalInString(expr, false)
+					diags = append(diags, shimDiags...)
+
 					traversal, travDiags := hcl.RelTraversalForExpr(expr)
 					diags = append(diags, travDiags...)
 					if len(traversal) != 0 {
@@ -257,7 +260,11 @@ type ProviderConfigRef struct {
 
 func decodeProviderConfigRef(attr *hcl.Attribute) (*ProviderConfigRef, hcl.Diagnostics) {
 	var diags hcl.Diagnostics
-	traversal, travDiags := hcl.AbsTraversalForExpr(attr.Expr)
+
+	expr, shimDiags := shimTraversalInString(attr.Expr, false)
+	diags = append(diags, shimDiags...)
+
+	traversal, travDiags := hcl.AbsTraversalForExpr(expr)
 
 	// AbsTraversalForExpr produces only generic errors, so we'll discard
 	// the errors given and produce our own with extra context. If we didn't
@@ -277,7 +284,7 @@ func decodeProviderConfigRef(attr *hcl.Attribute) (*ProviderConfigRef, hcl.Diagn
 				Severity: hcl.DiagError,
 				Summary:  "Invalid provider configuration reference",
 				Detail:   "A provider configuration reference must not be given in quotes.",
-				Subject:  attr.Expr.Range().Ptr(),
+				Subject:  expr.Range().Ptr(),
 			})
 			return nil, diags
 		}
@@ -286,7 +293,7 @@ func decodeProviderConfigRef(attr *hcl.Attribute) (*ProviderConfigRef, hcl.Diagn
 			Severity: hcl.DiagError,
 			Summary:  "Invalid provider configuration reference",
 			Detail:   fmt.Sprintf("The %s argument requires a provider type name, optionally followed by a period and then a configuration alias.", attr.Name),
-			Subject:  attr.Expr.Range().Ptr(),
+			Subject:  expr.Range().Ptr(),
 		})
 		return nil, diags
 	}

configs/test-fixtures/valid-files/resources-dependson-quoted.tf

@@ -0,0 +1,8 @@
+resource "aws_security_group" "firewall" {
+}
+
+resource "aws_instance" "web" {
+  depends_on = [
+    "aws_security_group.firewall",
+  ]
+}

configs/test-fixtures/valid-files/resources-ignorechanges-quoted.tf

@@ -0,0 +1,7 @@
+resource "aws_instance" "web" {
+  lifecycle {
+    ignore_changes = [
+      "ami",
+    ]
+  }
+}

configs/test-fixtures/valid-files/resources-provisioner-onfailure-quoted.tf

@@ -0,0 +1,6 @@
+resource "aws_security_group" "firewall" {
+  provisioner "local-exec" {
+    command    = "echo hello"
+    on_failure = "continue"
+  }
+}

configs/test-fixtures/valid-files/resources-provisioner-when-quoted.tf

@@ -0,0 +1,6 @@
+resource "aws_security_group" "firewall" {
+  provisioner "local-exec" {
+    command = "echo hello"
+    when    = "destroy"
+  }
+}