Add WithRelatedCheckConfigs option to check additional check configs for unused plugins warning (#3550)

Author: doriable

private/buf/cmd/buf/command/breaking/breaking.go

@@ -24,6 +24,7 @@ import (
 	"github.com/bufbuild/buf/private/buf/buffetch"
 	"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
 	"github.com/bufbuild/buf/private/bufpkg/bufcheck"
+	"github.com/bufbuild/buf/private/bufpkg/bufconfig"
 	"github.com/bufbuild/buf/private/bufpkg/bufimage"
 	"github.com/bufbuild/buf/private/pkg/app/appcmd"
 	"github.com/bufbuild/buf/private/pkg/app/appext"
@@ -218,10 +219,19 @@ func run(
 			len(againstImageWithConfigs),
 		)
 	}
+	// We add all check configs (both lint and breaking) as related configs to check if plugins
+	// have rules configured.
+	// We allocated twice the size of imageWithConfigs for both lint and breaking configs.
+	allCheckConfigs := make([]bufconfig.CheckConfig, 0, len(imageWithConfigs)*2)
+	for _, imageWithConfig := range imageWithConfigs {
+		allCheckConfigs = append(allCheckConfigs, imageWithConfig.LintConfig())
+		allCheckConfigs = append(allCheckConfigs, imageWithConfig.BreakingConfig())
+	}
 	var allFileAnnotations []bufanalysis.FileAnnotation
 	for i, imageWithConfig := range imageWithConfigs {
 		breakingOptions := []bufcheck.BreakingOption{
 			bufcheck.WithPluginConfigs(imageWithConfig.PluginConfigs()...),
+			bufcheck.WithRelatedCheckConfigs(allCheckConfigs...),
 		}
 		if flags.ExcludeImports {
 			breakingOptions = append(breakingOptions, bufcheck.BreakingWithExcludeImports())

private/buf/cmd/buf/command/config/internal/internal.go

@@ -243,6 +243,14 @@ func lsRun(
 			return syserror.Newf("unknown FileVersion: %v", fileVersion)
 		}
 		var checkConfig bufconfig.CheckConfig
+		// We add all check configs (both lint and breaking) as related configs to check if plugins
+		// have rules configured.
+		// We allocated twice the size of moduleConfigs for both lint and breaking configs.
+		allCheckConfigs := make([]bufconfig.CheckConfig, 0, len(moduleConfigs)*2)
+		for _, moduleConfig := range moduleConfigs {
+			allCheckConfigs = append(allCheckConfigs, moduleConfig.LintConfig())
+			allCheckConfigs = append(allCheckConfigs, moduleConfig.BreakingConfig())
+		}
 		switch ruleType {
 		case check.RuleTypeLint:
 			checkConfig = moduleConfig.LintConfig()
@@ -253,6 +261,7 @@ func lsRun(
 		}
 		configuredRuleOptions := []bufcheck.ConfiguredRulesOption{
 			bufcheck.WithPluginConfigs(bufYAMLFile.PluginConfigs()...),
+			bufcheck.WithRelatedCheckConfigs(allCheckConfigs...),
 		}
 		rules, err = checkClient.ConfiguredRules(
 			ctx,

private/buf/cmd/buf/command/lint/lint.go

@@ -23,6 +23,7 @@ import (
 	"github.com/bufbuild/buf/private/buf/bufctl"
 	"github.com/bufbuild/buf/private/bufpkg/bufanalysis"
 	"github.com/bufbuild/buf/private/bufpkg/bufcheck"
+	"github.com/bufbuild/buf/private/bufpkg/bufconfig"
 	"github.com/bufbuild/buf/private/pkg/app/appcmd"
 	"github.com/bufbuild/buf/private/pkg/app/appext"
 	"github.com/bufbuild/buf/private/pkg/stringutil"
@@ -143,9 +144,18 @@ func run(
 		return err
 	}
 	var allFileAnnotations []bufanalysis.FileAnnotation
+	// We add all check configs (both lint and breaking) as related configs to check if plugins
+	// have rules configured.
+	// We allocated twice the size of imageWithConfigs for both lint and breaking configs.
+	allCheckConfigs := make([]bufconfig.CheckConfig, 0, len(imageWithConfigs)*2)
+	for _, imageWithConfig := range imageWithConfigs {
+		allCheckConfigs = append(allCheckConfigs, imageWithConfig.LintConfig())
+		allCheckConfigs = append(allCheckConfigs, imageWithConfig.BreakingConfig())
+	}
 	for _, imageWithConfig := range imageWithConfigs {
 		lintOptions := []bufcheck.LintOption{
 			bufcheck.WithPluginConfigs(imageWithConfig.PluginConfigs()...),
+			bufcheck.WithRelatedCheckConfigs(allCheckConfigs...),
 		}
 		if err := checkClient.Lint(
 			ctx,

private/bufpkg/bufcheck/bufcheck.go

@@ -127,6 +127,25 @@ type ConfiguredRulesOption interface {
 	applyToConfiguredRules(*configuredRulesOptions)
 }
 
+// LintBreakingAndConfiguredRulesOption is an option for Lint, Breaking, and ConfiguredRules.
+type LintBreakingAndConfiguredRulesOption interface {
+	LintOption
+	BreakingOption
+	ConfiguredRulesOption
+}
+
+// WithRelatedCheckConfigs returns a new LintBreakingAndConfiguredRulesOption that allows
+// the caller to provide additional related check configs. This allows the client to check
+// for unused plugins across related check configs and provide users with a warning if the
+// plugin is unused in all check configs.
+//
+// The default is to only check the configs provided to the client for Lint, Breaking, or ConfiguredRules.
+func WithRelatedCheckConfigs(relatedCheckConfigs ...bufconfig.CheckConfig) LintBreakingAndConfiguredRulesOption {
+	return &relatedCheckConfigsOption{
+		relatedCheckConfigs: relatedCheckConfigs,
+	}
+}
+
 // AllRulesOption is an option for AllRules.
 type AllRulesOption interface {
 	applyToAllRules(*allRulesOptions)

private/bufpkg/bufcheck/client.go

@@ -104,7 +104,7 @@ func (c *client) Lint(
 	if err != nil {
 		return err
 	}
-	config, err := configForLintConfig(lintConfig, allRules, allCategories)
+	config, err := configForLintConfig(lintConfig, allRules, allCategories, lintOptions.relatedCheckConfigs)
 	if err != nil {
 		return err
 	}
@@ -168,6 +168,7 @@ func (c *client) Breaking(
 		allRules,
 		allCategories,
 		breakingOptions.excludeImports,
+		breakingOptions.relatedCheckConfigs,
 	)
 	if err != nil {
 		return err
@@ -229,7 +230,7 @@ func (c *client) ConfiguredRules(
 	if err != nil {
 		return nil, err
 	}
-	rulesConfig, err := rulesConfigForCheckConfig(checkConfig, allRules, allCategories, ruleType)
+	rulesConfig, err := rulesConfigForCheckConfig(checkConfig, allRules, allCategories, ruleType, configuredRulesOptions.relatedCheckConfigs)
 	if err != nil {
 		return nil, err
 	}
@@ -476,24 +477,27 @@ func checkCommentLineForCheckIgnore(
 }
 
 type lintOptions struct {
-	pluginConfigs []bufconfig.PluginConfig
+	pluginConfigs       []bufconfig.PluginConfig
+	relatedCheckConfigs []bufconfig.CheckConfig
 }
 
 func newLintOptions() *lintOptions {
 	return &lintOptions{}
 }
 
 type breakingOptions struct {
-	pluginConfigs  []bufconfig.PluginConfig
-	excludeImports bool
+	pluginConfigs       []bufconfig.PluginConfig
+	excludeImports      bool
+	relatedCheckConfigs []bufconfig.CheckConfig
 }
 
 func newBreakingOptions() *breakingOptions {
 	return &breakingOptions{}
 }
 
 type configuredRulesOptions struct {
-	pluginConfigs []bufconfig.PluginConfig
+	pluginConfigs       []bufconfig.PluginConfig
+	relatedCheckConfigs []bufconfig.CheckConfig
 }
 
 func newConfiguredRulesOptions() *configuredRulesOptions {
@@ -553,3 +557,19 @@ func (p *pluginConfigsOption) applyToAllRules(allRulesOptions *allRulesOptions)
 func (p *pluginConfigsOption) applyToAllCategories(allCategoriesOptions *allCategoriesOptions) {
 	allCategoriesOptions.pluginConfigs = append(allCategoriesOptions.pluginConfigs, p.pluginConfigs...)
 }
+
+type relatedCheckConfigsOption struct {
+	relatedCheckConfigs []bufconfig.CheckConfig
+}
+
+func (r *relatedCheckConfigsOption) applyToLint(lintOptions *lintOptions) {
+	lintOptions.relatedCheckConfigs = append(lintOptions.relatedCheckConfigs, r.relatedCheckConfigs...)
+}
+
+func (r *relatedCheckConfigsOption) applyToBreaking(breakingOptions *breakingOptions) {
+	breakingOptions.relatedCheckConfigs = append(breakingOptions.relatedCheckConfigs, r.relatedCheckConfigs...)
+}
+
+func (r *relatedCheckConfigsOption) applyToConfiguredRules(configuredRulesOptions *configuredRulesOptions) {
+	configuredRulesOptions.relatedCheckConfigs = append(configuredRulesOptions.relatedCheckConfigs, r.relatedCheckConfigs...)
+}

private/bufpkg/bufcheck/config.go

@@ -28,8 +28,9 @@ func configForLintConfig(
 	lintConfig bufconfig.LintConfig,
 	allRules []Rule,
 	allCategories []Category,
+	relatedCheckConfigs []bufconfig.CheckConfig,
 ) (*config, error) {
-	rulesConfig, err := rulesConfigForCheckConfig(lintConfig, allRules, allCategories, check.RuleTypeLint)
+	rulesConfig, err := rulesConfigForCheckConfig(lintConfig, allRules, allCategories, check.RuleTypeLint, relatedCheckConfigs)
 	if err != nil {
 		return nil, err
 	}
@@ -48,8 +49,9 @@ func configForBreakingConfig(
 	allRules []Rule,
 	allCategories []Category,
 	excludeImports bool,
+	relatedCheckConfigs []bufconfig.CheckConfig,
 ) (*config, error) {
-	rulesConfig, err := rulesConfigForCheckConfig(breakingConfig, allRules, allCategories, check.RuleTypeBreaking)
+	rulesConfig, err := rulesConfigForCheckConfig(breakingConfig, allRules, allCategories, check.RuleTypeBreaking, relatedCheckConfigs)
 	if err != nil {
 		return nil, err
 	}

private/bufpkg/bufcheck/rules_config.go

@@ -33,6 +33,7 @@ func rulesConfigForCheckConfig(
 	allRules []Rule,
 	allCategories []Category,
 	ruleType check.RuleType,
+	relatedCheckConfigs []bufconfig.CheckConfig,
 ) (*rulesConfig, error) {
 	return newRulesConfig(
 		checkConfig.UseIDsAndCategories(),
@@ -42,6 +43,7 @@ func rulesConfigForCheckConfig(
 		allRules,
 		allCategories,
 		ruleType,
+		relatedCheckConfigs,
 	)
 }
 
@@ -94,13 +96,9 @@ type rulesConfig struct {
 	//
 	// A plugin is unused if no rules from the plugin are configured.
 	//
-	// This map will *not* contain plugins that have Rules with RuleTypes other than the given
-	// RuleType. We need to account for this to properly print warnings. It is possible that
-	// a plugin is not used in the lint section, for example, but does have breaking rules configured.
-	// In client.Lint and client.Breaking, we only have the Lint or Breaking config, and we don't know
-	// the state of the other config. If a plugin is unused for lint, but has both lint and breaking
-	// Rules, we don't warn for this plugin, as it may have had rules configured in breaking that
-	// we haven't accounted for.
+	// The caller can provide additional related check configs to check if the plugin has rules
+	// configured. If the plugin has rules configured in any of the additional check configs
+	// provided, then we don't warn.
 	//
 	// The Rule IDs will be sorted.
 	// This will only contain RuleIDs of the given RuleType.
@@ -124,6 +122,7 @@ func newRulesConfig(
 	allRules []Rule,
 	allCategories []Category,
 	ruleType check.RuleType,
+	relatedCheckConfigs []bufconfig.CheckConfig,
 ) (*rulesConfig, error) {
 	allRulesForType := rulesForType(allRules, ruleType)
 	if len(allRulesForType) == 0 {
@@ -292,7 +291,6 @@ func newRulesConfig(
 		return nil, err
 	}
 
-	pluginNameToOtherRuleTypes := getPluginNameToOtherRuleTypes(allRules, ruleType)
 	// This map initially contains a map from plugin name to ALL Rule IDs, but we will
 	// then delete the used plugin names, and then delete the plugins with other rule types.
 	//
@@ -305,11 +303,31 @@ func newRulesConfig(
 			delete(unusedPluginNameToRuleIDs, pluginName)
 		}
 	}
-	for pluginName := range unusedPluginNameToRuleIDs {
-		// If the rule had other plugin types (see the comment on UnusedPluginNameToRuleIDs),
-		// delete the plugin name from the map
-		if _, ok := pluginNameToOtherRuleTypes[pluginName]; ok {
-			delete(unusedPluginNameToRuleIDs, pluginName)
+	// We check additional check configs. If rules are set in the related check configs, then
+	// the plugin is not considered unused. We check against all rules for all rule types.
+	allRuleIDToRule, err := getIDToRuleOrCategory(allRules)
+	if err != nil {
+		return nil, err
+	}
+	allRuleIDToCategoryIDs, err := getRuleIDToCategoryIDs(allRules)
+	if err != nil {
+		return nil, err
+	}
+	allCategoryIDToRuleIDs := getCategoryIDToRuleIDs(allRuleIDToCategoryIDs)
+	for _, checkConfig := range relatedCheckConfigs {
+		checkConfigUseRuleIDs, err := transformRuleOrCategoryIDsToRuleIDs(
+			checkConfig.UseIDsAndCategories(),
+			allRuleIDToCategoryIDs,
+			allCategoryIDToRuleIDs,
+		)
+		if err != nil {
+			return nil, err
+		}
+		for _, checkConfigRuleID := range checkConfigUseRuleIDs {
+			// If a rule from a non-builtin plugin is found, then we remove it from the unused plugins.
+			if rule, ok := allRuleIDToRule[checkConfigRuleID]; rule.PluginName() != "" && ok {
+				delete(unusedPluginNameToRuleIDs, rule.PluginName())
+			}
 		}
 	}
 
@@ -422,23 +440,6 @@ func getIDToRuleOrCategory[R RuleOrCategory](ruleOrCategories []R) (map[string]R
 	return m, nil
 }
 
-func getPluginNameToOtherRuleTypes(allRules []Rule, ruleType check.RuleType) map[string]map[check.RuleType]struct{} {
-	m := make(map[string]map[check.RuleType]struct{})
-	for _, rule := range allRules {
-		if pluginName := rule.PluginName(); pluginName != "" {
-			if rule.Type() != ruleType {
-				otherRuleTypes, ok := m[pluginName]
-				if !ok {
-					otherRuleTypes = make(map[check.RuleType]struct{})
-					m[pluginName] = otherRuleTypes
-				}
-				otherRuleTypes[rule.Type()] = struct{}{}
-			}
-		}
-	}
-	return m
-}
-
 func getPluginNameToRuleOrCategoryIDs[R RuleOrCategory](ruleOrCategories []R) map[string][]string {
 	m := make(map[string][]string)
 	for _, ruleOrCategory := range ruleOrCategories {