cue/ast/astutil: update ast.File.Imports properly in Sanitize

Sanitize removes unused imports from its input files, that is,
when a package is imported at the top of the file but not referenced.

Sanitize updated the ast.File.Decls list, which was OK for cue/format
to print out the file without the import, but it did not update
ast.File.Imports, which is a flattened list of import specs
used by the loader and the compiler.

As a sanity check, `cue trim` rebuilds each package instance it trims
before it writes the CUE files back to disk, to ensure it is not writing
invalid CUE which could cause confusing errors or data loss.
The rebuild then correctly failed with an "imported and not used" error.

Teach TestTrimFiles to catch the same mistake by rebuilding the files
as a new instance after the trimming has happened.
We already had a test which is meant to remove an unused import,
and since we didn't remove it properly per the above,
the rebuild tried and failed to load the imported package:

    --- FAIL: TestTrimFiles/trim/rmimport (0.00s)
        quicktest.go:12:
            error:
              got non-nil value
            got:
              e`package "mod.test/blah/a" not found`

Tweak Sanitize so that it keeps ast.File.Imports up to date.
ast.File holding all imports in both Decls and Imports is a bit of a
trap for any other user who wishes to add or remove any import,
so I've also raised #3324 to propose phasing out ast.File.Imports.

Updates #3206.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: I7552ffcba32f451842ea7fc0f39d71f0a365a13b
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1198351
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Matthew Sackman <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>

Author: mvdan

cue/ast/astutil/sanitize.go

@@ -168,16 +168,19 @@ func (z *sanitizer) markUsed(s *scope, n *ast.Ident) bool {
 }
 
 func (z *sanitizer) cleanImports() {
-	z.file.VisitImports(func(d *ast.ImportDecl) {
-		k := 0
-		for _, s := range d.Specs {
-			if _, ok := z.referenced[s]; ok {
-				d.Specs[k] = s
-				k++
+	var fileImports []*ast.ImportSpec
+	z.file.VisitImports(func(decl *ast.ImportDecl) {
+		newLen := 0
+		for _, spec := range decl.Specs {
+			if _, ok := z.referenced[spec]; ok {
+				fileImports = append(fileImports, spec)
+				decl.Specs[newLen] = spec
+				newLen++
 			}
 		}
-		d.Specs = d.Specs[:k]
+		decl.Specs = decl.Specs[:newLen]
 	})
+	z.file.Imports = fileImports
 }
 
 func (z *sanitizer) handleIdent(s *scope, n *ast.Ident) bool {

tools/trim/trim_test.go

@@ -17,9 +17,11 @@ package trim
 import (
 	"testing"
 
+	"cuelang.org/go/cue/build"
 	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/internal/cuetdtest"
 	"cuelang.org/go/internal/cuetxtar"
+	"github.com/go-quicktest/qt"
 )
 
 var (
@@ -43,8 +45,9 @@ func TestTrimFiles(t *testing.T) {
 		a := t.Instance()
 		ctx := t.Context()
 		val := ctx.BuildInstance(a)
-		// Note: don't check val.Err because there are deliberate
-		// errors in some tests.
+		// Note: don't require val.Err to be nil because there are deliberate
+		// errors in some tests, to ensure trim still works even with some errors.
+		hadError := val.Err() != nil
 
 		files := a.Files
 
@@ -53,6 +56,19 @@ func TestTrimFiles(t *testing.T) {
 			t.WriteErrors(errors.Promote(err, ""))
 		}
 
+		// If the files could be built without an error before,
+		// they should still build without an error after trimming.
+		// This might not be true if, for example, unused imports are not removed.
+		// Note that we need a new build.Instance to build the ast.Files from scratch again.
+		if !hadError {
+			a := build.NewContext().NewInstance("", nil)
+			for _, file := range files {
+				a.AddSyntax(file)
+			}
+			val := ctx.BuildInstance(a)
+			qt.Assert(t, qt.IsNil(val.Err()))
+		}
+
 		for _, f := range files {
 			t.WriteFile(f)
 		}