Repository: googleapis/api-linter
Pull request #512 (2020-03-04 18:08:04)

Support source code location overrides for ruleEnabled.
This fixes an issue where the linter does not respect a disable comment
on "package" statements, which themselves do not have an associated
Descriptor (natural file location). Instead these lint rules can specify
a Location, and this location will now be respected when checking for
rule disable comments.

mingzhi: Added Luke for another look.

Patch:
diff --git a/lint/lint.go b/lint/lint.go
index be19534..58e9166 100644
--- a/lint/lint.go
+++ b/lint/lint.go
@@ -70,7 +70,7 @@ func (l *Linter) lintFileDescriptor(fd *desc.FileDescriptor) (Response, error) {
 		if l.configs.IsRuleEnabled(string(name), fd.GetName()) {
 			if problems, err := l.runAndRecoverFromPanics(rule, fd); err == nil {
 				for _, p := range problems {
-					if ruleIsEnabled(rule, p.Descriptor, aliasMap) {
+					if ruleIsEnabled(rule, p.Descriptor, p.Location, aliasMap) {
 						p.RuleID = rule.GetName()
 						resp.Problems = append(resp.Problems, p)
 					}
diff --git a/lint/rule.go b/lint/rule.go
index 5832bbc..f6ceb29 100644
--- a/lint/rule.go
+++ b/lint/rule.go
@@ -371,7 +371,10 @@ 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 {
+//
+// Note, if the given source code location is not nil, it will be used to
+// augment the set of commentLines.
+func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, l *dpb.SourceCodeInfo_Location, aliasMap map[string]string) bool {
 	// Sanity check: All rules are disabled on a deprecated descriptor.
 	deprecated := false
 	switch v := d.(type) {
@@ -399,6 +402,9 @@ func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, aliasMap map[string]string
 	names := []string{ruleName, aliasMap[ruleName]}
 
 	commentLines := []string{}
+	if l != nil {
+		commentLines = append(commentLines, strings.Split(l.GetLeadingComments(), "\n")...)
+	}
 	if f, ok := d.(*desc.FileDescriptor); ok {
 		commentLines = append(commentLines, strings.Split(fileHeader(f), "\n")...)
 	} else {
@@ -420,8 +426,11 @@ func ruleIsEnabled(rule ProtoRule, d desc.Descriptor, aliasMap map[string]string
 
 	// The rule may have been disabled on a parent. (For example, a field rule
 	// may be disabled at the message level to cover all fields in the message).
+	//
+	// Do not pass the source code location here, the source location in relation
+	// to the parent is not helpful.
 	if parent := d.GetParent(); parent != nil {
-		return ruleIsEnabled(rule, parent, aliasMap)
+		return ruleIsEnabled(rule, parent, nil, aliasMap)
 	}
 
 	return true


Test patch:
diff --git a/cmd/api-linter/integration_test.go b/cmd/api-linter/integration_test.go
index 4e07cd9..ae98a94 100644
--- a/cmd/api-linter/integration_test.go
+++ b/cmd/api-linter/integration_test.go
@@ -44,6 +44,18 @@ var testCases = []struct {
 		message Book {}
 		`,
 	},
+	{
+		testName: "PackageVersion",
+		rule:     "core::0215::versioned-packages",
+		proto: `
+		syntax = "proto3";
+
+		// disable-me-here
+		package google.test;
+
+		message Test {}
+		`,
+	},
 	{
 		testName: "FieldNames",
 		rule:     "core::0140::lower-snake",
diff --git a/lint/rule_test.go b/lint/rule_test.go
index db7b89e..212ce04 100644
--- a/lint/rule_test.go
+++ b/lint/rule_test.go
@@ -407,7 +407,7 @@ func TestRuleIsEnabled(t *testing.T) {
 			if err != nil {
 				t.Fatalf("Error building test message")
 			}
-			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], aliases), test.enabled; got != want {
+			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, aliases), test.enabled; got != want {
 				t.Errorf("Expected the test rule to return %v from ruleIsEnabled, got %v", want, got)
 			}
 		})
@@ -434,10 +434,10 @@ func TestRuleIsEnabledFirstMessage(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error building test file: %q", err)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil), false; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0], nil, nil), false; got != want {
 		t.Errorf("Expected the first message to return %v from ruleIsEnabled, got %v", want, got)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1], nil), true; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1], nil, nil), true; got != want {
 		t.Errorf("Expected the second message to return %v from ruleIsEnabled, got %v", want, got)
 	}
 }
@@ -462,10 +462,10 @@ func TestRuleIsEnabledParent(t *testing.T) {
 	if err != nil {
 		t.Fatalf("Error building test file: %q", err)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil), false; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil, nil), false; got != want {
 		t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
 	}
-	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1].GetFields()[0], nil), true; got != want {
+	if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[1].GetFields()[0], nil, nil), true; got != want {
 		t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
 	}
 }
@@ -502,7 +502,7 @@ func TestRuleIsEnabledDeprecated(t *testing.T) {
 			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 {
+			if got, want := ruleIsEnabled(rule, f.GetMessageTypes()[0].GetFields()[0], nil, nil), test.enabled; got != want {
 				t.Errorf("Expected the foo field to return %v from ruleIsEnabled; got %v", want, got)
 			}
 		})