repl: Improved value renderer for console outputs

Use a slightly modified value renderer from terraform-provider-testing
to display values in the console REPL, as well as outputs from the apply
and outputs subcommands.

Derived from code in this repository, MIT licensed:

https://github.com/apparentlymart/terraform-provider-testing

Note that this is technically a breaking change for the console
subcommand, which would previously error if the user attempted to render
an unknown value (such as an unset variable). This was marked as an
unintentional side effect, with the goal being the new behaviour of
rendering "(unknown)", which is why I changed the behaviour in this
commit.

command/apply.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/hashicorp/terraform/addrs"
 	"github.com/hashicorp/terraform/backend"
-	"github.com/hashicorp/terraform/configs/hcl2shim"
 	"github.com/hashicorp/terraform/repl"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/tfdiags"
@@ -345,17 +344,7 @@ func outputsAsString(state *states.State, modPath addrs.ModuleInstance, includeH
 				continue
 			}
 
-			// Our formatter still wants an old-style raw interface{} value, so
-			// for now we'll just shim it.
-			// FIXME: Port the formatter to work with cty.Value directly.
-			legacyVal := hcl2shim.ConfigValueFromHCL2(v.Value)
-			result, err := repl.FormatResult(legacyVal)
-			if err != nil {
-				// We can't really return errors from here, so we'll just have
-				// to stub this out. This shouldn't happen in practice anyway.
-				result = "<error during formatting>"
-			}
-
+			result := repl.FormatValue(v.Value, 0)
 			outputBuf.WriteString(fmt.Sprintf("%s = %s\n", k, result))
 		}
 	}

command/apply_test.go

@@ -1107,7 +1107,7 @@ func TestApply_sensitiveOutput(t *testing.T) {
 	}
 
 	output := ui.OutputWriter.String()
-	if !strings.Contains(output, "notsensitive = Hello world") {
+	if !strings.Contains(output, "notsensitive = \"Hello world\"") {
 		t.Fatalf("bad: output should contain 'notsensitive' output\n%s", output)
 	}
 	if !strings.Contains(output, "sensitive = <sensitive>") {

command/console_test.go

@@ -27,7 +27,7 @@ func TestConsole_basic(t *testing.T) {
 	defer testFixCwd(t, tmp, cwd)
 
 	p := testProvider()
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &ConsoleCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -73,7 +73,7 @@ func TestConsole_tfvars(t *testing.T) {
 		},
 	}
 
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &ConsoleCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -95,7 +95,7 @@ func TestConsole_tfvars(t *testing.T) {
 	}
 
 	actual := output.String()
-	if actual != "bar\n" {
+	if actual != "\"bar\"\n" {
 		t.Fatalf("bad: %q", actual)
 	}
 }
@@ -120,7 +120,7 @@ func TestConsole_unsetRequiredVars(t *testing.T) {
 			},
 		},
 	}
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 	c := &ConsoleCommand{
 		Meta: Meta{
 			testingOverrides: metaOverridesForProvider(p),
@@ -140,21 +140,12 @@ func TestConsole_unsetRequiredVars(t *testing.T) {
 	code := c.Run(args)
 	outCloser()
 
-	// Because we're running "terraform console" in piped input mode, we're
-	// expecting it to return a nonzero exit status here but the message
-	// must be the one indicating that it did attempt to evaluate var.foo and
-	// got an unknown value in return, rather than an error about var.foo
-	// not being set or a failure to prompt for it.
-	if code == 0 {
-		t.Fatalf("unexpected success\n%s", ui.OutputWriter.String())
+	if code != 0 {
+		t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String())
 	}
 
-	// The error message should be the one console produces when it encounters
-	// an unknown value.
-	got := ui.ErrorWriter.String()
-	want := `Error: Result depends on values that cannot be determined`
-	if !strings.Contains(got, want) {
-		t.Fatalf("wrong output\ngot:\n%s\n\nwant string containing %q", got, want)
+	if got, want := output.String(), "(known after apply)\n"; got != want {
+		t.Fatalf("unexpected output\n got: %q\nwant: %q", got, want)
 	}
 }
 
@@ -165,7 +156,7 @@ func TestConsole_modules(t *testing.T) {
 	defer testChdir(t, td)()
 
 	p := applyFixtureProvider()
-	ui := new(cli.MockUi)
+	ui := cli.NewMockUi()
 
 	c := &ConsoleCommand{
 		Meta: Meta{
@@ -175,8 +166,8 @@ func TestConsole_modules(t *testing.T) {
 	}
 
 	commands := map[string]string{
-		"module.child.myoutput\n":          "bar\n",
-		"module.count_child[0].myoutput\n": "bar\n",
+		"module.child.myoutput\n":          "\"bar\"\n",
+		"module.count_child[0].myoutput\n": "\"bar\"\n",
 		"local.foo\n":                      "3\n",
 	}
 

command/output.go

@@ -10,7 +10,6 @@ import (
 	ctyjson "github.com/zclconf/go-cty/cty/json"
 
 	"github.com/hashicorp/terraform/addrs"
-	"github.com/hashicorp/terraform/configs/hcl2shim"
 	"github.com/hashicorp/terraform/repl"
 	"github.com/hashicorp/terraform/states"
 	"github.com/hashicorp/terraform/tfdiags"
@@ -195,16 +194,7 @@ func (c *OutputCommand) Run(args []string) int {
 
 		c.Ui.Output(string(jsonOutput))
 	} else {
-		// Our formatter still wants an old-style raw interface{} value, so
-		// for now we'll just shim it.
-		// FIXME: Port the formatter to work with cty.Value directly.
-		legacyVal := hcl2shim.ConfigValueFromHCL2(v)
-		result, err := repl.FormatResult(legacyVal)
-		if err != nil {
-			diags = diags.Append(err)
-			c.showDiagnostics(diags)
-			return 1
-		}
+		result := repl.FormatValue(v, 0)
 		c.Ui.Output(result)
 	}
 

command/output_test.go

@@ -41,7 +41,7 @@ func TestOutput(t *testing.T) {
 	}
 
 	actual := strings.TrimSpace(ui.OutputWriter.String())
-	if actual != "bar" {
+	if actual != `"bar"` {
 		t.Fatalf("bad: %#v", actual)
 	}
 }
@@ -80,9 +80,19 @@ func TestOutput_nestedListAndMap(t *testing.T) {
 	}
 
 	actual := strings.TrimSpace(ui.OutputWriter.String())
-	expected := "foo = [\n  {\n    \"key\" = \"value\"\n    \"key2\" = \"value2\"\n  },\n  {\n    \"key\" = \"value\"\n  },\n]"
+	expected := strings.TrimSpace(`
+foo = tolist([
+  tomap({
+    "key" = "value"
+    "key2" = "value2"
+  }),
+  tomap({
+    "key" = "value"
+  }),
+])
+`)
 	if actual != expected {
-		t.Fatalf("wrong output\ngot:  %#v\nwant: %#v", actual, expected)
+		t.Fatalf("wrong output\ngot:  %s\nwant: %s", actual, expected)
 	}
 }
 
@@ -260,7 +270,7 @@ func TestOutput_blank(t *testing.T) {
 		t.Fatalf("bad: \n%s", ui.ErrorWriter.String())
 	}
 
-	expectedOutput := "foo = bar\nname = john-doe\n"
+	expectedOutput := "foo = \"bar\"\nname = \"john-doe\"\n"
 	output := ui.OutputWriter.String()
 	if output != expectedOutput {
 		t.Fatalf("wrong output\ngot:  %#v\nwant: %#v", output, expectedOutput)
@@ -393,7 +403,7 @@ func TestOutput_stateDefault(t *testing.T) {
 	}
 
 	actual := strings.TrimSpace(ui.OutputWriter.String())
-	if actual != "bar" {
+	if actual != `"bar"` {
 		t.Fatalf("bad: %#v", actual)
 	}
 }

repl/format.go

@@ -1,116 +1,118 @@
 package repl
 
 import (
-	"bufio"
-	"bytes"
 	"fmt"
-	"sort"
 	"strconv"
 	"strings"
+
+	"github.com/zclconf/go-cty/cty"
 )
 
-// FormatResult formats the given result value for human-readable output.
-//
-// The value must currently be a string, list, map, and any nested values
-// with those same types.
-func FormatResult(value interface{}) (string, error) {
-	return formatResult(value, false)
+// FormatValue formats a value in a way that resembles Terraform language syntax
+// and uses the type conversion functions where necessary to indicate exactly
+// what type it is given, so that equality test failures can be quickly
+// understood.
+func FormatValue(v cty.Value, indent int) string {
+	if !v.IsKnown() {
+		return "(known after apply)"
 	}
-
-func formatResult(value interface{}, nested bool) (string, error) {
-	if value == nil {
-		return "null", nil
-	}
-	switch output := value.(type) {
-	case string:
-		if nested {
-			return fmt.Sprintf("%q", output), nil
-		}
-		return output, nil
-	case int:
-		return strconv.Itoa(output), nil
-	case float64:
-		return fmt.Sprintf("%g", output), nil
-	case bool:
+	if v.IsNull() {
+		ty := v.Type()
 		switch {
-		case output == true:
-			return "true", nil
+		case ty == cty.DynamicPseudoType:
+			return "null"
+		case ty == cty.String:
+			return "tostring(null)"
+		case ty == cty.Number:
+			return "tonumber(null)"
+		case ty == cty.Bool:
+			return "tobool(null)"
+		case ty.IsListType():
+			return fmt.Sprintf("tolist(null) /* of %s */", ty.ElementType().FriendlyName())
+		case ty.IsSetType():
+			return fmt.Sprintf("toset(null) /* of %s */", ty.ElementType().FriendlyName())
+		case ty.IsMapType():
+			return fmt.Sprintf("tomap(null) /* of %s */", ty.ElementType().FriendlyName())
 		default:
-			return "false", nil
-		}
-	case []interface{}:
-		return formatListResult(output)
-	case map[string]interface{}:
-		return formatMapResult(output)
-	default:
-		return "", fmt.Errorf("unknown value type: %T", value)
+			return fmt.Sprintf("null /* %s */", ty.FriendlyName())
 		}
 	}
 
-func formatListResult(value []interface{}) (string, error) {
-	var outputBuf bytes.Buffer
-	outputBuf.WriteString("[")
-	if len(value) > 0 {
-		outputBuf.WriteString("\n")
+	ty := v.Type()
+	switch {
+	case ty.IsPrimitiveType():
+		switch ty {
+		case cty.String:
+			// FIXME: If it's a multi-line string, better to render it using
+			// HEREDOC-style syntax.
+			return strconv.Quote(v.AsString())
+		case cty.Number:
+			bf := v.AsBigFloat()
+			return bf.Text('g', -1)
+		case cty.Bool:
+			if v.True() {
+				return "true"
+			} else {
+				return "false"
+			}
+		}
+	case ty.IsObjectType():
+		return formatMappingValue(v, indent)
+	case ty.IsTupleType():
+		return formatSequenceValue(v, indent)
+	case ty.IsListType():
+		return fmt.Sprintf("tolist(%s)", formatSequenceValue(v, indent))
+	case ty.IsSetType():
+		return fmt.Sprintf("toset(%s)", formatSequenceValue(v, indent))
+	case ty.IsMapType():
+		return fmt.Sprintf("tomap(%s)", formatMappingValue(v, indent))
 	}
 
-	for _, v := range value {
-		raw, err := formatResult(v, true)
-		if err != nil {
-			return "", err
+	// Should never get here because there are no other types
+	return fmt.Sprintf("%#v", v)
 }
 
-		outputBuf.WriteString(indent(raw))
-		outputBuf.WriteString(",\n")
+func formatMappingValue(v cty.Value, indent int) string {
+	var buf strings.Builder
+	count := 0
+	buf.WriteByte('{')
+	indent += 2
+	for it := v.ElementIterator(); it.Next(); {
+		count++
+		k, v := it.Element()
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
+		buf.WriteString(FormatValue(k, indent))
+		buf.WriteString(" = ")
+		buf.WriteString(FormatValue(v, indent))
+	}
+	indent -= 2
+	if count > 0 {
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
+	}
+	buf.WriteByte('}')
+	return buf.String()
 }
 
-	outputBuf.WriteString("]")
-	return outputBuf.String(), nil
+func formatSequenceValue(v cty.Value, indent int) string {
+	var buf strings.Builder
+	count := 0
+	buf.WriteByte('[')
+	indent += 2
+	for it := v.ElementIterator(); it.Next(); {
+		count++
+		_, v := it.Element()
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
+		buf.WriteString(FormatValue(v, indent))
+		buf.WriteByte(',')
 	}
-
-func formatMapResult(value map[string]interface{}) (string, error) {
-	ks := make([]string, 0, len(value))
-	for k, _ := range value {
-		ks = append(ks, k)
+	indent -= 2
+	if count > 0 {
+		buf.WriteByte('\n')
+		buf.WriteString(strings.Repeat(" ", indent))
 	}
-	sort.Strings(ks)
-
-	var outputBuf bytes.Buffer
-	outputBuf.WriteString("{")
-	if len(value) > 0 {
-		outputBuf.WriteString("\n")
-	}
-
-	for _, k := range ks {
-		v := value[k]
-		rawK, err := formatResult(k, true)
-		if err != nil {
-			return "", err
-		}
-		rawV, err := formatResult(v, true)
-		if err != nil {
-			return "", err
-		}
-
-		outputBuf.WriteString(indent(fmt.Sprintf("%s = %s", rawK, rawV)))
-		outputBuf.WriteString("\n")
-	}
-
-	outputBuf.WriteString("}")
-	return outputBuf.String(), nil
-}
-
-func indent(value string) string {
-	var outputBuf bytes.Buffer
-	s := bufio.NewScanner(strings.NewReader(value))
-	newline := false
-	for s.Scan() {
-		if newline {
-			outputBuf.WriteByte('\n')
-		}
-		outputBuf.WriteString("  " + s.Text())
-		newline = true
-	}
-
-	return outputBuf.String()
+	buf.WriteByte(']')
+	return buf.String()
 }

repl/format_test.go

@@ -0,0 +1,149 @@
+package repl
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/zclconf/go-cty/cty"
+)
+
+func TestFormatValue(t *testing.T) {
+	tests := []struct {
+		Val  cty.Value
+		Want string
+	}{
+		{
+			cty.NullVal(cty.DynamicPseudoType),
+			`null`,
+		},
+		{
+			cty.NullVal(cty.String),
+			`tostring(null)`,
+		},
+		{
+			cty.NullVal(cty.Number),
+			`tonumber(null)`,
+		},
+		{
+			cty.NullVal(cty.Bool),
+			`tobool(null)`,
+		},
+		{
+			cty.NullVal(cty.List(cty.String)),
+			`tolist(null) /* of string */`,
+		},
+		{
+			cty.NullVal(cty.Set(cty.Number)),
+			`toset(null) /* of number */`,
+		},
+		{
+			cty.NullVal(cty.Map(cty.Bool)),
+			`tomap(null) /* of bool */`,
+		},
+		{
+			cty.NullVal(cty.Object(map[string]cty.Type{"a": cty.Bool})),
+			`null /* object */`, // Ideally this would display the full object type, including its attributes
+		},
+		{
+			cty.UnknownVal(cty.DynamicPseudoType),
+			`(known after apply)`,
+		},
+		{
+			cty.StringVal(""),
+			`""`,
+		},
+		{
+			cty.StringVal("hello"),
+			`"hello"`,
+		},
+		{
+			cty.StringVal("hello\nworld"),
+			`"hello\nworld"`, // Ideally we'd use heredoc syntax here for better readability, but we don't yet
+		},
+		{
+			cty.Zero,
+			`0`,
+		},
+		{
+			cty.NumberIntVal(5),
+			`5`,
+		},
+		{
+			cty.NumberFloatVal(5.2),
+			`5.2`,
+		},
+		{
+			cty.False,
+			`false`,
+		},
+		{
+			cty.True,
+			`true`,
+		},
+		{
+			cty.EmptyObjectVal,
+			`{}`,
+		},
+		{
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.StringVal("b"),
+			}),
+			`{
+  "a" = "b"
+}`,
+		},
+		{
+			cty.ObjectVal(map[string]cty.Value{
+				"a": cty.StringVal("b"),
+				"c": cty.StringVal("d"),
+			}),
+			`{
+  "a" = "b"
+  "c" = "d"
+}`,
+		},
+		{
+			cty.MapValEmpty(cty.String),
+			`tomap({})`,
+		},
+		{
+			cty.EmptyTupleVal,
+			`[]`,
+		},
+		{
+			cty.TupleVal([]cty.Value{
+				cty.StringVal("b"),
+			}),
+			`[
+  "b",
+]`,
+		},
+		{
+			cty.TupleVal([]cty.Value{
+				cty.StringVal("b"),
+				cty.StringVal("d"),
+			}),
+			`[
+  "b",
+  "d",
+]`,
+		},
+		{
+			cty.ListValEmpty(cty.String),
+			`tolist([])`,
+		},
+		{
+			cty.SetValEmpty(cty.String),
+			`toset([])`,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("%#v", test.Val), func(t *testing.T) {
+			got := FormatValue(test.Val, 0)
+			if got != test.Want {
+				t.Errorf("wrong result\nvalue: %#v\ngot:   %s\nwant:  %s", test.Val, got, test.Want)
+			}
+		})
+	}
+}

repl/session.go

@@ -2,14 +2,12 @@ package repl
 
 import (
 	"errors"
-	"fmt"
 	"strings"
 
 	"github.com/zclconf/go-cty/cty"
 
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
-	"github.com/hashicorp/terraform/configs/hcl2shim"
 	"github.com/hashicorp/terraform/lang"
 	"github.com/hashicorp/terraform/tfdiags"
 )
@@ -61,25 +59,7 @@ func (s *Session) handleEval(line string) (string, tfdiags.Diagnostics) {
 		return "", diags
 	}
 
-	if !val.IsWhollyKnown() {
-		// FIXME: In future, once we've updated the result formatter to be
-		// cty-aware, we should just include unknown values as "(not yet known)"
-		// in the serialized result, allowing the rest (if any) to be seen.
-		diags = diags.Append(fmt.Errorf("Result depends on values that cannot be determined until after \"terraform apply\"."))
-		return "", diags
-	}
-
-	// Our formatter still wants an old-style raw interface{} value, so
-	// for now we'll just shim it.
-	// FIXME: Port the formatter to work with cty.Value directly.
-	legacyVal := hcl2shim.ConfigValueFromHCL2(val)
-	result, err := FormatResult(legacyVal)
-	if err != nil {
-		diags = diags.Append(err)
-		return "", diags
-	}
-
-	return result, diags
+	return FormatValue(val, 0), diags
 }
 
 func (s *Session) handleHelp() (string, tfdiags.Diagnostics) {

repl/session_test.go

@@ -73,7 +73,7 @@ func TestSession_basicState(t *testing.T) {
 			Inputs: []testSessionInput{
 				{
 					Input:  "test_instance.foo.id",
-					Output: "bar",
+					Output: `"bar"`,
 				},
 			},
 		})