Skip to content

Commit

Permalink
Remove heuristic for unused plugin warning and rely on related check …
Browse files Browse the repository at this point in the history
…configs.
  • Loading branch information
doriable committed Dec 18, 2024
1 parent 42c9619 commit e5d360e
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 64 deletions.
20 changes: 15 additions & 5 deletions private/buf/cmd/buf/command/breaking/breaking.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
10 changes: 6 additions & 4 deletions private/buf/cmd/buf/command/config/internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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()
Expand Down
20 changes: 15 additions & 5 deletions private/buf/cmd/buf/command/lint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions private/bufpkg/bufcheck/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
67 changes: 21 additions & 46 deletions private/bufpkg/bufcheck/rules_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -43,7 +43,7 @@ func rulesConfigForCheckConfig(
allRules,
allCategories,
ruleType,
additionalCheckConfigs,
relatedCheckConfigs,
)
}

Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
//
Expand All @@ -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())
}
}
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit e5d360e

Please sign in to comment.