From 17bc6130ac2310dc8c911f45a359101c2b1d011a Mon Sep 17 00:00:00 2001 From: Doria Keung Date: Thu, 19 Dec 2024 15:57:39 -0500 Subject: [PATCH] Add `WithRelatedCheckConfigs` option to check additional check configs for unused plugins warning (#3550) --- .../buf/cmd/buf/command/breaking/breaking.go | 10 +++ .../buf/command/config/internal/internal.go | 9 +++ private/buf/cmd/buf/command/lint/lint.go | 10 +++ private/bufpkg/bufcheck/bufcheck.go | 19 ++++++ private/bufpkg/bufcheck/client.go | 32 ++++++++-- private/bufpkg/bufcheck/config.go | 6 +- private/bufpkg/bufcheck/rules_config.go | 61 ++++++++++--------- 7 files changed, 109 insertions(+), 38 deletions(-) diff --git a/private/buf/cmd/buf/command/breaking/breaking.go b/private/buf/cmd/buf/command/breaking/breaking.go index 1ddbf6359e..293649f1bb 100644 --- a/private/buf/cmd/buf/command/breaking/breaking.go +++ b/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()) diff --git a/private/buf/cmd/buf/command/config/internal/internal.go b/private/buf/cmd/buf/command/config/internal/internal.go index 66fb68c483..69e068e1cf 100644 --- a/private/buf/cmd/buf/command/config/internal/internal.go +++ b/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, diff --git a/private/buf/cmd/buf/command/lint/lint.go b/private/buf/cmd/buf/command/lint/lint.go index 98b8c6debf..2c32a12a0c 100644 --- a/private/buf/cmd/buf/command/lint/lint.go +++ b/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, diff --git a/private/bufpkg/bufcheck/bufcheck.go b/private/bufpkg/bufcheck/bufcheck.go index d5cee8cfc9..fd107c4ce4 100644 --- a/private/bufpkg/bufcheck/bufcheck.go +++ b/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) diff --git a/private/bufpkg/bufcheck/client.go b/private/bufpkg/bufcheck/client.go index bba8ed0f80..f6f823575a 100644 --- a/private/bufpkg/bufcheck/client.go +++ b/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,7 +477,8 @@ func checkCommentLineForCheckIgnore( } type lintOptions struct { - pluginConfigs []bufconfig.PluginConfig + pluginConfigs []bufconfig.PluginConfig + relatedCheckConfigs []bufconfig.CheckConfig } func newLintOptions() *lintOptions { @@ -484,8 +486,9 @@ func newLintOptions() *lintOptions { } type breakingOptions struct { - pluginConfigs []bufconfig.PluginConfig - excludeImports bool + pluginConfigs []bufconfig.PluginConfig + excludeImports bool + relatedCheckConfigs []bufconfig.CheckConfig } func newBreakingOptions() *breakingOptions { @@ -493,7 +496,8 @@ func newBreakingOptions() *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...) +} diff --git a/private/bufpkg/bufcheck/config.go b/private/bufpkg/bufcheck/config.go index c50b124a90..4bab6bf48e 100644 --- a/private/bufpkg/bufcheck/config.go +++ b/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 } diff --git a/private/bufpkg/bufcheck/rules_config.go b/private/bufpkg/bufcheck/rules_config.go index 85e9f99e62..6a9cefcf65 100644 --- a/private/bufpkg/bufcheck/rules_config.go +++ b/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 {