From 80432a2dd924b79ef85dbadde4905dffdab4975d Mon Sep 17 00:00:00 2001
From: Jon Parise <jon@pinterest.com>
Date: Thu, 28 Apr 2022 11:17:01 -0700
Subject: [PATCH] Add a 'field.id.zero' check (#49)

This check reports an error if a field's ID is zero, which is generally
unsupported by the Apache Thrift compiler. This is distinct from the
`field.id.negative` check given the existence of the `--allow-neg-keys`
Apache Thrift compiler option.
---
 README.md             |  7 +++++++
 checks/fields.go      |  9 +++++++++
 checks/fields_test.go | 22 ++++++++++++++++++++++
 cmd/main.go           |  1 +
 4 files changed, 39 insertions(+)

diff --git a/README.md b/README.md
index bfde4b5..8262537 100644
--- a/README.md
+++ b/README.md
@@ -96,6 +96,13 @@ implicit/auto-assigning syntax).
 
 This check reports an error if a field's ID is negative.
 
+### `field.id.zero`
+
+This check reports an error if a field's ID is zero, which is generally
+unsupported by the Apache Thrift compiler. This is distinct from the
+`field.id.negative` check given the existence of the `--allow-neg-keys`
+Apache Thrift compiler option.
+
 ### `field.optional`
 
 This check warns if a field isn't declared as "optional", which is considered
diff --git a/checks/fields.go b/checks/fields.go
index b47341a..64daf35 100644
--- a/checks/fields.go
+++ b/checks/fields.go
@@ -37,6 +37,15 @@ func CheckFieldIDNegative() *thriftcheck.Check {
 	})
 }
 
+// CheckFieldIDZero reports an error if a field's ID is zero.
+func CheckFieldIDZero() *thriftcheck.Check {
+	return thriftcheck.NewCheck("field.id.zero", func(c *thriftcheck.C, f *ast.Field) {
+		if f.ID == 0 {
+			c.Errorf(f, "field ID for %q is zero", f.Name)
+		}
+	})
+}
+
 // CheckFieldOptional warns if a field isn't declared as "optional".
 func CheckFieldOptional() *thriftcheck.Check {
 	return thriftcheck.NewCheck("field.optional", func(c *thriftcheck.C, f *ast.Field) {
diff --git a/checks/fields_test.go b/checks/fields_test.go
index 2903a66..242813b 100644
--- a/checks/fields_test.go
+++ b/checks/fields_test.go
@@ -61,6 +61,28 @@ func TestCheckFieldIDNegative(t *testing.T) {
 	RunTests(t, check, tests)
 }
 
+func TestCheckFieldIDZero(t *testing.T) {
+	tests := []Test{
+		{
+			node: &ast.Field{ID: 1, Name: "Field"},
+			want: []string{},
+		},
+		{
+			node: &ast.Field{ID: 0, Name: "Field"},
+			want: []string{
+				`t.thrift:0:1: error: field ID for "Field" is zero (field.id.zero)`,
+			},
+		},
+		{
+			node: &ast.Field{ID: -1, Name: "Field"},
+			want: []string{},
+		},
+	}
+
+	check := checks.CheckFieldIDZero()
+	RunTests(t, check, tests)
+}
+
 func TestCheckFieldOptional(t *testing.T) {
 	tests := []Test{
 		{
diff --git a/cmd/main.go b/cmd/main.go
index e76bda4..128cb8b 100644
--- a/cmd/main.go
+++ b/cmd/main.go
@@ -155,6 +155,7 @@ func main() {
 		checks.CheckEnumSize(cfg.Checks.Enum.Size.Warning, cfg.Checks.Enum.Size.Error),
 		checks.CheckFieldIDMissing(),
 		checks.CheckFieldIDNegative(),
+		checks.CheckFieldIDZero(),
 		checks.CheckFieldOptional(),
 		checks.CheckFieldRequiredness(),
 		checks.CheckIncludePath(),