Repository: googleapis/api-linter
Pull request #506 (2020-02-25 03:09:01)

[feat] Ignore deprecated descriptors.
@wora had a really good point: running lint rules on deprecated
descriptors almost never makes sense, and they contribute to noise
for API producers.

This PR issues a blanket disable on any descriptor with a
`deprecated=true` annotation.


Patch:
diff --git a/lint/rule.go b/lint/rule.go
index 48c7686..5832bbc 100644
--- a/lint/rule.go
+++ b/lint/rule.go
@@ -372,6 +372,28 @@ func extractDisabledRuleName(commentLine string) string {
 // ruleIsEnabled returns true if the rule is enabled (not disabled by the comments
 // for the given descriptor or its file), false otherwise.
 func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, aliasMap map[string]string) bool {
+	// Sanity check: All rules are disabled on a deprecated descriptor.
+	deprecated := false
+	switch v := d.(type) {
+	case *desc.EnumDescriptor:
+		deprecated = v.GetEnumOptions().GetDeprecated()
+	case *desc.EnumValueDescriptor:
+		deprecated = v.GetEnumValueOptions().GetDeprecated()
+	case *desc.FieldDescriptor:
+		deprecated = v.GetFieldOptions().GetDeprecated()
+	case *desc.FileDescriptor:
+		deprecated = v.GetFileOptions().GetDeprecated()
+	case *desc.MessageDescriptor:
+		deprecated = v.GetMessageOptions().GetDeprecated()
+	case *desc.MethodDescriptor:
+		deprecated = v.GetMethodOptions().GetDeprecated()
+	case *desc.ServiceDescriptor:
+		deprecated = v.GetServiceOptions().GetDeprecated()
+	}
+	if deprecated {
+		return false
+	}
+
 	// Some rules have a legacy name. We add it to the check list.
 	ruleName := string(rule.GetName())
 	names := []string{ruleName, aliasMap[ruleName]}


Test patch:
diff --git a/lint/rule_test.go b/lint/rule_test.go
index 6dc9d85..db7b89e 100644
--- a/lint/rule_test.go
+++ b/lint/rule_test.go
@@ -18,6 +18,7 @@ import (
 	"reflect"
 	"testing"
 
+	dpb "github.com/golang/protobuf/protoc-gen-go/descriptor"
 	"github.com/jhump/protoreflect/desc"
 	"github.com/jhump/protoreflect/desc/builder"
 )
@@ -125,7 +126,7 @@ func TestFieldRule(t *testing.T) {
 	// Iterate over the tests and run them.
 	for _, test := range makeLintRuleTests(fd.GetMessageTypes()[0].GetFields()[1]) {
 		t.Run(test.testName, func(t *testing.T) {
-			// Create the message rule.
+			// Create the field rule.
 			rule := &FieldRule{
 				Name: RuleName("test"),
 				OnlyIf: func(f *desc.FieldDescriptor) bool {
@@ -469,6 +470,45 @@ func TestRuleIsEnabledParent(t *testing.T) {
 	}
 }
 
+func TestRuleIsEnabledDeprecated(t *testing.T) {
+	// Create a rule that we can check enabled status on.
+	rule := &FieldRule{
+		Name: RuleName("test"),
+		LintField: func(f *desc.FieldDescriptor) []Problem {
+			return nil
+		},
+	}
+
+	for _, test := range []struct {
+		name            string
+		msgDeprecated   bool
+		fieldDeprecated bool
+		enabled         bool
+	}{
+		{"Both", true, true, false},
+		{"Message", true, false, false},
+		{"Field", false, true, false},
+		{"Neither", false, false, true},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			// Build a proto with a message and field, possibly deprecated.
+			f, err := builder.NewFile("test.proto").AddMessage(
+				builder.NewMessage("Foo").SetOptions(&dpb.MessageOptions{
+					Deprecated: &test.msgDeprecated,
+				}).AddField(builder.NewField("bar", builder.FieldTypeBool()).SetOptions(
+					&dpb.FieldOptions{Deprecated: &test.fieldDeprecated},
+				)),
+			).Build()
+			if err != nil {
+				t.Fatalf("Error building test file: %q", err)
+			}
+			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil), test.enabled; got != want {
+				t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
+			}
+		})
+	}
+}
+
 type lintRuleTest struct {
 	testName string
 	problems []Problem