Surface module description in command outputs and errors when possible (

#3271)

private/buf/bufctl/controller.go

@@ -1023,6 +1023,7 @@ func (c *controller) buildTargetImageWithConfigs(
 		c.logger.Debug(
 			"building image for target module",
 			zap.String("moduleOpaqueID", module.OpaqueID()),
+			zap.String("moduleDescription", module.Description()),
 		)
 		opaqueID := module.OpaqueID()
 		// We need to make sure that all dependencies are non-targets, so that they

private/buf/bufworkspace/workspace_provider.go

@@ -396,6 +396,17 @@ func (w *workspaceProvider) getWorkspaceForBucketBufYAMLV2(
 			)
 		}
 	}
+	// Only check for duplicate module description in v2, which would be an user error, i.e.
+	// This is not a system error:
+	// modules:
+	//   - path: proto
+	//     excludes:
+	//       - proot/foo
+	//   - path: proto
+	//     excludes:
+	//       - proot/foo
+	// but duplicate module description in v1 is a system error, which the ModuleSetBuilder catches.
+	seenModuleDescriptions := make(map[string]struct{})
 	for _, moduleBucketAndTargeting := range v2Targeting.moduleBucketsAndTargeting {
 		mappedModuleBucket := moduleBucketAndTargeting.bucket
 		moduleTargeting := moduleBucketAndTargeting.moduleTargeting
@@ -408,6 +419,15 @@ func (w *workspaceProvider) getWorkspaceForBucketBufYAMLV2(
 			// configs, however, we return this error as a safety check
 			return nil, fmt.Errorf("no module config found for module at: %q", moduleTargeting.moduleDirPath)
 		}
+		moduleDescription := getLocalModuleDescription(
+			// See comments on getLocalModuleDescription.
+			moduleConfig.DirPath(),
+			moduleConfig,
+		)
+		if _, ok := seenModuleDescriptions[moduleDescription]; ok {
+			return nil, fmt.Errorf("multiple module configs found with the same description: %s", moduleDescription)
+		}
+		seenModuleDescriptions[moduleDescription] = struct{}{}
 		moduleSetBuilder.AddLocalModule(
 			mappedModuleBucket,
 			moduleBucketAndTargeting.bucketID,
@@ -421,13 +441,7 @@ func (w *workspaceProvider) getWorkspaceForBucketBufYAMLV2(
 				moduleTargeting.moduleProtoFileTargetPath,
 				moduleTargeting.includePackageFiles,
 			),
-			bufmodule.LocalModuleWithDescription(
-				getLocalModuleDescription(
-					// See comments on getLocalModuleDescription.
-					moduleConfig.DirPath(),
-					moduleConfig,
-				),
-			),
+			bufmodule.LocalModuleWithDescription(moduleDescription),
 		)
 	}
 	moduleSet, err := moduleSetBuilder.Build()

private/bufpkg/bufmodule/bufmodule_test.go

@@ -377,7 +377,7 @@ func TestModuleCycleError(t *testing.T) {
 			"buf.build/foo/c",
 			"buf.build/foo/a",
 		},
-		moduleCycleError.OpaqueIDs,
+		moduleCycleError.Descriptions,
 	)
 
 	moduleB := moduleSet.GetModuleForOpaqueID("buf.build/foo/b")
@@ -394,7 +394,7 @@ func TestModuleCycleError(t *testing.T) {
 			"buf.build/foo/a",
 			"buf.build/foo/b",
 		},
-		moduleCycleError.OpaqueIDs,
+		moduleCycleError.Descriptions,
 	)
 
 	moduleC := moduleSet.GetModuleForOpaqueID("buf.build/foo/c")
@@ -411,7 +411,7 @@ func TestModuleCycleError(t *testing.T) {
 			"buf.build/foo/b",
 			"buf.build/foo/c",
 		},
-		moduleCycleError.OpaqueIDs,
+		moduleCycleError.Descriptions,
 	)
 }
 

private/bufpkg/bufmodule/bufmoduleapi/registry.go

@@ -125,7 +125,7 @@ func getSingleRegistryForContentModules(contentModules []bufmodule.Module) (stri
 	for _, module := range contentModules {
 		moduleFullName := module.ModuleFullName()
 		if moduleFullName == nil {
-			return "", syserror.Newf("expected module name for %q", module.OpaqueID())
+			return "", syserror.Newf("expected module name for %s", module.Description())
 		}
 		moduleRegistry := moduleFullName.Registry()
 		if registry != "" && moduleRegistry != registry {

private/bufpkg/bufmodule/bufmoduleapi/uploader.go

@@ -112,23 +112,31 @@ func (a *uploader) Upload(
 	contentModules, err = slicesext.FilterError(contentModules, func(module bufmodule.Module) (bool, error) {
 		moduleName := module.ModuleFullName()
 		if moduleName == nil {
+			moduleDescription := module.Description()
 			if uploadOptions.ExcludeUnnamed() {
-				a.logger.Warn("Excluding unnamed module", zap.String("module", module.OpaqueID()))
+				a.logger.Warn("Excluding unnamed module", zap.String("module", moduleDescription))
 				return false, nil
 			}
-			return false, fmt.Errorf("A name must be specified in buf.yaml to push module %q.", module.OpaqueID())
+			return false, fmt.Errorf("a name must be specified in buf.yaml to push module: %s", moduleDescription)
 		}
 		deps, err := module.ModuleDeps()
 		if err != nil {
 			return false, err
 		}
-		if allDepModuleOpaqueIDs := slicesext.Reduce(deps, func(allDepModuleOpaqueIDs []string, dep bufmodule.ModuleDep) []string {
+		if allDepModuleDescriptions := slicesext.Reduce(deps, func(allDepModuleDescriptions []string, dep bufmodule.ModuleDep) []string {
 			if moduleName := dep.ModuleFullName(); moduleName == nil {
-				return append(allDepModuleOpaqueIDs, dep.OpaqueID())
+				return append(allDepModuleDescriptions, dep.Description())
 			}
-			return allDepModuleOpaqueIDs
-		}, nil); len(allDepModuleOpaqueIDs) > 0 {
-			return false, fmt.Errorf("All dependencies for module %q must be named but modules %s had no name.", moduleName.String(), strings.Join(allDepModuleOpaqueIDs, ", "))
+			return allDepModuleDescriptions
+		}, nil); len(allDepModuleDescriptions) > 0 {
+			return false, fmt.Errorf(
+				"all dependencies for module %q must be named but these modules are not:\n%s",
+				moduleName.String(),
+				strings.Join(
+					slicesext.Map(allDepModuleDescriptions, func(moduleDescription string) string { return "  " + moduleDescription }),
+					"\n",
+				),
+			)
 		}
 		return true, nil
 	})
@@ -449,7 +457,7 @@ func getV1Beta1ProtoUploadRequestContent(
 		return nil, syserror.New("expected local Module in getProtoLegacyFederationUploadRequestContent")
 	}
 	if module.ModuleFullName() == nil {
-		return nil, syserror.Newf("expected module name for local module %q", module.OpaqueID())
+		return nil, syserror.Newf("expected module name for local module: %s", module.Description())
 	}
 	if module.ModuleFullName().Registry() != primaryRegistry {
 		// This should never happen - the upload Modules should already be verified above to come from one registry.

private/bufpkg/bufmodule/errors.go

@@ -137,8 +137,8 @@ func (p *ParseError) Input() string {
 
 // ModuleCycleError is the error returned if a cycle is detected in module dependencies.
 type ModuleCycleError struct {
-	// OpaqueIDs are the OpaqueIDs that represent the cycle.
-	OpaqueIDs []string
+	// Descriptions are the module descriptions that represent the cycle.
+	Descriptions []string
 }
 
 // Error implements the error interface.
@@ -147,11 +147,16 @@ func (m *ModuleCycleError) Error() string {
 		return ""
 	}
 	var builder strings.Builder
-	_, _ = builder.WriteString(`cycle detected in module dependencies: `)
-	for i, opaqueID := range m.OpaqueIDs {
-		_, _ = builder.WriteString(opaqueID)
-		if i != len(m.OpaqueIDs)-1 {
-			_, _ = builder.WriteString(` -> `)
+	_, _ = builder.WriteString("cycle detected in module dependencies:\n")
+	for i, description := range m.Descriptions {
+		if i == 0 {
+			_, _ = builder.WriteString("    ")
+		} else {
+			_, _ = builder.WriteString(" -> ")
+		}
+		_, _ = builder.WriteString(description)
+		if i != len(m.Descriptions)-1 {
+			_, _ = builder.WriteString("\n")
 		}
 	}
 	return builder.String()

private/bufpkg/bufmodule/module.go

@@ -88,6 +88,23 @@ type Module interface {
 	//
 	// If ModuleFullName is nil, this will always be empty.
 	CommitID() uuid.UUID
+	// Description returns a human-readable description of the Module.
+	//
+	// This can be manually set by a constructor of a Module. In practice, the only current way
+	// to specifically set this string is by calling LocalModuleWithDescription when constructing
+	// a ModuleSet.
+	//
+	// This is used to construct descriptive error messages pointing to configured modules.
+	// For example, this may return something along the lines of:
+	//
+	//   path: proto/foo, includes; ["a", "b"], excludes: "c"
+	//
+	// The shape of this field should not be relied upon.
+	// This field will be unique within a given ModuleSet.
+	//
+	// This will never be empty. If a description was not explicitly set, this falls back to
+	// OpaqueID.
+	Description() string
 
 	// Digest returns the Module digest for the given DigestType.
 	//
@@ -170,26 +187,6 @@ type Module interface {
 	// Called in newModuleSet.
 	setModuleSet(ModuleSet)
 
-	// getDescription returns a human-readable description of the Module.
-	//
-	// This can be manually set by a constructor of a Module. In practice, the only current way
-	// to specifically set this string is by calling LocalModuleWithDescription when constructing
-	// a ModuleSet.
-	//
-	// This is used to construct descriptive error messages pointing to configured modules.
-	// For example, this may return something along the lines of:
-	//
-	//   path: proto/foo, includes; ["a", "b"], excludes: "c"
-	//
-	// The shape of this field should not be relied upon.
-	// This field will be unique within a given ModuleSet.
-	//
-	// This will never be empty. If a description was not explicitly set, this falls back to
-	// OpaqueID.
-	//
-	// Keeping this private for now. Change to Description if we ever want to make this public.
-	getDescription() string
-
 	// withIsTarget returns a copy of the Module with the specified target value.
 	//
 	// Do not expose publicly! This should only be called by ModuleSet.WithTargetOpaqueIDs.
@@ -358,6 +355,13 @@ func (m *module) CommitID() uuid.UUID {
 	return m.commitID
 }
 
+func (m *module) Description() string {
+	if m.description != "" {
+		return m.description
+	}
+	return m.OpaqueID()
+}
+
 func (m *module) Digest(digestType DigestType) (Digest, error) {
 	getDigest, ok := m.digestTypeToGetDigest[digestType]
 	if !ok {
@@ -419,13 +423,6 @@ func (m *module) setModuleSet(moduleSet ModuleSet) {
 	m.moduleSet = moduleSet
 }
 
-func (m *module) getDescription() string {
-	if m.description != "" {
-		return m.description
-	}
-	return m.OpaqueID()
-}
-
 func (*module) isModule() {}
 
 func newSyncOnceValueDigestTypeToGetDigestFuncForModule(module *module) map[DigestType]func() (Digest, error) {

private/bufpkg/bufmodule/module_dep.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
 	"github.com/bufbuild/buf/private/gen/data/datawkt"
+	"github.com/bufbuild/buf/private/pkg/slicesext"
 	"github.com/bufbuild/buf/private/pkg/syserror"
 )
 
@@ -85,7 +86,7 @@ func getModuleDeps(
 	if err := getModuleDepsRec(
 		ctx,
 		module,
-		make(map[string]struct{}),
+		make(map[string]string),
 		make(map[string]struct{}),
 		nil,
 		depOpaqueIDToModuleDep,
@@ -114,10 +115,10 @@ func getModuleDeps(
 func getModuleDepsRec(
 	ctx context.Context,
 	module Module,
-	visitedOpaqueIDs map[string]struct{},
+	visitedOpaqueIDToDescription map[string]string,
 	// Changes as we go down the stack.
 	parentOpaqueIDs map[string]struct{},
-	// Ordered version of parentOpaqueIDs so we can print a cycle error.
+	// Ordered (by dependency relationship) version of parentOpaqueIDs so we can print a cycle error.
 	orderedParentOpaqueIDs []string,
 	// Already discovered deps.
 	depOpaqueIDToModuleDep map[string]ModuleDep,
@@ -126,12 +127,19 @@ func getModuleDepsRec(
 ) error {
 	opaqueID := module.OpaqueID()
 	if _, ok := parentOpaqueIDs[opaqueID]; ok {
-		return &ModuleCycleError{OpaqueIDs: append(orderedParentOpaqueIDs, opaqueID)}
+		return &ModuleCycleError{
+			Descriptions: append(
+				slicesext.Map(orderedParentOpaqueIDs, func(parentOpaqueID string) string {
+					return visitedOpaqueIDToDescription[parentOpaqueID]
+				}),
+				module.Description(),
+			),
+		}
 	}
-	if _, ok := visitedOpaqueIDs[opaqueID]; ok {
+	if _, ok := visitedOpaqueIDToDescription[opaqueID]; ok {
 		return nil
 	}
-	visitedOpaqueIDs[opaqueID] = struct{}{}
+	visitedOpaqueIDToDescription[opaqueID] = module.Description()
 	moduleSet := module.ModuleSet()
 	if moduleSet == nil {
 		// This should never happen.
@@ -229,7 +237,7 @@ func getModuleDepsRec(
 		if err := getModuleDepsRec(
 			ctx,
 			newModuleDep,
-			visitedOpaqueIDs,
+			visitedOpaqueIDToDescription,
 			parentOpaqueIDs,
 			newOrderedParentOpaqueIDs,
 			depOpaqueIDToModuleDep,

private/bufpkg/bufmodule/module_set.go

@@ -269,7 +269,7 @@ func newModuleSet(
 			return nil, syserror.Newf("duplicate OpaqueID %q when constructing ModuleSet", opaqueID)
 		}
 		opaqueIDToModule[opaqueID] = module
-		description := module.getDescription()
+		description := module.Description()
 		if _, ok := descriptionToModule[description]; ok {
 			// This should never happen if we construct descriptions appropriately.
 			return nil, syserror.Newf("duplicate Description %q when constructing ModuleSet", description)
@@ -392,7 +392,7 @@ func (m *moduleSet) getModuleForFilePathUncached(ctx context.Context, filePath s
 			ModuleDescriptions: slicesext.ToUniqueSorted(
 				slicesext.Map(
 					slicesext.MapValuesToSlice(matchingOpaqueIDToModule),
-					Module.getDescription,
+					Module.Description,
 				),
 			),
 		}

private/bufpkg/bufmodule/proto_file_tracker.go

@@ -50,7 +50,7 @@ func (t *protoFileTracker) trackModule(module Module) {
 	if _, ok := t.opaqueIDToProtoFileExists[opaqueID]; !ok {
 		t.opaqueIDToProtoFileExists[opaqueID] = false
 	}
-	t.opaqueIDToDescription[opaqueID] = module.getDescription()
+	t.opaqueIDToDescription[opaqueID] = module.Description()
 }
 
 // trackFileInfo says to track the FileInfo to mark its associated Module as having .proto files
@@ -69,7 +69,7 @@ func (t *protoFileTracker) trackFileInfo(fileInfo FileInfo) {
 		t.protoPathToOpaqueIDMap[fileInfo.Path()] = protoPathOpaqueIDMap
 	}
 	protoPathOpaqueIDMap[opaqueID] = struct{}{}
-	t.opaqueIDToDescription[opaqueID] = module.getDescription()
+	t.opaqueIDToDescription[opaqueID] = module.Description()
 }
 
 // validate validates. This should be called when all tracking is complete.