[dev.cmdgo] cmd/go: clean up TODOWorkspaces instances

Address some of the easier todos to address and remove the todos that
have already been done and redundant todos.

For #45713

Change-Id: I3fe4393168b10c6e005325258d9701713c92e9e4
Reviewed-on: https://go-review.googlesource.com/c/go/+/344491
Trust: Michael Matloob <matloob@golang.org>
Run-TryBot: Michael Matloob <matloob@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Jay Conrod <jayconrod@google.com>

src/cmd/go/internal/modcmd/download.go

@@ -97,7 +97,7 @@ func runDownload(ctx context.Context, cmd *base.Command, args []string) {
 		modload.LoadModFile(ctx) // to fill MainModules
 
 		if len(modload.MainModules.Versions()) != 1 {
-			panic(modload.TODOWorkspaces("TODO: multiple main modules not supported in Download"))
+			panic(modload.TODOWorkspaces("Support workspace mode in go mod download"))
 		}
 		mainModule := modload.MainModules.Versions()[0]
 

src/cmd/go/internal/modcmd/initwork.go

@@ -13,9 +13,9 @@ import (
 	"path/filepath"
 )
 
-var _ = modload.TODOWorkspaces("Add more documentation below.T hough this is" +
+var _ = modload.TODOWorkspaces("Add more documentation below. Though this is" +
 	"enough for those trying workspaces out, there should be more through" +
-	"documentation if the proposal is accepted.")
+	"documentation if the proposal is accepted and released.")
 
 var cmdInitwork = &base.Command{
 	UsageLine: "go mod initwork [moddirs]",

src/cmd/go/internal/modget/get.go

@@ -690,7 +690,7 @@ func (r *resolver) queryNone(ctx context.Context, q *query) {
 				// However, neither of those behaviors would be consistent with the
 				// plain meaning of the query. To try to reduce confusion, reject the
 				// query explicitly.
-				return errSet(&modload.QueryMatchesMainModuleError{MainModule: v, Pattern: q.pattern, Query: q.version})
+				return errSet(&modload.QueryMatchesMainModulesError{MainModules: []module.Version{v}, Pattern: q.pattern, Query: q.version})
 			}
 
 			return pathSet{mod: module.Version{Path: q.pattern, Version: "none"}}
@@ -703,7 +703,7 @@ func (r *resolver) queryNone(ctx context.Context, q *query) {
 		}
 		q.pathOnce(curM.Path, func() pathSet {
 			if modload.HasModRoot() && curM.Version == "" && modload.MainModules.Contains(curM.Path) {
-				return errSet(&modload.QueryMatchesMainModuleError{MainModule: curM, Pattern: q.pattern, Query: q.version})
+				return errSet(&modload.QueryMatchesMainModulesError{MainModules: []module.Version{curM}, Pattern: q.pattern, Query: q.version})
 			}
 			return pathSet{mod: module.Version{Path: curM.Path, Version: "none"}}
 		})
@@ -805,8 +805,8 @@ func (r *resolver) queryWildcard(ctx context.Context, q *query) {
 
 			if modload.MainModules.Contains(curM.Path) && !versionOkForMainModule(q.version) {
 				if q.matchesPath(curM.Path) {
-					return errSet(&modload.QueryMatchesMainModuleError{
+					return errSet(&modload.QueryMatchesMainModulesError{
-						MainModule: curM,
+						MainModules: []module.Version{curM},
 						Pattern:     q.pattern,
 						Query:       q.version,
 					})
@@ -1760,8 +1760,8 @@ func (r *resolver) resolve(q *query, m module.Version) {
 	}
 
 	if modload.MainModules.Contains(m.Path) && m.Version != "" {
-		reportError(q, &modload.QueryMatchesMainModuleError{
+		reportError(q, &modload.QueryMatchesMainModulesError{
-			MainModule: module.Version{Path: m.Path},
+			MainModules: []module.Version{{Path: m.Path}},
 			Pattern:     q.pattern,
 			Query:       q.version,
 		})

src/cmd/go/internal/modload/build.go

@@ -218,7 +218,6 @@ func moduleInfo(ctx context.Context, rs *Requirements, m module.Version, mode Li
 			Version: m.Version,
 			Main:    true,
 		}
-		_ = TODOWorkspaces("handle rawGoVersion here")
 		if v, ok := rawGoVersion.Load(m); ok {
 			info.GoVersion = v.(string)
 		} else {

src/cmd/go/internal/modload/buildlist.go

@@ -139,7 +139,7 @@ func (rs *Requirements) initVendor(vendorList []module.Version) {
 		}
 
 		if MainModules.Len() != 1 {
-			panic("There should be exactly one main moudle in Vendor mode.")
+			panic("There should be exactly one main module in Vendor mode.")
 		}
 		mainModule := MainModules.Versions()[0]
 
@@ -284,8 +284,10 @@ func readModGraph(ctx context.Context, depth modDepth, roots []module.Version) (
 	)
 	for _, m := range MainModules.Versions() {
 		// Require all roots from all main modules.
-		_ = TODOWorkspaces("This isn't the correct behavior. " +
+		_ = TODOWorkspaces("This flattens a level of the module graph, adding the dependencies " +
-			"Fix this when the requirements struct is updated to reflect the struct of the module graph.")
+			"of all main modules to a single requirements struct, and losing the information of which " +
+			"main module required which requirement. Rework the requirements struct and change this" +
+			"to reflect the structure of the main modules.")
 		mg.g.Require(m, roots)
 	}
 

src/cmd/go/internal/modload/init.go

@@ -123,8 +123,6 @@ func (mms *MainModuleSet) Contains(path string) bool {
 }
 
 func (mms *MainModuleSet) ModRoot(m module.Version) string {
-	_ = TODOWorkspaces(" Do we need the Init? The original modRoot calls it. Audit callers.")
-	Init()
 	if mms == nil {
 		return ""
 	}
@@ -143,8 +141,11 @@ func (mms *MainModuleSet) mustGetSingleMainModule() module.Version {
 		panic("internal error: mustGetSingleMainModule called in context with no main modules")
 	}
 	if len(mms.versions) != 1 {
-		_ = TODOWorkspaces("Check if we're in workspace mode before returning the below error.")
+		if inWorkspaceMode() {
 			panic("internal error: mustGetSingleMainModule called in workspace mode")
+		} else {
+			panic("internal error: multiple main modules present outside of workspace mode")
+		}
 	}
 	return mms.versions[0]
 }
@@ -156,11 +157,7 @@ func (mms *MainModuleSet) GetSingleIndexOrNil() *modFileIndex {
 	if len(mms.versions) == 0 {
 		return nil
 	}
-	if len(mms.versions) != 1 {
+	return mms.indices[mms.mustGetSingleMainModule()]
-		_ = TODOWorkspaces("Check if we're in workspace mode before returning the below error.")
-		panic("internal error: mustGetSingleMainModule called in workspace mode")
-	}
-	return mms.indices[mms.versions[0]]
 }
 
 func (mms *MainModuleSet) Index(m module.Version) *modFileIndex {
@@ -363,7 +360,9 @@ func Init() {
 	// We're in module mode. Set any global variables that need to be set.
 	cfg.ModulesEnabled = true
 	setDefaultBuildMod()
-	_ = TODOWorkspaces("ensure that buildmod is readonly")
+	_ = TODOWorkspaces("In workspace mode, mod will not be readonly for go mod download," +
+		"verify, graph, and why. Implement support for go mod download and add test cases" +
+		"to ensure verify, graph, and why work properly.")
 	list := filepath.SplitList(cfg.BuildContext.GOPATH)
 	if len(list) == 0 || list[0] == "" {
 		base.Fatalf("missing $GOPATH")
@@ -374,15 +373,14 @@ func Init() {
 	}
 
 	if inWorkspaceMode() {
-		_ = TODOWorkspaces("go.work.sum, and also allow modfetch to fall back to individual go.sums")
-		_ = TODOWorkspaces("replaces")
 		var err error
 		modRoots, err = loadWorkFile(workFilePath)
 		if err != nil {
 			base.Fatalf("reading go.work: %v", err)
 		}
+		_ = TODOWorkspaces("Support falling back to individual module go.sum " +
+			"files for sums not in the workspace sum file.")
 		modfetch.GoSumFile = workFilePath + ".sum"
-		// TODO(matloob) should workRoot just be workFile?
 	} else if modRoots == nil {
 		// We're in module mode, but not inside a module.
 		//
@@ -539,6 +537,7 @@ func (goModDirtyError) Error() string {
 var errGoModDirty error = goModDirtyError{}
 
 func loadWorkFile(path string) (modRoots []string, err error) {
+	_ = TODOWorkspaces("Clean up and write back the go.work file: add module paths for workspace modules.")
 	workDir := filepath.Dir(path)
 	workData, err := lockedfile.Read(path)
 	if err != nil {
@@ -661,8 +660,7 @@ func loadModFile(ctx context.Context) (rs *Requirements, needCommit bool) {
 		// We don't need to do anything for vendor or update the mod file so
 		// return early.
 
-		_ = TODOWorkspaces("don't worry about commits for now, but eventually will want to update go.work files")
+		return rs, true
-		return rs, false
 	}
 
 	mainModule := MainModules.mustGetSingleMainModule()
@@ -761,7 +759,7 @@ func CreateModFile(ctx context.Context, modPath string) {
 	MainModules = makeMainModules([]module.Version{modFile.Module.Mod}, []string{modRoot}, []*modfile.File{modFile}, []*modFileIndex{nil})
 	addGoStmt(modFile, modFile.Module.Mod, LatestGoVersion()) // Add the go directive before converted module requirements.
 
-	convertedFrom, err := convertLegacyConfig(modFile, modPath)
+	convertedFrom, err := convertLegacyConfig(modFile, modRoot)
 	if convertedFrom != "" {
 		fmt.Fprintf(os.Stderr, "go: copying requirements from %s\n", base.ShortPath(convertedFrom))
 	}
@@ -1037,7 +1035,7 @@ func mustHaveCompleteRequirements() bool {
 
 // convertLegacyConfig imports module requirements from a legacy vendoring
 // configuration file, if one is present.
-func convertLegacyConfig(modFile *modfile.File, modPath string) (from string, err error) {
+func convertLegacyConfig(modFile *modfile.File, modRoot string) (from string, err error) {
 	noneSelected := func(path string) (version string) { return "none" }
 	queryPackage := func(path, rev string) (module.Version, error) {
 		pkgMods, modOnly, err := QueryPattern(context.Background(), path, rev, noneSelected, nil)
@@ -1050,10 +1048,7 @@ func convertLegacyConfig(modFile *modfile.File, modPath string) (from string, er
 		return modOnly.Mod, nil
 	}
 	for _, name := range altConfigs {
-		if len(modRoots) != 1 {
+		cfg := filepath.Join(modRoot, name)
-			panic(TODOWorkspaces("what do do here?"))
-		}
-		cfg := filepath.Join(modRoots[0], name)
 		data, err := os.ReadFile(cfg)
 		if err == nil {
 			convert := modconv.Converters[name]
@@ -1166,7 +1161,8 @@ func findWorkspaceFile(dir string) (root string) {
 			break
 		}
 		if d == cfg.GOROOT {
-			_ = TODOWorkspaces("Address how go.work files interact with GOROOT")
+			_ = TODOWorkspaces("If we end up checking in a go.work file to GOROOT/src," +
+				"remove this case.")
 			return "" // As a special case, don't cross GOROOT to find a go.work file.
 		}
 		dir = d
@@ -1345,7 +1341,7 @@ func commitRequirements(ctx context.Context, goVersion string, rs *Requirements)
 		// We aren't in a module, so we don't have anywhere to write a go.mod file.
 		return
 	}
-	mainModule := MainModules.Versions()[0]
+	mainModule := MainModules.mustGetSingleMainModule()
 	modFilePath := modFilePath(MainModules.ModRoot(mainModule))
 	modFile := MainModules.ModFile(mainModule)
 
@@ -1398,12 +1394,6 @@ func commitRequirements(ctx context.Context, goVersion string, rs *Requirements)
 		base.Fatalf("go: %v", err)
 	}
 	defer func() {
-		if MainModules.Len() != 1 {
-			panic(TODOWorkspaces("There should be exactly one main module when committing reqs"))
-		}
-
-		mainModule := MainModules.Versions()[0]
-
 		// At this point we have determined to make the go.mod file on disk equal to new.
 		MainModules.SetIndex(mainModule, indexModFile(new, modFile, mainModule, false))
 

src/cmd/go/internal/modload/load.go

@@ -552,8 +552,7 @@ func resolveLocalPackage(ctx context.Context, dir string, rs *Requirements) (str
 				// return an error.
 				if len(mainModulePrefix) > len(pkgNotFoundLongestPrefix) {
 					pkgNotFoundLongestPrefix = mainModulePrefix
-					pkgNotFoundErr = &PackageNotInModuleError{Mod: mainModule, Pattern: pkg}
+					pkgNotFoundErr = &PackageNotInModuleError{MainModules: []module.Version{mainModule}, Pattern: pkg}
-
 				}
 				continue
 			}

src/cmd/go/internal/modload/modfile.go

@@ -322,7 +322,7 @@ func replacement(mod module.Version, index *modFileIndex) (fromVersion string, t
 // If there is no replacement for mod, Replacement returns
 // a module.Version with Path == "".
 func Replacement(mod module.Version) (module.Version, string) {
-	_ = TODOWorkspaces("support replaces in the go.work file")
+	_ = TODOWorkspaces("Support replaces in the go.work file.")
 	foundFrom, found, foundModRoot := "", module.Version{}, ""
 	for _, v := range MainModules.Versions() {
 		if index := MainModules.Index(v); index != nil {

src/cmd/go/internal/modload/query.go

@@ -632,16 +632,14 @@ func QueryPattern(ctx context.Context, pattern, query string, current func(strin
 		if modOnly != nil {
 			return nil, modOnly, nil
 		} else if len(mainModuleMatches) != 0 {
-			_ = TODOWorkspaces("add multiple main modules to the error?")
+			return nil, nil, &QueryMatchesMainModulesError{
-			return nil, nil, &QueryMatchesMainModuleError{
+				MainModules: mainModuleMatches,
-				MainModule: mainModuleMatches[0],
 				Pattern:     pattern,
 				Query:       query,
 			}
 		} else {
-			_ = TODOWorkspaces("This should maybe be PackageNotInModule*s* error with the main modules that are prefixes of base")
 			return nil, nil, &PackageNotInModuleError{
-				Mod:     MainModules.Versions()[0],
+				MainModules: mainModuleMatches,
 				Query:       query,
 				Pattern:     pattern,
 			}
@@ -695,7 +693,7 @@ func QueryPattern(ctx context.Context, pattern, query string, current func(strin
 	})
 
 	if len(mainModuleMatches) > 0 && len(results) == 0 && modOnly == nil && errors.Is(err, fs.ErrNotExist) {
-		return nil, nil, &QueryMatchesMainModuleError{
+		return nil, nil, &QueryMatchesMainModulesError{
 			Pattern: pattern,
 			Query:   query,
 		}
@@ -893,6 +891,7 @@ func (e *WildcardInFirstElementError) Error() string {
 // code for the versions it knows about, and thus did not have the opportunity
 // to return a non-400 status code to suppress fallback.
 type PackageNotInModuleError struct {
+	MainModules []module.Version
 	Mod         module.Version
 	Replacement module.Version
 	Query       string
@@ -900,11 +899,15 @@ type PackageNotInModuleError struct {
 }
 
 func (e *PackageNotInModuleError) Error() string {
-	if MainModules.Contains(e.Mod.Path) {
+	if len(e.MainModules) > 0 {
-		if strings.Contains(e.Pattern, "...") {
+		prefix := "workspace modules do"
-			return fmt.Sprintf("main module (%s) does not contain packages matching %s", e.Mod.Path, e.Pattern)
+		if len(e.MainModules) == 1 {
+			prefix = fmt.Sprintf("main module (%s) does", e.MainModules[0])
 		}
-		return fmt.Sprintf("main module (%s) does not contain package %s", e.Mod.Path, e.Pattern)
+		if strings.Contains(e.Pattern, "...") {
+			return fmt.Sprintf("%s not contain packages matching %s", prefix, e.Pattern)
+		}
+		return fmt.Sprintf("%s not contain package %s", prefix, e.Pattern)
 	}
 
 	found := ""
@@ -1153,21 +1156,29 @@ func (rr *replacementRepo) replacementStat(v string) (*modfetch.RevInfo, error)
 	return rev, nil
 }
 
-// A QueryMatchesMainModuleError indicates that a query requests
+// A QueryMatchesMainModulesError indicates that a query requests
 // a version of the main module that cannot be satisfied.
 // (The main module's version cannot be changed.)
-type QueryMatchesMainModuleError struct {
+type QueryMatchesMainModulesError struct {
-	MainModule module.Version
+	MainModules []module.Version
 	Pattern     string
 	Query       string
 }
 
-func (e *QueryMatchesMainModuleError) Error() string {
+func (e *QueryMatchesMainModulesError) Error() string {
 	if MainModules.Contains(e.Pattern) {
 		return fmt.Sprintf("can't request version %q of the main module (%s)", e.Query, e.Pattern)
 	}
 
-	return fmt.Sprintf("can't request version %q of pattern %q that includes the main module (%s)", e.Query, e.Pattern, e.MainModule.Path)
+	plural := ""
+	mainModulePaths := make([]string, len(e.MainModules))
+	for i := range e.MainModules {
+		mainModulePaths[i] = e.MainModules[i].Path
+	}
+	if len(e.MainModules) > 1 {
+		plural = "s"
+	}
+	return fmt.Sprintf("can't request version %q of pattern %q that includes the main module%s (%s)", e.Query, e.Pattern, plural, strings.Join(mainModulePaths, ", "))
 }
 
 // A QueryUpgradesAllError indicates that a query requests