cue: fix nil pointer deref in BuildInstances

When building instances, we currently unconditionally try to make and
then append a Value to the []Value slice returned, even in cases where
the build of an instance fails.

Fix that by appending an error Value in cases where the build fails,
only appending a non-error Value when the build succeeds.

This way we retain the property that len(instances) matches the length
of the returned []Value slice.

Per the docs, the error value returned is the combined errors of
building those instances, or nil if there were no errors.

Closes cue-lang#1444

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
Change-Id: I3e5107f67814de5eeeaa8db23040bf200de8fd9e
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/532990
Reviewed-by: Paul Jolly <paul@myitcv.io>

cue/context.go

@@ -143,8 +143,10 @@ func (c *Context) BuildInstances(instances []*build.Instance) ([]Value, error) {
 		v, err := c.runtime().Build(nil, b)
 		if err != nil {
 			errs = errors.Append(errs, err)
+			a = append(a, c.makeError(err))
+		} else {
+			a = append(a, c.make(v))
 		}
-		a = append(a, c.make(v))
 	}
 	return a, errs
 }

cue/context_test.go

@@ -16,10 +16,14 @@ package cue_test
 
 import (
 	"fmt"
+	"io/ioutil"
 	"testing"
 
 	"cuelang.org/go/cue"
+	"cuelang.org/go/cue/build"
 	"cuelang.org/go/cue/cuecontext"
+	"cuelang.org/go/internal/cuetxtar"
+	"github.com/rogpeppe/go-internal/txtar"
 )
 
 func TestNewList(t *testing.T) {
@@ -80,3 +84,58 @@ func TestNewList(t *testing.T) {
 		})
 	}
 }
+
+func TestBuildInstancesSuccess(t *testing.T) {
+	in := `
+-- foo.cue --
+package foo
+
+foo: [{a: "b", c: "d"}, {a: "e", g: "f"}]
+bar: [
+	for f in foo
+	if (f & {c: "b"}) != _|_
+	{f}
+]
+`
+
+	a := txtar.Parse([]byte(in))
+	dir, _ := ioutil.TempDir("", "*")
+	instance := cuetxtar.Load(a, dir)[0]
+	if instance.Err != nil {
+		t.Fatal(instance.Err)
+	}
+
+	_, err := cuecontext.New().BuildInstances([]*build.Instance{instance})
+	if err != nil {
+		t.Fatalf("BuildInstances() = %v", err)
+	}
+}
+
+func TestBuildInstancesError(t *testing.T) {
+	in := `
+-- foo.cue --
+package foo
+
+foo: [{a: "b", c: "d"}, {a: "e", g: "f"}]
+bar: [
+	for f in foo
+	if f & {c: "b") != _|_   // NOTE: ')' instead of '}'
+	{f}
+]
+`
+
+	a := txtar.Parse([]byte(in))
+	dir, _ := ioutil.TempDir("", "*")
+	instance := cuetxtar.Load(a, dir)[0]
+
+	// Normally, this should be checked, however, this is explicitly
+	// testing the path where this was NOT checked.
+	// if instance.Err != nil {
+	// 	t.Fatal(instance.Err)
+	// }
+
+	vs, err := cuecontext.New().BuildInstances([]*build.Instance{instance})
+	if err == nil {
+		t.Fatalf("BuildInstances() = %#v, wanted error", vs)
+	}
+}