cue: fix import usage detection for partially resolved files

Previously, the usage of imports was deteced by matching
them with unresolved references. This did not work, however,
when passing a previously processed ast.File, with already
resolved references.

The problem was previously undetected, as the CUE builder
would always write out files and parse them back in. Recent
optimizations that elide this step exposed this problem.

This change also makes uses of the newer multi-error
infrastructure and improves some of the naming of vars.

Change-Id: Ieda38b989b050d90f0ced49e91acaea58ca47e19
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/5360
Reviewed-by: Jason Wang <jasonwzm@google.com>
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>

cue/build.go

@@ -361,9 +361,9 @@ func resolveFiles(idx *index, p *build.Instance) errors.Error {
 }
 
 func resolveFile(idx *index, f *ast.File, p *build.Instance, allFields map[string]ast.Node) errors.Error {
-	index := map[string][]*ast.Ident{}
+	unresolved := map[string][]*ast.Ident{}
 	for _, u := range f.Unresolved {
-		index[u.Name] = append(index[u.Name], u)
+		unresolved[u.Name] = append(unresolved[u.Name], u)
 	}
 	fields := map[string]ast.Node{}
 	for _, d := range f.Decls {
@@ -373,7 +373,9 @@ func resolveFile(idx *index, f *ast.File, p *build.Instance, allFields map[strin
 			}
 		}
 	}
-	var errUnused errors.Error
+	var errs errors.Error
+
+	specs := []*ast.ImportSpec{}
 
 	for _, spec := range f.Imports {
 		id, err := strconv.Unquote(spec.Path.Value)
@@ -384,34 +386,65 @@ func resolveFile(idx *index, f *ast.File, p *build.Instance, allFields map[strin
 		if imp := p.LookupImport(id); imp != nil {
 			name = imp.PkgName
 		} else if _, ok := builtins[id]; !ok {
-			// continue
-			return nodeErrorf(spec, "package %q not found", id)
+			errs = errors.Append(errs,
+				nodeErrorf(spec, "package %q not found", id))
+			continue
 		}
 		if spec.Name != nil {
 			name = spec.Name.Name
 		}
 		if n, ok := fields[name]; ok {
-			return nodeErrorf(spec,
+			errs = errors.Append(errs, nodeErrorf(spec,
 				"%s redeclared as imported package name\n"+
-					"\tprevious declaration at %v", name, lineStr(idx, n))
+					"\tprevious declaration at %v", name, lineStr(idx, n)))
+			continue
 		}
 		fields[name] = spec
 		used := false
-		for _, u := range index[name] {
+		for _, u := range unresolved[name] {
 			used = true
 			u.Node = spec
 		}
 		if !used {
+			specs = append(specs, spec)
+		}
+	}
+
+	// Verify each import is used.
+	if len(specs) > 0 {
+		// Find references to imports. This assumes that identifiers in labels
+		// are not resolved or that such errors are caught elsewhere.
+		ast.Walk(f, nil, func(n ast.Node) {
+			if x, ok := n.(*ast.Ident); ok {
+				// As we also visit labels, most nodes will be nil.
+				if x.Node == nil {
+					return
+				}
+				for i, s := range specs {
+					if s == x.Node {
+						specs[i] = nil
+						return
+					}
+				}
+			}
+		})
+
+		// Add errors for unused imports.
+		for _, spec := range specs {
+			if spec == nil {
+				continue
+			}
 			if spec.Name == nil {
-				errUnused = nodeErrorf(spec,
-					"imported and not used: %s", spec.Path.Value)
+				errs = errors.Append(errs, nodeErrorf(spec,
+					"imported and not used: %s", spec.Path.Value))
 			} else {
-				errUnused = nodeErrorf(spec,
-					"imported and not used: %s as %s", spec.Path.Value, spec.Name)
+				errs = errors.Append(errs, nodeErrorf(spec,
+					"imported and not used: %s as %s", spec.Path.Value, spec.Name))
 			}
 		}
 	}
-	i := 0
+
+	k := 0
 	for _, u := range f.Unresolved {
 		if u.Node != nil {
 			continue
@@ -421,17 +454,14 @@ func resolveFile(idx *index, f *ast.File, p *build.Instance, allFields map[strin
 			u.Scope = f
 			continue
 		}
-		f.Unresolved[i] = u
-		i++
+		f.Unresolved[k] = u
+		k++
 	}
-	f.Unresolved = f.Unresolved[:i]
+	f.Unresolved = f.Unresolved[:k]
 	// TODO: also need to resolve types.
 	// if len(f.Unresolved) > 0 {
 	// 	n := f.Unresolved[0]
 	// 	return ctx.mkErr(newBase(n), "unresolved reference %s", n.Name)
 	// }
-	if errUnused != nil {
-		return errUnused
-	}
-	return nil
+	return errs
 }

cue/build_test.go

@@ -53,6 +53,45 @@ func TestFromExpr(t *testing.T) {
 	}
 }
 
+// TestPartiallyResolved tests that the resolve will detect the usage of
+// imports that are referenced by previously resolved nodes.
+func TestPartiallyResolved(t *testing.T) {
+	const importPath = "acme.com/foo"
+	spec1 := &ast.ImportSpec{
+		Path: ast.NewString(importPath),
+	}
+	spec2 := &ast.ImportSpec{
+		Name: ast.NewIdent("bar"),
+		Path: ast.NewString(importPath),
+	}
+
+	f := &ast.File{
+		Decls: []ast.Decl{
+			&ast.ImportDecl{Specs: []*ast.ImportSpec{spec1, spec2}},
+			&ast.Field{
+				Label: ast.NewIdent("X"),
+				Value: &ast.Ident{Name: "foo", Node: spec1},
+			},
+			&ast.Alias{
+				Ident: ast.NewIdent("Y"),
+				Expr:  &ast.Ident{Name: "bar", Node: spec2},
+			},
+		},
+		Imports: []*ast.ImportSpec{spec1, spec2},
+	}
+
+	err := resolveFile(nil, f, &build.Instance{
+		Imports: []*build.Instance{{
+			ImportPath: importPath,
+			PkgName:    "foo",
+		}},
+	}, map[string]ast.Node{})
+
+	if err != nil {
+		t.Errorf("exected no error, found %v", err)
+	}
+}
+
 func TestBuild(t *testing.T) {
 	files := func(s ...string) []string { return s }
 	insts := func(i ...*bimport) []*bimport { return i }
@@ -137,16 +176,17 @@ func TestBuild(t *testing.T) {
 		}),
 		`"Hello World!"`,
 	}, {
-		insts(pkg1, &bimport{"",
+		insts(pkg1, pkg2, &bimport{"",
 			files(
 				`package test
 
 				import bar "pkg1"
+				import baz "example.com/foo/pkg2:pkg"
 
 				pkg1 Object: 3
 				"Hello \(pkg1.Object)!"`),
 		}),
-		`imported and not used: "pkg1" as bar`,
+		`imported and not used: "pkg1" as bar (and 1 more errors)`,
 	}, {
 		insts(pkg2, &bimport{"",
 			files(