From b58377e26b16f488d2e0b1ebf2bb1a83c17d6265 Mon Sep 17 00:00:00 2001 From: Peter Edge Date: Wed, 9 Jan 2019 19:25:21 -0500 Subject: [PATCH 1/3] Add linter to check for keywords in the package name --- README.md | 8 ++ internal/cmd/cmd_test.go | 21 ++++- .../lint/nokeywords/foo/public/public.proto | 8 ++ .../foo/public/public_suppressed.proto | 9 +++ .../package_starts_with_keyword.proto | 3 + .../testdata/lint/nokeywords/prototool.yaml | 4 + internal/lint/check_package_no_keywords.go | 77 +++++++++++++++++++ internal/lint/lint.go | 2 + 8 files changed, 128 insertions(+), 4 deletions(-) create mode 100644 internal/cmd/testdata/lint/nokeywords/foo/public/public.proto create mode 100644 internal/cmd/testdata/lint/nokeywords/foo/public/public_suppressed.proto create mode 100644 internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto create mode 100644 internal/cmd/testdata/lint/nokeywords/prototool.yaml create mode 100644 internal/lint/check_package_no_keywords.go diff --git a/README.md b/README.md index 923a2e67..e25bcb47 100644 --- a/README.md +++ b/README.md @@ -197,10 +197,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 +470,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/cmd_test.go b/internal/cmd/cmd_test.go index bad4716b..a2419b83 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, 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 00000000..1151acb8 --- /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 00000000..d3955c39 --- /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/package_starts_with_keyword.proto b/internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto new file mode 100644 index 00000000..63668e61 --- /dev/null +++ b/internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto @@ -0,0 +1,3 @@ +syntax = "proto3"; + +package group.v1; diff --git a/internal/cmd/testdata/lint/nokeywords/prototool.yaml b/internal/cmd/testdata/lint/nokeywords/prototool.yaml new file mode 100644 index 00000000..6de555e4 --- /dev/null +++ b/internal/cmd/testdata/lint/nokeywords/prototool.yaml @@ -0,0 +1,4 @@ +lint: + rules: + add: + - PACKAGE_NO_KEYWORDS diff --git a/internal/lint/check_package_no_keywords.go b/internal/lint/check_package_no_keywords.go new file mode 100644 index 00000000..004d31f6 --- /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, 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 442bfd0c..e7cbe2e4 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, From 8917b7635c4b86b8465119145b09ac1d20468e15 Mon Sep 17 00:00:00 2001 From: Peter Edge Date: Wed, 9 Jan 2019 19:27:16 -0500 Subject: [PATCH 2/3] Remove extea file --- .../testdata/lint/nokeywords/package_starts_with_keyword.proto | 3 --- 1 file changed, 3 deletions(-) delete mode 100644 internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto diff --git a/internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto b/internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto deleted file mode 100644 index 63668e61..00000000 --- a/internal/cmd/testdata/lint/nokeywords/package_starts_with_keyword.proto +++ /dev/null @@ -1,3 +0,0 @@ -syntax = "proto3"; - -package group.v1; From 6c7a32147ad080429a709fe8146abe417606bdba Mon Sep 17 00:00:00 2001 From: Peter Edge Date: Wed, 9 Jan 2019 19:34:32 -0500 Subject: [PATCH 3/3] Comment --- internal/lint/check_package_no_keywords.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/lint/check_package_no_keywords.go b/internal/lint/check_package_no_keywords.go index 004d31f6..4568691a 100644 --- a/internal/lint/check_package_no_keywords.go +++ b/internal/lint/check_package_no_keywords.go @@ -35,7 +35,7 @@ var ( "internal", // Golang "public", // Java, C++, others "private", // Java, C++, others - "protected", // Java, others + "protected", // Java, C++, others "std", // C++ (causes a problem with the std package) }