Merge #11359

11359: Address the `cannot traverse union(...)` error when converting examples r=iwahbe a=iwahbe

Fixes #11346

In every case I examined, the original error came from mis-typed PCL. The fix here allows us to continue without error, but it won't generate correctly typed code.

This PR fixes a couple of different problems. 
1. It improves the diagognostics, both within the binder and within the test framework.
2. It moves omitted errors to warnings (instead of dropping them).
3. It prevents rewrites on dynamic types, fixing the original error.


Co-authored-by: Ian Wahbe <ian@wahbe.com>

changelog/pending/20221116--programgen-dotnet-go-nodejs-python--dont-generate-traverse-error-when-typecheck-is-disabled.yaml

@@ -0,0 +1,4 @@
+changes:
+  - type: fix
+    scope: programgen/dotnet,go,nodejs,python
+    description: Don't generate traverse errors when typechecking a dynamic type

pkg/codegen/hcl2/model/binder_expression.go

@@ -607,10 +607,9 @@ func (b *expressionBinder) bindScopeTraversalExpression(
 			parts[i] = DynamicType
 		}
 
-		if !b.options.allowMissingVariables {
-			diagnostics = hcl.Diagnostics{
-				undefinedVariable(rootName, syntax.Traversal.SimpleSplit().Abs.SourceRange()),
-			}
+		diagnostics = hcl.Diagnostics{
+			undefinedVariable(rootName, syntax.Traversal.SimpleSplit().Abs.SourceRange(),
+				b.options.allowMissingVariables),
 		}
 		return &ScopeTraversalExpression{
 			Syntax:    syntax,

pkg/codegen/hcl2/model/diagnostics.go

@@ -25,6 +25,10 @@ func errorf(subject hcl.Range, f string, args ...interface{}) *hcl.Diagnostic {
 	return diagf(hcl.DiagError, subject, f, args...)
 }
 
+func warnf(subject hcl.Range, f string, args ...interface{}) *hcl.Diagnostic {
+	return diagf(hcl.DiagWarning, subject, f, args...)
+}
+
 func diagf(severity hcl.DiagnosticSeverity, subject hcl.Range, f string, args ...interface{}) *hcl.Diagnostic {
 	message := fmt.Sprintf(f, args...)
 	return &hcl.Diagnostic{
@@ -111,8 +115,12 @@ func unsupportedCollectionType(collectionType Type, iteratorRange hcl.Range) *hc
 	return errorf(iteratorRange, "cannot iterate over a value of type %v", collectionType)
 }
 
-func undefinedVariable(variableName string, variableRange hcl.Range) *hcl.Diagnostic {
-	return errorf(variableRange, fmt.Sprintf("undefined variable %v", variableName))
+func undefinedVariable(variableName string, variableRange hcl.Range, warn bool) *hcl.Diagnostic {
+	f := errorf
+	if warn {
+		f = warnf
+	}
+	return f(variableRange, "undefined variable %v", variableName)
 }
 
 func internalError(rng hcl.Range, fmt string, args ...interface{}) *hcl.Diagnostic {

pkg/codegen/hcl2/model/scope.go

@@ -140,13 +140,13 @@ func (s *Scope) Traverse(traverser hcl.Traverser) (Traversable, hcl.Diagnostics)
 	name, nameType := GetTraverserKey(traverser)
 	if nameType != StringType {
 		// TODO(pdg): return a better error here
-		return DynamicType, hcl.Diagnostics{undefinedVariable("", traverser.SourceRange())}
+		return DynamicType, hcl.Diagnostics{undefinedVariable("", traverser.SourceRange(), false)}
 	}
 
 	memberName := name.AsString()
 	member, hasMember := s.BindReference(memberName)
 	if !hasMember {
-		return DynamicType, hcl.Diagnostics{undefinedVariable(memberName, traverser.SourceRange())}
+		return DynamicType, hcl.Diagnostics{undefinedVariable(memberName, traverser.SourceRange(), false)}
 	}
 	return member, nil
 }

pkg/codegen/hcl2/model/traversable.go

@@ -103,9 +103,8 @@ func bindTraversalParts(receiver Traversable, traversal hcl.Traversal,
 		// OK
 	default:
 		// TODO(pdg): improve this diagnostic
-		if !allowMissingVariables {
-			diagnostics = append(diagnostics, undefinedVariable("", traversal.SourceRange()))
-		}
+		diagnostics = append(diagnostics, undefinedVariable("",
+			traversal.SourceRange(), allowMissingVariables))
 	}
 
 	return parts, diagnostics

pkg/codegen/hcl2/model/type_union.go

@@ -35,7 +35,11 @@ type UnionType struct {
 }
 
 // NewUnionTypeAnnotated creates a new union type with the given element types and annotations.
-// Any element types that are union types are replaced with their element types.
+// NewUnionTypeAnnotated enforces 3 properties on the returned type:
+// 1. Any element types that are union types are replaced with their element types.
+// 2. Any duplicate types are removed.
+// 3. Unions have have more then 1 type. If only a single type is left after (1) and (2),
+// it is returned as is.
 func NewUnionTypeAnnotated(types []Type, annotations ...interface{}) Type {
 	var elementTypes []Type
 	for _, t := range types {
@@ -46,10 +50,12 @@ func NewUnionTypeAnnotated(types []Type, annotations ...interface{}) Type {
 		}
 	}
 
+	// Remove duplicate types
+	// We first sort the types so duplicates will be adjacent
 	sort.Slice(elementTypes, func(i, j int) bool {
 		return elementTypes[i].String() < elementTypes[j].String()
 	})
-
+	// We then filter out adjacent duplicates
 	dst := 0
 	for src := 0; src < len(elementTypes); {
 		for src < len(elementTypes) && elementTypes[src].Equals(elementTypes[dst]) {
@@ -63,6 +69,8 @@ func NewUnionTypeAnnotated(types []Type, annotations ...interface{}) Type {
 	}
 	elementTypes = elementTypes[:dst]
 
+	// If the union turns out to be the union of a single type, just return the underlying
+	// type.
 	if len(elementTypes) == 1 {
 		return elementTypes[0]
 	}
@@ -103,9 +111,10 @@ func (t *UnionType) Traverse(traverser hcl.Traverser) (Traversable, hcl.Diagnost
 	var foundDiags hcl.Diagnostics
 	for _, t := range t.ElementTypes {
 		// We handle 'none' specially here: so that traversing an optional type returns an optional type.
-		if t == NoneType {
+		switch t {
+		case NoneType:
 			types = append(types, NoneType)
-		} else {
+		default:
 			// Note that we only report errors when the entire operation fails. We try to
 			// strike a balance between assuming that the traversal will dynamically
 			// succeed and good error reporting.

pkg/codegen/pcl/binder_resource.go

@@ -361,24 +361,29 @@ func (b *binder) bindResourceBody(node *Resource) hcl.Diagnostics {
 	}
 
 	// Typecheck the attributes.
-	if objectType, ok := node.InputType.(*model.ObjectType); ok && !b.options.skipResourceTypecheck {
+	if objectType, ok := node.InputType.(*model.ObjectType); ok {
+		diag := func(d *hcl.Diagnostic) {
+			if b.options.skipResourceTypecheck && d.Severity == hcl.DiagError {
+				d.Severity = hcl.DiagWarning
+			}
+			diagnostics = append(diagnostics, d)
+		}
 		attrNames := codegen.StringSet{}
 		for _, attr := range node.Inputs {
 			attrNames.Add(attr.Name)
 
 			if typ, ok := objectType.Properties[attr.Name]; ok {
 				if !typ.ConversionFrom(attr.Value.Type()).Exists() {
-					diagnostics = append(diagnostics, model.ExprNotConvertible(typ, attr.Value))
+					diag(model.ExprNotConvertible(typ, attr.Value))
 				}
 			} else {
-				diagnostics = append(diagnostics, unsupportedAttribute(attr.Name, attr.Syntax.NameRange))
+				diag(unsupportedAttribute(attr.Name, attr.Syntax.NameRange))
 			}
 		}
 
 		for _, k := range codegen.SortedKeys(objectType.Properties) {
 			if !model.IsOptionalType(objectType.Properties[k]) && !attrNames.Has(k) {
-				diagnostics = append(diagnostics,
-					missingRequiredAttribute(k, block.Body.Syntax.MissingItemRange()))
+				diag(missingRequiredAttribute(k, block.Body.Syntax.MissingItemRange()))
 			}
 		}
 	}

pkg/codegen/pcl/binder_schema.go

@@ -237,7 +237,7 @@ func (b *binder) loadReferencedPackageSchemas(n Node) error {
 	contract.Assert(len(diags) == 0)
 
 	for _, name := range packageNames.SortedValues() {
-		if _, ok := b.referencedPackages[name]; ok && pkgOpts.version == "" {
+		if _, ok := b.referencedPackages[name]; ok && pkgOpts.version == "" || name == "" {
 			continue
 		}
 

pkg/codegen/pcl/binder_test.go

@@ -1,4 +1,4 @@
-package pcl
+package pcl_test
 
 import (
 	"bytes"
@@ -13,10 +13,14 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/pulumi/pulumi/pkg/v3/codegen/hcl2/syntax"
+	"github.com/pulumi/pulumi/pkg/v3/codegen/pcl"
 	"github.com/pulumi/pulumi/pkg/v3/codegen/schema"
+	"github.com/pulumi/pulumi/pkg/v3/codegen/testing/test"
 	"github.com/pulumi/pulumi/pkg/v3/codegen/testing/utils"
 )
 
+var testdataPath = filepath.Join("..", "testing", "test", "testdata")
+
 func TestBindProgram(t *testing.T) {
 	t.Parallel()
 
@@ -25,6 +29,11 @@ func TestBindProgram(t *testing.T) {
 		t.Fatalf("could not read test data: %v", err)
 	}
 
+	bindOptions := map[string][]pcl.BindOption{}
+	for _, r := range test.PulumiPulumiProgramTests {
+		bindOptions[r.Directory+"-pp"] = r.BindOptions
+	}
+
 	//nolint:paralleltest // false positive because range var isn't used directly in t.Run(name) arg
 	for _, v := range testdata {
 		v := v
@@ -56,16 +65,9 @@ func TestBindProgram(t *testing.T) {
 
 				var bindError error
 				var diags hcl.Diagnostics
-				loader := Loader(schema.NewPluginLoader(utils.NewHost(testdataPath)))
-				if fileName == "simple-range.pp" {
-					// simple-range.pp requires AllowMissingVariables
-					// TODO: remove this once we have a better way to handle this
-					// https://github.com/pulumi/pulumi/issues/10985
-					_, diags, bindError = BindProgram(parser.Files, loader, AllowMissingVariables)
-				} else {
-					// all other PCL files use a more restrict program bind
-					_, diags, bindError = BindProgram(parser.Files, loader)
-				}
+				loader := pcl.Loader(schema.NewPluginLoader(utils.NewHost(testdataPath)))
+				// PCL binder options are taken from program_driver.go
+				_, diags, bindError = pcl.BindProgram(parser.Files, append(bindOptions[v.Name()], loader)...)
 
 				assert.NoError(t, bindError)
 				if diags.HasErrors() {

pkg/codegen/pcl/rewrite_convert.go

@@ -98,13 +98,19 @@ func rewriteConversions(x model.Expression, to model.Type, diags *hcl.Diagnostic
 		}
 		for i := range x.Items {
 			item := &x.Items[i]
+			if item.Key.Type() == model.DynamicType {
+				// We don't know the type of this expression, so we can't correct the
+				// type.
+				continue
+			}
 
 			key, ediags := item.Key.Evaluate(&hcl.EvalContext{}) // empty context, we need a constant string
 			*diags = diags.Extend(ediags)
 
-			traverser := hcl.TraverseIndex{Key: key}
-
-			valueType, tdiags := to.Traverse(traverser)
+			valueType, tdiags := to.Traverse(hcl.TraverseIndex{
+				Key:      key,
+				SrcRange: item.Key.SyntaxNode().Range(),
+			})
 			*diags = diags.Extend(tdiags)
 
 			var valueChanged bool
@@ -113,12 +119,20 @@ func rewriteConversions(x model.Expression, to model.Type, diags *hcl.Diagnostic
 			typecheck = typecheck || valueChanged
 		}
 	case *model.TupleConsExpression:
-		for i := range x.Expressions {
-			valueType, tdiags := to.Traverse(hcl.TraverseIndex{Key: cty.NumberIntVal(int64(i))})
+		for i, expr := range x.Expressions {
+			if expr.Type() == model.DynamicType {
+				// We don't know the type of this expression, so we can't correct the
+				// type.
+				continue
+			}
+			valueType, tdiags := to.Traverse(hcl.TraverseIndex{
+				Key:      cty.NumberIntVal(int64(i)),
+				SrcRange: x.Syntax.Range(),
+			})
 			*diags = diags.Extend(tdiags)
 
 			var exprChanged bool
-			x.Expressions[i], exprChanged = rewriteConversions(x.Expressions[i], valueType.(model.Type), diags)
+			x.Expressions[i], exprChanged = rewriteConversions(expr, valueType.(model.Type), diags)
 			typecheck = typecheck || exprChanged
 		}
 	case *model.UnaryOpExpression:

pkg/codegen/report/report_test.go

@@ -65,7 +65,7 @@ func TestReportExample(t *testing.T) {
 					"Might not bind": "error: could not locate a compatible plugin in " +
 						"deploytest, the makefile and & constructor of the plugin host " +
 						"must define the location of the schema: failed " +
-						"to locate compatible plugin",
+						`to locate compatible plugin: "not"`,
 				},
 				Files: map[string][]report.File{
 					"Might not bind": {{Name: "Might not bind", Body: "resource foo \"not:a:Resource\" { foo: \"bar\" }"}},

pkg/codegen/testing/test/program_driver.go

@@ -215,6 +215,26 @@ var PulumiPulumiProgramTests = []ProgramTest{
 		Skip:        codegen.NewStringSet("go"),
 		// Blocked on go: TODO[pulumi/pulumi#10834]
 	},
+	{
+		Directory:   "traverse-union-repro",
+		Description: `Handling the error "cannot traverse value of type union(...)"`,
+		BindOptions: []pcl.BindOption{
+			pcl.SkipResourceTypechecking,
+			pcl.AllowMissingVariables,
+			pcl.AllowMissingProperties,
+		},
+		// The example is known to be invalid. Specifically it hands a
+		// `[aws_subnet.test1.id]` to a `string` attribute, where `aws_subnet` is not in
+		// scope.
+		//
+		// The example is invalid in two ways:
+		// 1. `aws_subnet` is a missing variable.
+		// 2. `[...]` is a tuple, which can never be a string.
+		//
+		// Even though the generated code will not type check, it should still be
+		// directionally correct.
+		SkipCompile: allProgLanguages,
+	},
 }
 
 var PulumiPulumiYAMLProgramTests = []ProgramTest{
@@ -415,17 +435,21 @@ func TestProgramCodegen(
 				t.Fatalf("failed to parse files: %v", parser.Diagnostics)
 			}
 
-			opts := []pcl.BindOption{
-				pcl.PluginHost(utils.NewHost(testdataPath)),
-			}
-			opts = append(opts, tt.BindOptions...)
+			hclFiles := map[string]*hcl.File{
+				tt.Directory + ".pp": {Body: parser.Files[0].Body, Bytes: parser.Files[0].Bytes}}
+			opts := append(tt.BindOptions, pcl.PluginHost(utils.NewHost(testdataPath)))
 
 			program, diags, err := pcl.BindProgram(parser.Files, opts...)
 			if err != nil {
 				t.Fatalf("could not bind program: %v", err)
 			}
-			if diags.HasErrors() {
-				t.Fatalf("failed to bind program: %v", diags)
+			bindDiags := new(bytes.Buffer)
+			if len(diags) > 0 {
+				require.NoError(t, hcl.NewDiagnosticTextWriter(bindDiags, hclFiles, 80, false).WriteDiagnostics(diags))
+				if diags.HasErrors() {
+					t.Fatalf("failed to bind program:\n%s", bindDiags)
+				}
+				t.Logf("bind diags:\n%s", bindDiags)
 			}
 			var files map[string][]byte
 			// generate a full project and check expected package versions
@@ -455,7 +479,12 @@ func TestProgramCodegen(
 				diags = tmpDiags
 			}
 			if diags.HasErrors() {
-				t.Fatalf("failed to generate program: %v", diags)
+				buf := new(bytes.Buffer)
+
+				err := hcl.NewDiagnosticTextWriter(buf, hclFiles, 80, false).WriteDiagnostics(diags)
+				require.NoError(t, err, "Failed to write diag")
+
+				t.Fatalf("failed to generate program:\n%s", buf)
 			}
 
 			if pulumiAccept {

pkg/codegen/testing/test/testdata/traverse-union-repro-pp/dotnet/traverse-union-repro.cs

@@ -0,0 +1,19 @@
+using System.Collections.Generic;
+using Pulumi;
+using Aws = Pulumi.Aws;
+
+return await Deployment.RunAsync(() => 
+{
+    var test = new Aws.Fsx.OpenZfsFileSystem("test", new()
+    {
+        StorageCapacity = 64,
+        SubnetIds = new[]
+        {
+            aws_subnet.Test1.Id,
+        },
+        DeploymentType = "SINGLE_AZ_1",
+        ThroughputCapacity = 64,
+    });
+
+});
+

pkg/codegen/testing/test/testdata/traverse-union-repro-pp/go/traverse-union-repro.go

@@ -0,0 +1,23 @@
+package main
+
+import (
+	"github.com/pulumi/pulumi-aws/sdk/v5/go/aws/fsx"
+	"github.com/pulumi/pulumi/sdk/v3/go/pulumi"
+)
+
+func main() {
+	pulumi.Run(func(ctx *pulumi.Context) error {
+		_, err := fsx.NewOpenZfsFileSystem(ctx, "test", &fsx.OpenZfsFileSystemArgs{
+			StorageCapacity: pulumi.Int(64),
+			SubnetIds: pulumi.String{
+				aws_subnet.Test1.Id,
+			},
+			DeploymentType:     pulumi.String("SINGLE_AZ_1"),
+			ThroughputCapacity: pulumi.Int(64),
+		})
+		if err != nil {
+			return err
+		}
+		return nil
+	})
+}

pkg/codegen/testing/test/testdata/traverse-union-repro-pp/nodejs/traverse-union-repro.ts

@@ -0,0 +1,9 @@
+import * as pulumi from "@pulumi/pulumi";
+import * as aws from "@pulumi/aws";
+
+const test = new aws.fsx.OpenZfsFileSystem("test", {
+    storageCapacity: 64,
+    subnetIds: [aws_subnet.test1.id],
+    deploymentType: "SINGLE_AZ_1",
+    throughputCapacity: 64,
+});