Repository: uber/prototool
Pull request #281 (2019-01-10 00:28:56)

Add linter to check for keywords in the package name
Re: #265.

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

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

```diff
@@            Coverage Diff             @@
##              dev     #281      +/-   ##
==========================================
+ Coverage   72.15%   72.22%   +0.06%     
==========================================
  Files         120      121       +1     
  Lines        5413     5425      +12     
==========================================
+ Hits         3906     3918      +12     
  Misses       1135     1135              
  Partials      372      372
```


| [Impacted Files](https://codecov.io/gh/uber/prototool/pull/281?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/lint/lint.go](https://codecov.io/gh/uber/prototool/pull/281/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9saW50Lmdv) | `79.2% <ø> (ø)` | :arrow_up: |
| [internal/lint/check\_package\_no\_keywords.go](https://codecov.io/gh/uber/prototool/pull/281/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbGludC9jaGVja19wYWNrYWdlX25vX2tleXdvcmRzLmdv) | `100% <100%> (ø)` | |

------

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


Patch:
diff --git a/README.md b/README.md
index 923a2e6..5ffcdac 100644
--- a/README.md
+++ b/README.md
@@ -177,6 +177,12 @@ Lint rules can be set using the configuration file. See the configuration at [et
 - `uber2`: This lint group is the v2 of the `uber` lint group, and makes some modifcations to more closely follow the Google Cloud APIs file
   structure, as well as adding even more rules to enforce more consistent development patterns. This lint group is under development.
 
+To see the differences between lint groups, use the `--diff-lint-groups` flag:
+
+```
+prototool lint --diff-lint-groups google,uber
+```
+
 Configuration of your group can be done by setting the `lint.group` option in your `prototool.yaml` file:
 
 ```yaml
@@ -197,10 +203,16 @@ to the element comment. The following lint rules can be suppressed:
 | Lint Rule                   | Containing Lint Groups   | Suppressing Annotation   |
 |-----------------------------|--------------------------|--------------------------|
 | `MESSAGE_FIELDS_NOT_FLOATS` | `uber2`                  | `floats`                 |
+| `PACKAGE_NO_KEYWORDS`       | `uber2`                  | `keywords`               |
 
 As an example:
 
 ```
+// This contains the "public". keyword.
+//
+// @suppresswarnings keywords
+package foo.public.bar;
+
 // Hello is a field where we understand the concerns with using doubles but require them.
 //
 // @suppresswarnings floats
@@ -464,6 +476,8 @@ major version, with some exceptions:
   reflect things such as max line lengths.
 - The breaking change detector may have additional checks added between minor versions, and therefore a change that might not have been
   breaking previously might become a breaking change.
+- The `PACKAGE_NO_KEYWORDS` linter on the `uber2` lint group may have additional keywords added. This can be suppressed by adding
+  `@suppresswarnings keywords` to the package comment.
 
 ## Development
 
diff --git a/internal/cmd/flags.go b/internal/cmd/flags.go
index 38916d5..c16e85a 100644
--- a/internal/cmd/flags.go
+++ b/internal/cmd/flags.go
@@ -32,6 +32,7 @@ type flags struct {
 	connectTimeout    string
 	data              string
 	debug             bool
+	diffLintGroups    string
 	diffMode          bool
 	disableFormat     bool
 	disableLint       bool
@@ -88,6 +89,10 @@ func (f *flags) bindDebug(flagSet *pflag.FlagSet) {
 	flagSet.BoolVar(&f.debug, "debug", false, "Run in debug mode, which will print out debug logging.")
 }
 
+func (f *flags) bindDiffLintGroups(flagSet *pflag.FlagSet) {
+	flagSet.StringVar(&f.diffLintGroups, "diff-lint-groups", "", "Diff the two lint groups separated by '.', for example google,uber.")
+}
+
 func (f *flags) bindDiffMode(flagSet *pflag.FlagSet) {
 	flagSet.BoolVarP(&f.diffMode, "diff", "d", false, "Write a diff instead of writing the formatted file to stdout.")
 }
diff --git a/internal/cmd/templates.go b/internal/cmd/templates.go
index d183d60..ce47975 100644
--- a/internal/cmd/templates.go
+++ b/internal/cmd/templates.go
@@ -458,7 +458,7 @@ sys	0m0.924s`,
 
 		Args: cobra.MaximumNArgs(1),
 		Run: func(runner exec.Runner, args []string, flags *flags) error {
-			return runner.Lint(args, flags.listAllLinters, flags.listLinters, flags.listAllLintGroups, flags.listLintGroup)
+			return runner.Lint(args, flags.listAllLinters, flags.listLinters, flags.listAllLintGroups, flags.listLintGroup, flags.diffLintGroups)
 		},
 		BindFlags: func(flagSet *pflag.FlagSet, flags *flags) {
 			flags.bindCachePath(flagSet)
@@ -468,6 +468,7 @@ sys	0m0.924s`,
 			flags.bindListLinters(flagSet)
 			flags.bindListAllLintGroups(flagSet)
 			flags.bindListLintGroup(flagSet)
+			flags.bindDiffLintGroups(flagSet)
 			flags.bindProtocURL(flagSet)
 			flags.bindProtocBinPath(flagSet)
 			flags.bindProtocWKTPath(flagSet)
diff --git a/internal/exec/exec.go b/internal/exec/exec.go
index 3a3c4f8..149d094 100644
--- a/internal/exec/exec.go
+++ b/internal/exec/exec.go
@@ -53,7 +53,7 @@ type Runner interface {
 	Files(args []string) error
 	Compile(args []string, dryRun bool) error
 	Gen(args []string, dryRun bool) error
-	Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string) error
+	Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string, diffLintGroups string) error
 	Format(args []string, overwrite, diffMode, lintMode, fix bool) error
 	All(args []string, disableFormat, disableLint, fix bool) error
 	GRPC(args, headers []string, address, method, data, callTimeout, connectTimeout, keepaliveTime string, stdin bool) error
diff --git a/internal/exec/runner.go b/internal/exec/runner.go
index 7286e88..fc5536d 100644
--- a/internal/exec/runner.go
+++ b/internal/exec/runner.go
@@ -272,9 +272,9 @@ func (r *runner) printCommands(doGen bool, protoSet *file.ProtoSet) error {
 	return nil
 }
 
-func (r *runner) Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string) error {
-	if moreThanOneSet(listAllLinters, listLinters, listAllLintGroups, listLintGroup != "") {
-		return newExitErrorf(255, "can only set one of list-all-linters, list-linters, list-all-lint-groups, list-lint-group")
+func (r *runner) Lint(args []string, listAllLinters bool, listLinters bool, listAllLintGroups bool, listLintGroup string, diffLintGroups string) error {
+	if moreThanOneSet(listAllLinters, listLinters, listAllLintGroups, listLintGroup != "", diffLintGroups != "") {
+		return newExitErrorf(255, "can only set one of list-all-linters, list-linters, list-all-lint-groups, list-lint-group, diff-lint-groups")
 	}
 	if listAllLinters {
 		return r.listAllLinters()
@@ -288,6 +288,9 @@ func (r *runner) Lint(args []string, listAllLinters bool, listLinters bool, list
 	if listLintGroup != "" {
 		return r.listLintGroup(listLintGroup)
 	}
+	if diffLintGroups != "" {
+		return r.diffLintGroups(diffLintGroups)
+	}
 	meta, err := r.getMeta(args)
 	if err != nil {
 		return err
@@ -352,6 +355,27 @@ func (r *runner) listAllLintGroups() error {
 	return nil
 }
 
+func (r *runner) diffLintGroups(groups string) error {
+	split := strings.Split(groups, ",")
+	if len(split) != 2 {
+		return fmt.Errorf("argument to --diff-lint-groups must be two lint groups separated by '.', for example google,uber")
+	}
+	firstLinterIDs, err := getLinterIDs(split[0])
+	if err != nil {
+		return err
+	}
+	secondLinterIDs, err := getLinterIDs(split[1])
+	if err != nil {
+		return err
+	}
+	for _, s := range diffMaps(firstLinterIDs, secondLinterIDs) {
+		if err := r.println(s); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func (r *runner) Format(args []string, overwrite, diffMode, lintMode, fix bool) error {
 	if moreThanOneSet(overwrite, diffMode, lintMode) {
 		return newExitErrorf(255, "can only set one of overwrite, diff, lint")
@@ -988,3 +1012,31 @@ func extractSortPackageNames(m map[string]*extract.Package) []string {
 	sort.Strings(s)
 	return s
 }
+
+func getLinterIDs(group string) (map[string]struct{}, error) {
+	linters, ok := lint.GroupToLinters[strings.ToLower(group)]
+	if !ok {
+		return nil, newExitErrorf(255, "unknown lint group: %s", strings.ToLower(group))
+	}
+	m := make(map[string]struct{})
+	for _, linter := range linters {
+		m[linter.ID()] = struct{}{}
+	}
+	return m, nil
+}
+
+func diffMaps(one map[string]struct{}, two map[string]struct{}) []string {
+	var s []string
+	for key := range one {
+		if _, ok := two[key]; !ok {
+			s = append(s, "< "+key)
+		}
+	}
+	for key := range two {
+		if _, ok := one[key]; !ok {
+			s = append(s, "> "+key)
+		}
+	}
+	sort.Strings(s)
+	return s
+}
diff --git a/internal/lint/check_package_no_keywords.go b/internal/lint/check_package_no_keywords.go
new file mode 100644
index 0000000..4568691
--- /dev/null
+++ b/internal/lint/check_package_no_keywords.go
@@ -0,0 +1,77 @@
+// 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"
+
+	"github.com/emicklei/proto"
+	"github.com/uber/prototool/internal/text"
+)
+
+const packageNoKeywordsSuppressableAnnotation = "keywords"
+
+var (
+	packageNoKeywordsKeywords = []string{
+		"internal",  // Golang
+		"public",    // Java, C++, others
+		"private",   // Java, C++, others
+		"protected", // Java, C++, others
+		"std",       // C++ (causes a problem with the std package)
+	}
+
+	packageNoKeywordsLinter = NewLinter(
+		"PACKAGE_NO_KEYWORDS",
+		fmt.Sprintf(`Suppressable with "@suppresswarnings %s". Verifies that no packages contain one of the keywords "%s" as part of the name when split on '.'.`, packageNoKeywordsSuppressableAnnotation, strings.Join(packageNoKeywordsKeywords, ",")),
+		checkPackageNoKeywords,
+	)
+
+	packageNoKeywordsKeywordsMap = make(map[string]struct{}, len(packageNoKeywordsKeywords))
+)
+
+func init() {
+	for _, keyword := range packageNoKeywordsKeywords {
+		packageNoKeywordsKeywordsMap[keyword] = struct{}{}
+	}
+
+}
+
+func checkPackageNoKeywords(add func(*text.Failure), dirPath string, descriptors []*proto.Proto) error {
+	return runVisitor(packageNoKeywordsVisitor{baseAddVisitor: newBaseAddVisitor(add)}, descriptors)
+}
+
+type packageNoKeywordsVisitor struct {
+	baseAddVisitor
+}
+
+func (v packageNoKeywordsVisitor) VisitPackage(pkg *proto.Package) {
+	for _, subPackage := range strings.Split(pkg.Name, ".") {
+		potentialKeyword := strings.ToLower(subPackage)
+		if _, ok := packageNoKeywordsKeywordsMap[potentialKeyword]; ok {
+			if isSuppressed(pkg.Comment, packageNoKeywordsSuppressableAnnotation) {
+				// can just return as this will be true for other keywords as well
+				return
+			}
+			v.AddFailuref(pkg.Position, `Package %q contains the keyword %q, this could cause problems in generated code. This can be suppressed by adding "@suppresswarnings %s" to the package comment.`, pkg.Name, potentialKeyword, packageNoKeywordsSuppressableAnnotation)
+		}
+	}
+}
diff --git a/internal/lint/lint.go b/internal/lint/lint.go
index 442bfd0..e7cbe2e 100644
--- a/internal/lint/lint.go
+++ b/internal/lint/lint.go
@@ -78,6 +78,7 @@ var (
 		packageIsDeclaredLinter,
 		packageLowerSnakeCaseLinter,
 		packageMajorBetaVersionedLinter,
+		packageNoKeywordsLinter,
 		packagesSameInDirLinter,
 		rpcsHaveCommentsLinter,
 		rpcNamesCamelCaseLinter,
@@ -181,6 +182,7 @@ var (
 		packageIsDeclaredLinter,
 		packageLowerSnakeCaseLinter,
 		packageMajorBetaVersionedLinter,
+		packageNoKeywordsLinter,
 		packagesSameInDirLinter,
 		rpcNamesCamelCaseLinter,
 		rpcNamesCapitalizedLinter,


Test patch:
diff --git a/internal/cmd/cmd_test.go b/internal/cmd/cmd_test.go
index bad4716..57b964d 100644
--- a/internal/cmd/cmd_test.go
+++ b/internal/cmd/cmd_test.go
@@ -392,10 +392,10 @@ func TestLint(t *testing.T) {
 		t,
 		false,
 		`11:3:FIELDS_NOT_RESERVED
-                 12:3:FIELDS_NOT_RESERVED
-                 14:5:FIELDS_NOT_RESERVED
-                 15:5:FIELDS_NOT_RESERVED
-                 20:5:FIELDS_NOT_RESERVED`,
+		12:3:FIELDS_NOT_RESERVED
+		14:5:FIELDS_NOT_RESERVED
+		15:5:FIELDS_NOT_RESERVED
+		20:5:FIELDS_NOT_RESERVED`,
 		"testdata/lint/noreserved/foo.proto",
 	)
 
@@ -435,6 +435,19 @@ func TestLint(t *testing.T) {
 		"testdata/lint/floats/foo/v1/foo.proto",
 	)
 
+	assertDoLintFile(
+		t,
+		false,
+		`3:1:PACKAGE_NO_KEYWORDS`,
+		"testdata/lint/nokeywords/foo/public/public.proto",
+	)
+	assertDoLintFile(
+		t,
+		false,
+		``,
+		"testdata/lint/nokeywords/foo/public/public_suppressed.proto",
+	)
+
 	assertDoLintFile(
 		t,
 		false,
@@ -768,6 +781,38 @@ func TestListLintGroup(t *testing.T) {
 	assertLinters(t, lint.Uber2Linters, "lint", "--list-lint-group", "uber2")
 }
 
+func TestDiffLintGroups(t *testing.T) {
+	assertExact(
+		t,
+		0,
+		`> COMMENTS_NO_C_STYLE
+> ENUMS_NO_ALLOW_ALIAS
+> ENUM_FIELD_PREFIXES
+> ENUM_ZERO_VALUES_INVALID
+> FILE_OPTIONS_EQUAL_GO_PACKAGE_PB_SUFFIX
+> FILE_OPTIONS_EQUAL_JAVA_MULTIPLE_FILES_TRUE
+> FILE_OPTIONS_EQUAL_JAVA_OUTER_CLASSNAME_PROTO_SUFFIX
+> FILE_OPTIONS_EQUAL_JAVA_PACKAGE_COM_PREFIX
+> FILE_OPTIONS_GO_PACKAGE_NOT_LONG_FORM
+> FILE_OPTIONS_GO_PACKAGE_SAME_IN_DIR
+> FILE_OPTIONS_JAVA_MULTIPLE_FILES_SAME_IN_DIR
+> FILE_OPTIONS_JAVA_PACKAGE_SAME_IN_DIR
+> FILE_OPTIONS_REQUIRE_GO_PACKAGE
+> FILE_OPTIONS_REQUIRE_JAVA_MULTIPLE_FILES
+> FILE_OPTIONS_REQUIRE_JAVA_OUTER_CLASSNAME
+> FILE_OPTIONS_REQUIRE_JAVA_PACKAGE
+> ONEOF_NAMES_LOWER_SNAKE_CASE
+> PACKAGES_SAME_IN_DIR
+> PACKAGE_IS_DECLARED
+> PACKAGE_LOWER_SNAKE_CASE
+> REQUEST_RESPONSE_TYPES_IN_SAME_FILE
+> REQUEST_RESPONSE_TYPES_UNIQUE
+> SYNTAX_PROTO3
+> WKT_DIRECTLY_IMPORTED`,
+		"lint", "--diff-lint-groups", "google,uber",
+	)
+}
+
 func assertLinters(t *testing.T, linters []lint.Linter, args ...string) {
 	linterIDs := make([]string, 0, len(linters))
 	for _, linter := range linters {
diff --git a/internal/cmd/testdata/lint/nokeywords/foo/public/public.proto b/internal/cmd/testdata/lint/nokeywords/foo/public/public.proto
new file mode 100644
index 0000000..1151acb
--- /dev/null
+++ b/internal/cmd/testdata/lint/nokeywords/foo/public/public.proto
@@ -0,0 +1,8 @@
+syntax = "proto3";
+
+package foo.public;
+
+option go_package = "publicpb";
+option java_multiple_files = true;
+option java_outer_classname = "PublicProto";
+option java_package = "com.foo.public";
diff --git a/internal/cmd/testdata/lint/nokeywords/foo/public/public_suppressed.proto b/internal/cmd/testdata/lint/nokeywords/foo/public/public_suppressed.proto
new file mode 100644
index 0000000..d3955c3
--- /dev/null
+++ b/internal/cmd/testdata/lint/nokeywords/foo/public/public_suppressed.proto
@@ -0,0 +1,9 @@
+syntax = "proto3";
+
+// @suppresswarnings keywords
+package foo.public;
+
+option go_package = "publicpb";
+option java_multiple_files = true;
+option java_outer_classname = "PublicSuppressedProto";
+option java_package = "com.foo.public";
diff --git a/internal/cmd/testdata/lint/nokeywords/prototool.yaml b/internal/cmd/testdata/lint/nokeywords/prototool.yaml
new file mode 100644
index 0000000..6de555e
--- /dev/null
+++ b/internal/cmd/testdata/lint/nokeywords/prototool.yaml
@@ -0,0 +1,4 @@
+lint:
+  rules:
+    add:
+      - PACKAGE_NO_KEYWORDS