Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Dec 19, 2024
10 changes: 10 additions & 0 deletions private/buf/cmd/buf/command/breaking/breaking.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
Expand Down
9 changes: 9 additions & 0 deletions private/buf/cmd/buf/command/config/internal/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand All @@ -253,6 +261,7 @@ func lsRun(
}
configuredRuleOptions := []bufcheck.ConfiguredRulesOption{
bufcheck.WithPluginConfigs(bufYAMLFile.PluginConfigs()...),
bufcheck.WithRelatedCheckConfigs(allCheckConfigs...),
}
rules, err = checkClient.ConfiguredRules(
ctx,
Expand Down
10 changes: 10 additions & 0 deletions private/buf/cmd/buf/command/lint/lint.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down
19 changes: 19 additions & 0 deletions private/bufpkg/bufcheck/bufcheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
32 changes: 26 additions & 6 deletions private/bufpkg/bufcheck/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -168,6 +168,7 @@ func (c *client) Breaking(
allRules,
allCategories,
breakingOptions.excludeImports,
breakingOptions.relatedCheckConfigs,
)
if err != nil {
return err
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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...)
}
6 changes: 4 additions & 2 deletions private/bufpkg/bufcheck/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
}
Expand Down
61 changes: 31 additions & 30 deletions private/bufpkg/bufcheck/rules_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func rulesConfigForCheckConfig(
allRules []Rule,
allCategories []Category,
ruleType check.RuleType,
relatedCheckConfigs []bufconfig.CheckConfig,
) (*rulesConfig, error) {
return newRulesConfig(
checkConfig.UseIDsAndCategories(),
Expand All @@ -42,6 +43,7 @@ func rulesConfigForCheckConfig(
allRules,
allCategories,
ruleType,
relatedCheckConfigs,
)
}

Expand Down Expand Up @@ -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.
Expand All @@ -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 {
Expand Down Expand Up @@ -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.
//
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like if you specify the WithRelatedCheckConfigs option, these CheckConfigs are not additionally checked, rather these CheckConfigs are the only CheckConfigs that are checked, and the original CheckConfigs (?) from allRules are not checked. Am I wrong/does this make sense? Make sure it is additional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! On lines 294-305, we are still checking the original configurations, we are just checking the additional rules in lieu of checking whether the plugin has rules from other rules types.

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())
}
}
}

Expand Down Expand Up @@ -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 {
Expand Down
Loading