Repository: uber/prototool
Pull request #300 (2019-01-14 18:06:40)

Add naming linters
This adds linters that outlaw the names `common, data, uuid` as part of any name. These are suppressable with `// @suppresswarnings naming`. Re: #265.

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

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

```diff
@@            Coverage Diff             @@
##              dev     #300      +/-   ##
==========================================
+ Coverage   72.89%   73.13%   +0.24%     
==========================================
  Files         136      137       +1     
  Lines        6046     6120      +74     
==========================================
+ Hits         4407     4476      +69     
- Misses       1223     1224       +1     
- Partials      416      420       +4
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/300?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/lint/lint.go](https://codecov.io/gh/uber/prototool/pull/300/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9saW50Lmdv) | `72.14% <ø> (ø)` | :arrow_up: |
| [internal/lint/check\_names.go](https://codecov.io/gh/uber/prototool/pull/300/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19uYW1lcy5nbw==) | `83.78% <83.78%> (ø)` | |
| [internal/lint/check\_comments\_no\_inline.go](https://codecov.io/gh/uber/prototool/pull/300/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19jb21tZW50c19ub19pbmxpbmUuZ28=) | `78.84% <0%> (+13.46%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/uber/prototool/pull/300?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/300?src=pr&el=footer). Last update [264fd67...5f564ba](https://codecov.io/gh/uber/prototool/pull/300?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/Makefile b/Makefile
index ac9c38b..c90b4a9 100644
--- a/Makefile
+++ b/Makefile
@@ -136,8 +136,12 @@ cover:
 	@mkdir -p $(TMP_ETC)
 	@rm -f $(TMP_ETC)/coverage.txt $(TMP_ETC)/coverage.html
 	go test -race -coverprofile=$(TMP_ETC)/coverage.txt -coverpkg=$(shell echo $(PKGS) | tr ' ' ',') $(PKGS)
-	go tool cover -html=$(TMP_ETC)/coverage.txt -o $(TMP_ETC)/coverage.html
-	go tool cover -func=$(TMP_ETC)/coverage.txt | grep total
+	@go tool cover -html=$(TMP_ETC)/coverage.txt -o $(TMP_ETC)/coverage.html
+	@echo
+	@go tool cover -func=$(TMP_ETC)/coverage.txt | grep total
+	@echo
+	@echo Open the coverage report:
+	@echo open $(TMP_ETC)/coverage.html
 
 .PHONY: codecov
 codecov:
diff --git a/internal/lint/check_names.go b/internal/lint/check_names.go
new file mode 100644
index 0000000..de54673
--- /dev/null
+++ b/internal/lint/check_names.go
@@ -0,0 +1,188 @@
+// 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 (
+	"fmt"
+	"strings"
+	"text/scanner"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/text"
+)
+
+const namesSuppressableAnnotation = "naming"
+
+var (
+	namesNoCommonLinter = newNamesLinter(
+		"common",
+		"Common has no semantic meaning, consider using a name that reflects the type instead.",
+	)
+	namesNoDataLinter = newNamesLinter(
+		"data",
+		`Data is a decorator and all types on Protobuf are data, consider merging this information into a higher-level type, or if you must have such a type, Use "Info" instead.`,
+	)
+	namesNoUUIDLinter = newNamesLinter(
+		"uuid",
+		`UUIDs in Protobuf are named ID instead of UUID.`,
+	)
+)
+
+func newNamesLinter(outlawedName string, additionalHelp string) Linter {
+	return NewLinter(
+		"NAMES_NO_"+strings.ToUpper(outlawedName),
+		fmt.Sprintf(
+			`Suppressable with "@suppresswarnings %s". Verifies that no type name contains the word %q.`,
+			namesSuppressableAnnotation,
+			outlawedName,
+		),
+		newCheckNames(
+			outlawedName,
+			additionalHelp,
+		),
+	)
+}
+
+func newCheckNames(outlawedName string, additionalHelp string) func(func(*text.Failure), string, []*FileDescriptor) error {
+	return func(add func(*text.Failure), dirPath string, descriptors []*FileDescriptor) error {
+		return runVisitor(&namesVisitor{
+			baseAddVisitor: newBaseAddVisitor(add),
+			outlawedName:   outlawedName,
+			additionalHelp: additionalHelp,
+		}, descriptors)
+	}
+}
+
+type namesVisitor struct {
+	baseAddVisitor
+
+	outlawedName   string
+	additionalHelp string
+}
+
+func (v *namesVisitor) VisitMessage(element *proto.Message) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Elements {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitService(element *proto.Service) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Elements {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitSyntax(element *proto.Syntax) {
+	// do nothing
+}
+
+func (v *namesVisitor) VisitPackage(element *proto.Package) {
+	v.checkName(element.Position, element.Name, element.Comment)
+}
+
+func (v *namesVisitor) VisitOption(element *proto.Option) {
+	v.checkName(element.Position, element.Name, element.Comment)
+}
+
+func (v *namesVisitor) VisitImport(element *proto.Import) {
+	// do nothing
+}
+
+func (v *namesVisitor) VisitNormalField(element *proto.NormalField) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Options {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitEnumField(element *proto.EnumField) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	if element.ValueOption != nil {
+		element.ValueOption.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitEnum(element *proto.Enum) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Elements {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitComment(element *proto.Comment) {
+	// do nothing
+}
+
+func (v *namesVisitor) VisitOneof(element *proto.Oneof) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Elements {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitOneofField(element *proto.OneOfField) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Options {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitReserved(element *proto.Reserved) {
+	for _, fieldName := range element.FieldNames {
+		v.checkName(element.Position, fieldName, element.Comment)
+	}
+}
+
+func (v *namesVisitor) VisitRPC(element *proto.RPC) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Elements {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitMapField(element *proto.MapField) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Options {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitGroup(element *proto.Group) {
+	v.checkName(element.Position, element.Name, element.Comment)
+	for _, child := range element.Elements {
+		child.Accept(v)
+	}
+}
+
+func (v *namesVisitor) VisitExtensions(element *proto.Extensions) {
+	// do nothing
+}
+
+func (v *namesVisitor) checkName(position scanner.Position, name string, comment *proto.Comment) {
+	if strings.Contains(strings.ToLower(name), v.outlawedName) {
+		if isSuppressed(comment, namesSuppressableAnnotation) {
+			return
+		}
+		v.AddFailuref(position, `The name %q contains the outlawed name %q. %s This can be suppressed by adding "@suppresswarnings %s" to the type comment.`, name, v.outlawedName, v.additionalHelp, namesSuppressableAnnotation)
+	}
+}
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index e92b085..17b09d0 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -89,6 +89,9 @@ var (
 		messagesHaveCommentsLinter,
 		messagesHaveCommentsExceptRequestResponseTypesLinter,
 		messagesHaveSentenceCommentsExceptRequestResponseTypesLinter,
+		namesNoCommonLinter,
+		namesNoDataLinter,
+		namesNoUUIDLinter,
 		oneofNamesLowerSnakeCaseLinter,
 		packageIsDeclaredLinter,
 		packageLowerSnakeCaseLinter,
@@ -218,6 +221,9 @@ var (
 		messageFieldsNotFloatsLinter,
 		messageNamesCamelCaseLinter,
 		messageNamesCapitalizedLinter,
+		namesNoCommonLinter,
+		namesNoDataLinter,
+		namesNoUUIDLinter,
 		oneofNamesLowerSnakeCaseLinter,
 		packageIsDeclaredLinter,
 		packageLowerSnakeCaseLinter,


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index b6703e6..43c9175 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -455,6 +455,33 @@ func TestLint(t *testing.T) {
 		16:58:COMMENTS_NO_INLINE`,
 		"testdata/lint/inlinecomments/foo/v1/hello_api.proto",
 	)
+	assertDoLintFile(
+		t,
+		false,
+		`13:1:NAMES_NO_COMMON
+		14:3:NAMES_NO_COMMON
+		17:1:NAMES_NO_DATA
+		18:3:NAMES_NO_DATA
+		21:1:NAMES_NO_UUID
+		22:3:NAMES_NO_UUID
+		25:1:NAMES_NO_COMMON
+		26:3:NAMES_NO_UUID
+		27:5:NAMES_NO_UUID
+		28:7:NAMES_NO_UUID
+		33:1:NAMES_NO_COMMON
+		33:1:NAMES_NO_DATA
+		37:1:NAMES_NO_UUID
+		38:3:NAMES_NO_DATA
+		40:3:NAMES_NO_DATA
+		41:5:NAMES_NO_COMMON
+		45:1:NAMES_NO_COMMON
+		46:3:NAMES_NO_COMMON
+		49:1:NAMES_NO_DATA
+		50:3:NAMES_NO_COMMON
+		53:1:NAMES_NO_UUID
+		54:3:NAMES_NO_COMMON`,
+		"testdata/lint/naming/foo/v1/foo.proto",
+	)
 
 	assertDoLintFile(
 		t,
diff --git a/internal/cmd/testdata/lint/naming/foo/v1/foo.proto b/internal/cmd/testdata/lint/naming/foo/v1/foo.proto
new file mode 100644
index 0000000..5a564ab
--- /dev/null
+++ b/internal/cmd/testdata/lint/naming/foo/v1/foo.proto
@@ -0,0 +1,64 @@
+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";
+
+enum CommonEnum {
+  COMMON_ENUM_INVALID = 0;
+}
+
+enum DataEnum {
+  DATA_ENUM_INVALID = 0;
+}
+
+enum UuidEnum {
+  UUID_ENUM_INVALID = 0;
+}
+
+message Common {
+  message UUID {
+    enum HelloUUID {
+      HELLO_UUID_INVALID = 0;
+    }
+  }
+}
+
+message DataCommon {
+
+}
+
+message UUID {
+  reserved "foo_data";
+
+  oneof data {
+    int64 common = 1;
+  }
+}
+
+service FooCommonAPI {
+  rpc Common(Request) returns (Response);
+}
+
+service FooDataAPI {
+  rpc Common(Request) returns (Response);
+}
+
+service FooUUIDAPI {
+  rpc Common(Request) returns (Response);
+}
+
+message Request {}
+
+message Response {}
+
+message Suppressed {
+  // @suppresswarnings naming
+  map<int64, string> common = 1;
+}
diff --git a/internal/cmd/testdata/lint/naming/prototool.yaml b/internal/cmd/testdata/lint/naming/prototool.yaml
new file mode 100644
index 0000000..af881ba
--- /dev/null
+++ b/internal/cmd/testdata/lint/naming/prototool.yaml
@@ -0,0 +1,13 @@
+lint:
+  group: uber2
+  rules:
+    remove:
+      - ENUMS_HAVE_SENTENCE_COMMENTS
+      - FIELDS_NOT_RESERVED
+      - MESSAGES_HAVE_SENTENCE_COMMENTS_EXCEPT_REQUEST_RESPONSE_TYPES
+      - REQUEST_RESPONSE_NAMES_MATCH_RPC
+      - REQUEST_RESPONSE_TYPES_ONLY_IN_FILE
+      - REQUEST_RESPONSE_TYPES_UNIQUE
+      - RPCS_HAVE_SENTENCE_COMMENTS
+      - SERVICES_HAVE_SENTENCE_COMMENTS
+      - SERVICE_NAMES_MATCH_FILE_NAME
diff --git a/internal/cmd/testdata/lint/noplurals/foo/v1/foo.proto b/internal/cmd/testdata/lint/noplurals/foo/v1/foo.proto
index ecebbed..40a0787 100644
--- a/internal/cmd/testdata/lint/noplurals/foo/v1/foo.proto
+++ b/internal/cmd/testdata/lint/noplurals/foo/v1/foo.proto
@@ -36,13 +36,13 @@ service FooOperationsBarAPI {}
 // @suppresswarnings plurals
 service FooOperationsBarsAPI {}
 
-service DataAPI {}
+service InfoAPI {}
 
-service FooDataAPI {}
+service FooInfoAPI {}
 
-service FooDataBarAPI {}
+service FooInfoBarAPI {}
 
-service FooDataBarsAPI {}
+service FooInfoBarsAPI {}
 
 // @suppresswarnings plurals
-service BarDataBarsAPI {}
+service BarInfoBarsAPI {}