Repository: uber/prototool
Pull request #344 (2019-02-16 19:56:38)

Outlaw field name "descriptor" and variants due to collision with Java-generated code
Re: https://github.com/protocolbuffers/protobuf/issues/5742

codecov-io: # [Codecov](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=h1) Report
> Merging [#344](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=desc) into [dev](https://codecov.io/gh/uber/prototool/commit/f156fbf3eac0494073b38bc74b8fcc784f6251ba?src=pr&el=desc) will **increase** coverage by `0.07%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/uber/prototool/pull/344/graphs/tree.svg?width=650&token=A9V0DApqZg&height=150&src=pr)](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##              dev    #344      +/-   ##
=========================================
+ Coverage   73.43%   73.5%   +0.07%     
=========================================
  Files         145     146       +1     
  Lines        6430    6447      +17     
=========================================
+ Hits         4722    4739      +17     
  Misses       1271    1271              
  Partials      437     437
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/lint/lint.go](https://codecov.io/gh/uber/prototool/pull/344/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9saW50Lmdv) | `71.01% <ø> (ø)` | :arrow_up: |
| [...al/lint/check\_message\_field\_names\_no\_descriptor.go](https://codecov.io/gh/uber/prototool/pull/344/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19tZXNzYWdlX2ZpZWxkX25hbWVzX25vX2Rlc2NyaXB0b3IuZ28=) | `100% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=footer). Last update [f156fbf...ae1aafc](https://codecov.io/gh/uber/prototool/pull/344?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/internal/lint/BUILD.bazel b/internal/lint/BUILD.bazel
index c212991..c50effe 100644
--- a/internal/lint/BUILD.bazel
+++ b/internal/lint/BUILD.bazel
@@ -33,6 +33,7 @@ go_library(
         "check_message_field_names_filepath.go",
         "check_message_field_names_lower_snake_case.go",
         "check_message_field_names_lowercase.go",
+        "check_message_field_names_no_descriptor.go",
         "check_message_fields_duration.go",
         "check_message_fields_no_json_name.go",
         "check_message_fields_not_floats.go",
diff --git a/internal/lint/check_message_field_names_no_descriptor.go b/internal/lint/check_message_field_names_no_descriptor.go
new file mode 100644
index 0000000..6f4939d
--- /dev/null
+++ b/internal/lint/check_message_field_names_no_descriptor.go
@@ -0,0 +1,74 @@
+// Copyright (c) 2019 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package lint
+
+import (
+	"strings"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/text"
+)
+
+var messageFieldNamesNoDescriptorLinter = NewLinter(
+	"MESSAGE_FIELD_NAMES_NO_DESCRIPTOR",
+	`Verifies that all message field names are not "descriptor", which results in a collision in Java-generated code.`,
+	checkMessageFieldNamesNoDescriptor,
+)
+
+func checkMessageFieldNamesNoDescriptor(add func(*text.Failure), dirPath string, descriptors []*FileDescriptor) error {
+	return runVisitor(messageFieldNamesNoDescriptorVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type messageFieldNamesNoDescriptorVisitor struct {
+	baseAddVisitor
+}
+
+func (v messageFieldNamesNoDescriptorVisitor) VisitMessage(message *proto.Message) {
+	for _, element := range message.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldNamesNoDescriptorVisitor) VisitOneof(oneof *proto.Oneof) {
+	for _, element := range oneof.Elements {
+		element.Accept(v)
+	}
+}
+
+func (v messageFieldNamesNoDescriptorVisitor) VisitNormalField(field *proto.NormalField) {
+	v.handleField(field.Field)
+}
+
+func (v messageFieldNamesNoDescriptorVisitor) VisitOneofField(field *proto.OneOfField) {
+	v.handleField(field.Field)
+}
+
+func (v messageFieldNamesNoDescriptorVisitor) VisitMapField(field *proto.MapField) {
+	v.handleField(field.Field)
+}
+
+func (v messageFieldNamesNoDescriptorVisitor) handleField(field *proto.Field) {
+	// Descriptor, _descriptor, descriptor_, __descriptor, descriptor__ are also invalid
+	// https://github.com/protocolbuffers/protobuf/issues/5742
+	if strings.Trim(strings.ToLower(field.Name), "_") == "descriptor" {
+		v.AddFailuref(field.Position, `Field name %q causes a collision in Java-generated code.`, field.Name)
+	}
+}
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index d400efd..da9b898 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -89,6 +89,7 @@ var (
 		messageFieldNamesFilepathLinter,
 		messageFieldNamesLowerSnakeCaseLinter,
 		messageFieldNamesLowercaseLinter,
+		messageFieldNamesNoDescriptorLinter,
 		messageNamesCamelCaseLinter,
 		messageNamesCapitalizedLinter,
 		messagesHaveCommentsLinter,
@@ -228,6 +229,7 @@ var (
 		messageFieldNamesFilenameLinter,
 		messageFieldNamesFilepathLinter,
 		messageFieldNamesLowerSnakeCaseLinter,
+		messageFieldNamesNoDescriptorLinter,
 		messageFieldsDurationLinter,
 		messageFieldsNoJSONNameLinter,
 		messageFieldsNotFloatsLinter,


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index 9968146..703d15c 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -492,6 +492,18 @@ func TestLint(t *testing.T) {
 		"testdata/lint/notempty/foo/v1/hello_api.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`14:3:MESSAGE_FIELD_NAMES_NO_DESCRIPTOR
+		18:3:MESSAGE_FIELD_NAMES_NO_DESCRIPTOR
+		22:3:MESSAGE_FIELD_NAMES_NO_DESCRIPTOR
+		26:3:MESSAGE_FIELD_NAMES_NO_DESCRIPTOR
+		30:3:MESSAGE_FIELD_NAMES_NO_DESCRIPTOR
+		34:3:MESSAGE_FIELD_NAMES_NO_DESCRIPTOR`,
+		"testdata/lint/nodescriptor/foo/v1/foo.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
diff --git a/internal/cmd/testdata/lint/nodescriptor/foo/v1/foo.proto b/internal/cmd/testdata/lint/nodescriptor/foo/v1/foo.proto
new file mode 100644
index 0000000..59e7f20
--- /dev/null
+++ b/internal/cmd/testdata/lint/nodescriptor/foo/v1/foo.proto
@@ -0,0 +1,35 @@
+syntax = "proto3";
+
+package foo.v1;
+
+option csharp_namespace = "Foo.V1";
+option go_package = "foov1";
+option java_multiple_files = true;
+option java_outer_classname = "FooProto";
+option java_package = "com.foo.v1";
+option objc_class_prefix = "FXX";
+option php_namespace = "Foo\\V1";
+
+message One {
+  int64 descriptor = 1;
+}
+
+message Two {
+  int64 Descriptor = 1;
+}
+
+message Three {
+  int64 _descriptor = 1;
+}
+
+message Four {
+  int64 descriptor_ = 1;
+}
+
+message Five {
+  int64 __descriptor = 1;
+}
+
+message Six {
+  int64 descriptor__ = 1;
+}
diff --git a/internal/cmd/testdata/lint/nodescriptor/prototool.yaml b/internal/cmd/testdata/lint/nodescriptor/prototool.yaml
new file mode 100644
index 0000000..d86492d
--- /dev/null
+++ b/internal/cmd/testdata/lint/nodescriptor/prototool.yaml
@@ -0,0 +1,6 @@
+lint:
+  group: uber2
+  rules:
+    remove:
+      - MESSAGES_HAVE_SENTENCE_COMMENTS_EXCEPT_REQUEST_RESPONSE_TYPES
+      - MESSAGE_FIELD_NAMES_LOWER_SNAKE_CASE