configs: Emit warnings for deprecated quoted references/keywords

Terraform 0.12.0 removed the need for putting references and keywords
in quotes, but we disabled the deprecation warnings for the initial
release in order to avoid creating noise for folks who were intentionally
attempting to maintain modules that were cross-compatible with both
Terraform 0.11 and Terraform 0.12.

However, with Terraform 0.12 now more widely used, the lack of these
warnings seems to be causing newcomers to copy the quoted versions from
existing examples on the internet, which is perpetuating the old and
confusing quoted form in newer configurations.

In preparation for phasing out these deprecated forms altogether in a
future major release, and for the shorter-term benefit of giving better
feedback to newcomers when they are learning from outdated examples, we'll
now re-enable those deprecation warnings, and be explicit that the old
forms are intended for removal in a future release.

In order to properly test this, we establish a new set of test
configurations that explicitly mark which warnings they are expecting and
verify that they do indeed produce those expected warnings. We also
verify that the "success" tests do _not_ produce warnings, while removing
the ones that were previously written to succeed but have their warnings
ignored.

configs/compat_shim.go

@@ -69,28 +69,21 @@ func shimTraversalInString(expr hcl.Expression, wantKeyword bool) (hcl.Expressio
 	)
 	diags = append(diags, tDiags...)
 
-	// For initial release our deprecation warnings are disabled to allow
-	// a period where modules can be compatible with both old and new
-	// conventions.
-	// FIXME: Re-enable these deprecation warnings in a release prior to
-	// Terraform 0.13 and then remove the shims altogether for 0.13.
-	/*
-		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,
-			})
-		}
-	*/
+	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. Terraform 0.11 and earlier required quotes, but quoted keywords are now deprecated and will be removed in a future version of Terraform. 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. Terraform 0.11 and earlier required quotes, but quoted references are now deprecated and will be removed in a future version of Terraform. Remove the quotes surrounding this reference to silence this warning.",
+			Subject:  &srcRange,
+		})
+	}
 
 	return &hclsyntax.ScopeTraversalExpr{
 		Traversal: traversal,

configs/parser_config_test.go

@@ -1,10 +1,15 @@
 package configs
 
 import (
+	"bufio"
+	"bytes"
 	"io/ioutil"
 	"path/filepath"
+	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/hashicorp/hcl/v2"
 )
 
@@ -34,8 +39,8 @@ func TestParserLoadConfigFileSuccess(t *testing.T) {
 			})
 
 			_, diags := parser.LoadConfigFile(name)
-			if diags.HasErrors() {
-				t.Errorf("unexpected error diagnostics")
+			if len(diags) != 0 {
+				t.Errorf("unexpected diagnostics")
 				for _, diag := range diags {
 					t.Logf("- %s", diag)
 				}
@@ -124,16 +129,6 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) {
 			hcl.DiagError,
 			"Unsuitable value type",
 		},
-		{
-			"valid-files/resources-ignorechanges-all-legacy.tf",
-			hcl.DiagWarning,
-			"Deprecated ignore_changes wildcard",
-		},
-		{
-			"valid-files/resources-ignorechanges-all-legacy.tf.json",
-			hcl.DiagWarning,
-			"Deprecated ignore_changes wildcard",
-		},
 	}
 
 	for _, test := range tests {
@@ -164,3 +159,68 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) {
 		})
 	}
 }
+
+// TestParseLoadConfigFileWarning is a test that verifies files from
+// testdata/warning-files produce particular warnings.
+//
+// This test does not verify that reading these files produces the correct
+// file element contents in spite of those warnings. More detailed assertions
+// may be made on some subset of these configuration files in other tests.
+func TestParserLoadConfigFileWarning(t *testing.T) {
+	files, err := ioutil.ReadDir("testdata/warning-files")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, info := range files {
+		name := info.Name()
+		t.Run(name, func(t *testing.T) {
+			src, err := ioutil.ReadFile(filepath.Join("testdata/warning-files", name))
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			// First we'll scan the file to see what warnings are expected.
+			// That's declared inside the files themselves by using the
+			// string "WARNING: " somewhere on each line that is expected
+			// to produce a warning, followed by the expected warning summary
+			// text. A single-line comment (with #) is the main way to do that.
+			const marker = "WARNING: "
+			sc := bufio.NewScanner(bytes.NewReader(src))
+			wantWarnings := make(map[int]string)
+			lineNum := 1
+			for sc.Scan() {
+				lineText := sc.Text()
+				if idx := strings.Index(lineText, marker); idx != -1 {
+					summaryText := lineText[idx+len(marker):]
+					wantWarnings[lineNum] = summaryText
+				}
+				lineNum++
+			}
+
+			parser := testParser(map[string]string{
+				name: string(src),
+			})
+
+			_, diags := parser.LoadConfigFile(name)
+			if diags.HasErrors() {
+				t.Errorf("unexpected error diagnostics")
+				for _, diag := range diags {
+					t.Logf("- %s", diag)
+				}
+			}
+
+			gotWarnings := make(map[int]string)
+			for _, diag := range diags {
+				if diag.Severity != hcl.DiagWarning || diag.Subject == nil {
+					continue
+				}
+				gotWarnings[diag.Subject.Start.Line] = diag.Summary
+			}
+
+			if diff := cmp.Diff(wantWarnings, gotWarnings); diff != "" {
+				t.Errorf("wrong warnings\n%s", diff)
+			}
+		})
+	}
+}

configs/testdata/valid-files/resources-dependson-quoted.tf

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

configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf

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

configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf.json

@@ -1,11 +0,0 @@
-{
-  "resource": {
-    "aws_instance": {
-      "web": {
-        "lifecycle": {
-          "ignore_changes": ["*"]
-        }
-      }
-    }
-  }
-}

configs/testdata/valid-files/resources-ignorechanges-quoted.tf

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

configs/testdata/valid-files/resources-provisioner-onfailure-quoted.tf

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

configs/testdata/valid-files/resources-provisioner-when-quoted.tf

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

configs/testdata/warning-files/depends_on.tf

@@ -0,0 +1,6 @@
+resource "null_resource" "a" {
+}
+
+resource "null_resource" "b" {
+  depends_on = ["null_resource.a"] # WARNING: Quoted references are deprecated
+}

configs/testdata/warning-files/ignore_changes.tf

@@ -0,0 +1,11 @@
+resource "null_resource" "one" {
+  lifecycle {
+    ignore_changes = ["triggers"] # WARNING: Quoted references are deprecated
+  }
+}
+
+resource "null_resource" "all" {
+  lifecycle {
+    ignore_changes = ["*"] # WARNING: Deprecated ignore_changes wildcard
+  }
+}

configs/testdata/warning-files/provider_ref.tf

@@ -0,0 +1,7 @@
+provider "null" {
+  alias = "foo"
+}
+
+resource "null_resource" "test" {
+  provider = "null.foo" # WARNING: Quoted references are deprecated
+}

configs/testdata/warning-files/provisioner_keyword.tf

@@ -0,0 +1,6 @@
+resource "null_resource" "a" {
+  provisioner "local-exec" {
+    when       = "create" # WARNING: Quoted keywords are deprecated
+    on_failure = "fail"   # WARNING: Quoted keywords are deprecated
+  }
+}