eval: tolerate syntax errors

We must tolerate syntax errors when loading an environment. Returning a
nil `EnvironmentDecl` prevents us from being able to analyze the
portions of the environment that were successfully parsed. This
interferes with analysis of environments that are only partially
erroneous. One important scenario this covers is autocompletion in the
face of syntax errors, which is common while editing an environment.

Author: pgavlin

CHANGELOG_PENDING.md

@@ -9,4 +9,6 @@
 
 ### Bug Fixes
 
+- Environment declarations are now returned even in the face of syntax errors.
+
 ### Breaking changes

ast/expr.go

@@ -892,7 +892,7 @@ func parseFromBase64(node *syntax.ObjectNode, name *StringExpr, args Expr) (Expr
 func parseSecret(node *syntax.ObjectNode, name *StringExpr, value Expr) (Expr, syntax.Diagnostics) {
 	if arg, ok := value.(*ObjectExpr); ok && len(arg.Entries) == 1 {
 		kvp := arg.Entries[0]
-		if kvp.Key.Value == "ciphertext" {
+		if kvp.Key.GetValue() == "ciphertext" {
 			if str, ok := kvp.Value.(*StringExpr); ok {
 				return CiphertextSyntax(node, name, arg, str), nil
 			}

eval/eval.go

@@ -70,10 +70,6 @@ func LoadYAMLBytes(filename string, source []byte) (*ast.EnvironmentDecl, syntax
 
 	t, tdiags := ast.ParseEnvironment(source, syn)
 	diags.Extend(tdiags...)
-	if tdiags.HasErrors() {
-		return nil, diags, nil
-	}
-
 	return t, diags, nil
 }
 
@@ -391,11 +387,14 @@ func declare[Expr exprNode](e *evalContext, path string, x Expr, base *value) *e
 	case *ast.ObjectExpr:
 		properties := make(map[string]*expr, len(x.Entries))
 		for _, entry := range x.Entries {
-			k := entry.Key.Value
-			if _, ok := properties[k]; ok {
-				e.errorf(entry.Key, "duplicate key %q", k)
-			} else {
-				properties[k] = declare(e, util.JoinKey(path, k), entry.Value, base.property(entry.Key, k))
+			// Possible during parse errors. We elide properties with nil keys.
+			if entry.Key != nil {
+				k := entry.Key.Value
+				if _, ok := properties[k]; ok {
+					e.errorf(entry.Key, "duplicate key %q", k)
+				} else {
+					properties[k] = declare(e, util.JoinKey(path, k), entry.Value, base.property(entry.Key, k))
+				}
 			}
 		}
 		repr := &objectExpr{node: x, properties: properties}

eval/eval_test.go

@@ -252,6 +252,10 @@ type testEnvironments struct {
 }
 
 func (e *testEnvironments) LoadEnvironment(ctx context.Context, name string) ([]byte, Decrypter, error) {
+	if e.root == "" {
+		return nil, nil, os.ErrNotExist
+	}
+
 	bytes, err := os.ReadFile(filepath.Join(e.root, name+".yaml"))
 	if err != nil {
 		return nil, nil, err
@@ -600,3 +604,37 @@ func BenchmarkEvalEnvLoad(b *testing.B) {
 func BenchmarkEvalAll(b *testing.B) {
 	benchmarkEval(b, 10*time.Millisecond, 10*time.Millisecond)
 }
+
+// TestSyntaxErrorCheck provides additional insurance that we are able to parse and analyze environments that contain
+// syntax errors. It is important that we are able to provide as much information about an environment as we can so
+// that tools that depend on an environment's typed AST or implied schema needs this fix can operate properly, even on
+// environment definitions that contain syntax errors. Most notably we need this for autocomplete, but other
+// type/schema-based features also benefit (signature help, go-to-def, et cetera).
+//
+// This is also covered by tests under TestEval, but it can be easy to miss the magnitude of changes to those tests.
+func TestSyntaxErrorCheck(t *testing.T) {
+	environmentName := "syntax-error"
+	envBytes := []byte(`values:
+  foo:
+    bar: ${foo.}
+`)
+
+	env, loadDiags, err := LoadYAMLBytes(environmentName, envBytes)
+	require.NoError(t, err)
+	sortEnvironmentDiagnostics(loadDiags)
+
+	execContext, err := esc.NewExecContext(map[string]esc.Value{
+		"pulumi": esc.NewValue(map[string]esc.Value{
+			"user": esc.NewValue(map[string]esc.Value{
+				"id": esc.NewValue("USER_123"),
+			}),
+		}),
+	})
+	assert.NoError(t, err)
+
+	check, checkDiags := CheckEnvironment(context.Background(), environmentName, env, rot128{}, testProviders{},
+		&testEnvironments{}, execContext, false)
+
+	assert.True(t, checkDiags.HasErrors())
+	assert.NotNil(t, check)
+}

eval/expr.go

@@ -300,7 +300,9 @@ func (x *expr) export(environment string) esc.Expr {
 	case *objectExpr:
 		ex.KeyRanges = make(map[string]esc.Range, len(repr.node.Entries))
 		for _, kvp := range repr.node.Entries {
-			ex.KeyRanges[kvp.Key.Value] = convertRange(kvp.Key.Syntax().Syntax().Range(), environment)
+			if kvp.Key != nil {
+				ex.KeyRanges[kvp.Key.Value] = convertRange(kvp.Key.Syntax().Syntax().Range(), environment)
+			}
 		}
 
 		ex.Object = make(map[string]esc.Expr, len(repr.properties))