Simplify the LoadRootsWithConfig logic

This patch simplifies the LoadRootsWithConfig logic to be closer
to the logic that existed prior to the patch that addressed support
for multiple, distinct root paths. The fix that was merged was
overly complex, and not necessarily so. This update simplifies
that patch to the necessary bits.

pkg/loader/loader.go

@@ -323,7 +323,7 @@ func (l *loader) parseFile(filename string, src []byte) (*ast.File, error) {
 // populated.  Additional information, like ASTs and type-checking information,
 // can be accessed via methods on individual packages.
 func LoadRoots(roots ...string) ([]*Package, error) {
-	return LoadRootsWithConfig(nil, roots...)
+	return LoadRootsWithConfig(&packages.Config{}, roots...)
 }
 
 // LoadRootsWithConfig functions like LoadRoots, except that it allows passing
@@ -366,41 +366,30 @@ func LoadRoots(roots ...string) ([]*Package, error) {
 //
 //     5. Load the filesystem path roots and return the load packages for the
 //        package/module roots AND the filesystem path roots.
-//
-//nolint:gocyclo
-func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Package, retErr error) {
+func LoadRootsWithConfig(cfg *packages.Config, roots ...string) ([]*Package, error) {
+	l := &loader{
+		cfg:      cfg,
+		packages: make(map[*packages.Package]*Package),
+	}
+	l.cfg.Mode |= packages.LoadImports | packages.NeedTypesSizes
+	if l.cfg.Fset == nil {
+		l.cfg.Fset = token.NewFileSet()
+	}
+	// put our build flags first so that callers can override them
+	l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)
+
+	// Visit the import graphs of the loaded, root packages. If an imported
+	// package refers to another loaded, root package, then replace the
+	// instance of the imported package with a reference to the loaded, root
+	// package. This is required to make kubebuilder markers work correctly
+	// when multiple root paths are loaded and types from one path reference
+	// types from another root path.
 	defer func() {
-		if retErr != nil {
-			return
-		}
-		for i := range result {
-			visitImports(result, result[i], nil)
+		for i := range l.Roots {
+			visitImports(l.Roots, l.Roots[i], nil)
 		}
 	}()
 
-	newLoader := func(cfg *packages.Config, cfgDir string) *loader {
-		if cfg == nil {
-			cfg = &packages.Config{
-				Dir: cfgDir,
-			}
-		}
-		l := &loader{
-			cfg:      cfg,
-			packages: map[*packages.Package]*Package{},
-		}
-		l.cfg.Mode |= packages.LoadImports | packages.NeedTypesSizes
-		if l.cfg.Fset == nil {
-			l.cfg.Fset = token.NewFileSet()
-		}
-		// put our build flags first so that callers can override them
-		l.cfg.BuildFlags = append([]string{"-tags", "ignore_autogenerated"}, l.cfg.BuildFlags...)
-
-		return l
-	}
-
-	// initialize the default loader
-	l := newLoader(cfg, "")
-
 	// uniquePkgIDs is used to keep track of the discovered packages to be nice
 	// and try and prevent packages from showing up twice when nested module
 	// support is enabled. there is not harm that comes from this per se, but
@@ -412,7 +401,7 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 	// if validatePkgFn is nil, a package will be returned in the slice,
 	// otherwise the package is only returned if the result of
 	// validatePkgFn(pkg.ID) is truthy
-	loadPackages := func(l *loader, roots ...string) ([]*Package, error) {
+	loadPackages := func(roots ...string) ([]*Package, error) {
 		rawPkgs, err := packages.Load(l.cfg, roots...)
 		if err != nil {
 			return nil, err
@@ -430,11 +419,12 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 
 	// if no roots were provided then load the current package and return early
 	if len(roots) == 0 {
-		pkgs, err := loadPackages(l)
+		pkgs, err := loadPackages()
 		if err != nil {
 			return nil, err
 		}
-		return pkgs, nil
+		l.Roots = append(l.Roots, pkgs...)
+		return l.Roots, nil
 	}
 
 	// pkgRoots is a slice of roots that are package/modules and fspRoots
@@ -460,39 +450,34 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 	// system path roots due to them needing a custom, calculated value for the
 	// cfg.Dir field
 	if len(pkgRoots) > 0 {
-		pkgs, err := loadPackages(l, pkgRoots...)
+		pkgs, err := loadPackages(pkgRoots...)
 		if err != nil {
 			return nil, err
 		}
-		result = append(result, pkgs...)
+		l.Roots = append(l.Roots, pkgs...)
 	}
 
 	// if there are no filesystem path roots then go ahead and return early
 	if len(fspRoots) == 0 {
-		return result, nil
+		return l.Roots, nil
 	}
 
 	//
 	// at this point we are handling filesystem path roots
 	//
 
-	// store the value of cfg.Dir so we can use it later if it is non-empty.
-	// we need to store it now as the value of cfg.Dir will be updated by
-	// a loop below
-	var cfgDir string
-	if cfg != nil {
-		cfgDir = cfg.Dir
-	}
-
 	// ensure the cfg.Dir field is reset to its original value upon
 	// returning from this function. it should honestly be fine if it is
 	// not given most callers will not send in the cfg parameter directly,
 	// as it's largely for testing, but still, let's be good stewards.
 	defer func(d string) {
-		if cfg != nil {
-			cfg.Dir = d
-		}
-	}(cfgDir)
+		cfg.Dir = d
+	}(cfg.Dir)
+
+	// store the value of cfg.Dir so we can use it later if it is non-empty.
+	// we need to store it now as the value of cfg.Dir will be updated by
+	// a loop below
+	cfgDir := cfg.Dir
 
 	// addNestedGoModulesToRoots is given to filepath.WalkDir and adds the
 	// directory part of p to the list of filesystem path roots IFF p is the
@@ -591,16 +576,9 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 			b = "."
 		}
 
-		// create the loader for this filesystem path. this is required in order
-		// to properly load the files as AST later in various workflows,
-		// including CRD generation
-		var fspLoader *loader
-		if cfg == nil {
-			fspLoader = newLoader(nil, d)
-		} else {
-			fspLoader = l
-			fspLoader.cfg.Dir = d
-		}
+		// update the loader configuration's Dir field to the directory part of
+		// the root
+		l.cfg.Dir = d
 
 		// update the root to be "./..." or "./."
 		// (with OS-specific filepath separator). please note filepath.Join
@@ -609,14 +587,14 @@ func LoadRootsWithConfig(cfg *packages.Config, roots ...string) (result []*Packa
 		r = fmt.Sprintf(".%s%s", string(filepath.Separator), b)
 
 		// load the packages from the roots
-		pkgs, err := loadPackages(fspLoader, r)
+		pkgs, err := loadPackages(r)
 		if err != nil {
 			return nil, err
 		}
-		result = append(result, pkgs...)
+		l.Roots = append(l.Roots, pkgs...)
 	}
 
-	return result, nil
+	return l.Roots, nil
 }
 
 // visitImports walks a dependency graph, replacing imported package
@@ -634,8 +612,8 @@ func visitImports(rootPkgs []*Package, pkg *Package, seen sets.String) {
 			}
 		}
 		if !seen.Has(importedPkgID) {
-			visitImports(rootPkgs, importedPkg, seen)
 			seen.Insert(importedPkgID)
+			visitImports(rootPkgs, importedPkg, seen)
 		}
 	}
 }