cue-lang / cue

The home of the CUE language! Validate and define text-based and dynamic configuration
https://cuelang.org
Apache License 2.0
5.02k stars 287 forks source link

cue/ast: deprecate File.Imports in favor of an import iterator like File.VisitImports #3324

Open mvdan opened 1 month ago

mvdan commented 1 month ago

Currently, a file's imported packages are tracked in two places separately:

1) As a flat list of type []*ast.ImportSpec in ast.File.Imports, used in cases like the loader or compiler, where one just needs to list all the imports to use them.

2) As grouped *ast.ImportDecl declarations in ast.File.Decls, used to keep track of the original syntax information so that cue/format can print the syntax back out as CUE, and full position information is retained.

This is inherited from Go's https://pkg.go.dev/go/ast#File. It made sense that Go began with both fields and duplicate data back in the day, because being able to iterate over the flat list of imports is a common need for many syntax tree users, and Go didn't gain iterators until very recently. A method that let you iterate over a flat list of imports would have had to allocate a slice and copy them all in upfront, which isn't great.

The duplicate information is not just wasteful, but also error prone. Take for instance https://review.gerrithub.io/c/cue-lang/cue/+/1198351 - astutil.Sanitize updated Decls when removing an unused import but not Imports, hence they ended up inconsistent. Printing the CUE via cue/format would omit the removed import as it uses Decls, but trying to build the file would fail as the compiler uses Imports.

Now we have iterators, so we should replace ast.File.Imports with an iterator method. We even have one such method already in https://pkg.go.dev/cuelang.org/go/cue/ast@master#File.VisitImports, although for _, spec := range file.VisitImports { reads a bit weird, so we might need a new name for it like AllImports.

I propose the following sequential steps: 1) Add a new File.AllImports method following the common iterator signature from https://github.com/golang/go/issues/61897, e.g. func (*File) AllImports() iter.Seq[*ImportSpec]. 2) Rewrite File.VisitImports in terms of File.AllImports, and deprecate it in favor of the new func. 3) Deprecate File.Imports in favor of File.AllImports. 4) About a year later, remove File.Imports.

mvdan commented 1 month ago

Also worth noting that I'm tracking the APIs where we should start using Go func iterators soon here, including ast.File.VisitImports: https://github.com/cue-lang/cue/issues/2953

I should also note that the iterator being called AllImports rather than just Imports is perhaps slightly unfortunate, but we can't possibly make a slow and nice transition where we swap File.Imports from []*ast.ImportSpec to iter.Seq[*ImportSpec]. Iterating via a for loop would work the same on both, but any other use case would be broken.