From e5d360e84fb8ac6db66b4ff6a0aa8a4c5c1c12df Mon Sep 17 00:00:00 2001 From: Doria Keung Date: Wed, 18 Dec 2024 14:49:53 -0500 Subject: [PATCH] Remove heuristic for unused plugin warning and rely on related check configs. --- .../buf/cmd/buf/command/breaking/breaking.go | 20 ++++-- .../buf/command/config/internal/internal.go | 10 +-- private/buf/cmd/buf/command/lint/lint.go | 20 ++++-- private/bufpkg/bufcheck/config.go | 8 +-- private/bufpkg/bufcheck/rules_config.go | 67 ++++++------------- 5 files changed, 61 insertions(+), 64 deletions(-) diff --git a/private/buf/cmd/buf/command/breaking/breaking.go b/private/buf/cmd/buf/command/breaking/breaking.go index 0911c33e24..dddb666783 100644 --- a/private/buf/cmd/buf/command/breaking/breaking.go +++ b/private/buf/cmd/buf/command/breaking/breaking.go @@ -218,11 +218,21 @@ func run( defer func() { retErr = errors.Join(retErr, wasmRuntime.Close(ctx)) }() - allCheckConfigs := slicesext.Map( - imageWithConfigs, - func(imageWithConfig bufctl.ImageWithConfig) bufconfig.CheckConfig { - return imageWithConfig.BreakingConfig() - }, + // We add all check configs (both lint and breaking) as related configs to check if plugins + // have rules configured. + allCheckConfigs := append( + slicesext.Map( + imageWithConfigs, + func(imageWithConfig bufctl.ImageWithConfig) bufconfig.CheckConfig { + return imageWithConfig.LintConfig() + }, + ), + slicesext.Map( + imageWithConfigs, + func(imageWithConfig bufctl.ImageWithConfig) bufconfig.CheckConfig { + return imageWithConfig.BreakingConfig() + }, + )..., ) var allFileAnnotations []bufanalysis.FileAnnotation for i, imageWithConfig := range imageWithConfigs { diff --git a/private/buf/cmd/buf/command/config/internal/internal.go b/private/buf/cmd/buf/command/config/internal/internal.go index 995f1b9007..d5826816ff 100644 --- a/private/buf/cmd/buf/command/config/internal/internal.go +++ b/private/buf/cmd/buf/command/config/internal/internal.go @@ -211,10 +211,6 @@ func lsRun( var rules []bufcheck.Rule if flags.ConfiguredOnly { moduleConfigs := bufYAMLFile.ModuleConfigs() - allCheckConfigs := append( - slicesext.Map(moduleConfigs, func(moduleConfig bufconfig.ModuleConfig) bufconfig.CheckConfig { return moduleConfig.LintConfig() }), - slicesext.Map(moduleConfigs, func(moduleConfig bufconfig.ModuleConfig) bufconfig.CheckConfig { return moduleConfig.BreakingConfig() })..., - ) var moduleConfig bufconfig.ModuleConfig switch fileVersion := bufYAMLFile.FileVersion(); fileVersion { case bufconfig.FileVersionV1Beta1, bufconfig.FileVersionV1: @@ -241,6 +237,12 @@ 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. + allCheckConfigs := append( + slicesext.Map(moduleConfigs, func(moduleConfig bufconfig.ModuleConfig) bufconfig.CheckConfig { return moduleConfig.LintConfig() }), + slicesext.Map(moduleConfigs, func(moduleConfig bufconfig.ModuleConfig) bufconfig.CheckConfig { return moduleConfig.BreakingConfig() })..., + ) switch ruleType { case check.RuleTypeLint: checkConfig = moduleConfig.LintConfig() diff --git a/private/buf/cmd/buf/command/lint/lint.go b/private/buf/cmd/buf/command/lint/lint.go index 035cd5365e..d5e243989c 100644 --- a/private/buf/cmd/buf/command/lint/lint.go +++ b/private/buf/cmd/buf/command/lint/lint.go @@ -145,11 +145,21 @@ func run( retErr = errors.Join(retErr, wasmRuntime.Close(ctx)) }() var allFileAnnotations []bufanalysis.FileAnnotation - allCheckConfigs := slicesext.Map( - imageWithConfigs, - func(imageWithConfig bufctl.ImageWithConfig) bufconfig.CheckConfig { - return imageWithConfig.LintConfig() - }, + // We add all check configs (both lint and breaking) as related configs to check if plugins + // have rules configured. + allCheckConfigs := append( + slicesext.Map( + imageWithConfigs, + func(imageWithConfig bufctl.ImageWithConfig) bufconfig.CheckConfig { + return imageWithConfig.LintConfig() + }, + ), + slicesext.Map( + imageWithConfigs, + func(imageWithConfig bufctl.ImageWithConfig) bufconfig.CheckConfig { + return imageWithConfig.BreakingConfig() + }, + )..., ) for _, imageWithConfig := range imageWithConfigs { client, err := bufcheck.NewClient( diff --git a/private/bufpkg/bufcheck/config.go b/private/bufpkg/bufcheck/config.go index f1b4d75b65..4bab6bf48e 100644 --- a/private/bufpkg/bufcheck/config.go +++ b/private/bufpkg/bufcheck/config.go @@ -28,9 +28,9 @@ func configForLintConfig( lintConfig bufconfig.LintConfig, allRules []Rule, allCategories []Category, - additionalCheckConfigs []bufconfig.CheckConfig, + relatedCheckConfigs []bufconfig.CheckConfig, ) (*config, error) { - rulesConfig, err := rulesConfigForCheckConfig(lintConfig, allRules, allCategories, check.RuleTypeLint, additionalCheckConfigs) + rulesConfig, err := rulesConfigForCheckConfig(lintConfig, allRules, allCategories, check.RuleTypeLint, relatedCheckConfigs) if err != nil { return nil, err } @@ -49,9 +49,9 @@ func configForBreakingConfig( allRules []Rule, allCategories []Category, excludeImports bool, - additionalCheckConfigs []bufconfig.CheckConfig, + relatedCheckConfigs []bufconfig.CheckConfig, ) (*config, error) { - rulesConfig, err := rulesConfigForCheckConfig(breakingConfig, allRules, allCategories, check.RuleTypeBreaking, additionalCheckConfigs) + 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 70968663c5..6a9cefcf65 100644 --- a/private/bufpkg/bufcheck/rules_config.go +++ b/private/bufpkg/bufcheck/rules_config.go @@ -33,7 +33,7 @@ func rulesConfigForCheckConfig( allRules []Rule, allCategories []Category, ruleType check.RuleType, - additionalCheckConfigs []bufconfig.CheckConfig, + relatedCheckConfigs []bufconfig.CheckConfig, ) (*rulesConfig, error) { return newRulesConfig( checkConfig.UseIDsAndCategories(), @@ -43,7 +43,7 @@ func rulesConfigForCheckConfig( allRules, allCategories, ruleType, - additionalCheckConfigs, + relatedCheckConfigs, ) } @@ -96,17 +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 provider additional 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 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. @@ -130,7 +122,7 @@ func newRulesConfig( allRules []Rule, allCategories []Category, ruleType check.RuleType, - additionalCheckConfigs []bufconfig.CheckConfig, + relatedCheckConfigs []bufconfig.CheckConfig, ) (*rulesConfig, error) { allRulesForType := rulesForType(allRules, ruleType) if len(allRulesForType) == 0 { @@ -299,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. // @@ -312,28 +303,29 @@ 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. - for _, checkConfig := range additionalCheckConfigs { + // 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(), - ruleIDToCategoryIDs, - categoryIDToRuleIDs, + allRuleIDToCategoryIDs, + allCategoryIDToRuleIDs, ) if err != nil { return nil, err } for _, checkConfigRuleID := range checkConfigUseRuleIDs { - // We do not validate the additional check config rules here, only check for those - // that are found. - if rule, ok := ruleIDToRule[checkConfigRuleID]; ok { + // 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()) } } @@ -448,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 {