From 4ac2f2faaedba07a3b16d90c5b2262fc89a2c668 Mon Sep 17 00:00:00 2001 From: Anthony Amadeo Date: Tue, 28 Nov 2023 04:55:58 +0000 Subject: [PATCH 1/3] feat(AIP-192): Catch unpaired backticks in comments --- docs/rules/0192/closed-backticks.md | 68 +++++++++ rules/aip0192/aip0192.go | 1 + rules/aip0192/closed_backticks.go | 186 +++++++++++++++++++++++++ rules/aip0192/closed_backticks_test.go | 90 ++++++++++++ 4 files changed, 345 insertions(+) create mode 100644 docs/rules/0192/closed-backticks.md create mode 100644 rules/aip0192/closed_backticks.go create mode 100644 rules/aip0192/closed_backticks_test.go diff --git a/docs/rules/0192/closed-backticks.md b/docs/rules/0192/closed-backticks.md new file mode 100644 index 000000000..91ad2f5db --- /dev/null +++ b/docs/rules/0192/closed-backticks.md @@ -0,0 +1,68 @@ +--- +rule: + aip: 192 + name: [core, '0192', closed-backticks] + summary: Inline code should be surrounded by backticks. +permalink: /192/closed-backticks +redirect_from: + - /0192/closed-backticks +--- + +# Closed backticks + +This rule enforces that any inline code in public comments for proto descriptors +is surrounded by backticks, as mandated in [AIP-192][]. + +## Details + +This rule looks at each descriptor in each proto file (exempting oneofs and the +file itself) and complains if _public_ comments include text that only has +either an opening backtick or a closing backtick. + +## Examples + +**Incorrect** code for this rule: + +```proto +// Incorrect. +// Fields on the book include: +// +// - name`: `string +message Book { + // The resource name of the book. + string name = 1; +} +``` + +**Correct** code for this rule: + +```proto +// Correct. +// Fields on the book include: +// +// - `name`: `string` +message Book { + // The resource name of the book. + string name = 1; +} +``` + +## Disabling + +If you need to violate this rule, use a leading comment above the descriptor. +Remember to also include an [aip.dev/not-precedent][] comment explaining why. + +```proto +// Fields on the book include: +// +// - name`: `string +// (-- api-linter: core::0192::closed-backticks=disabled +// aip.dev/not-precedent: We need to do this because reasons. --) +message Book { + // The resource name of the book. + string name = 1; +} +``` + +[aip-192]: https://aip.dev/192 +[aip.dev/not-precedent]: https://aip.dev/not-precedent diff --git a/rules/aip0192/aip0192.go b/rules/aip0192/aip0192.go index 620bff8ac..ae4685fe0 100644 --- a/rules/aip0192/aip0192.go +++ b/rules/aip0192/aip0192.go @@ -25,6 +25,7 @@ func AddRules(r lint.RuleRegistry) error { return r.Register( 192, absoluteLinks, + closedBackticks, deprecatedComment, hasComments, noHTML, diff --git a/rules/aip0192/closed_backticks.go b/rules/aip0192/closed_backticks.go new file mode 100644 index 000000000..cc78b2563 --- /dev/null +++ b/rules/aip0192/closed_backticks.go @@ -0,0 +1,186 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package aip0192 + +import ( + "regexp" + "strings" + + "bitbucket.org/creachadair/stringset" + "github.com/googleapis/api-linter/lint" + "github.com/googleapis/api-linter/rules/internal/utils" + "github.com/jhump/protoreflect/desc" +) + +type backtickPair int + +const ( + unspecified backtickPair = iota + missingOpening + missingClosing + paired +) + +// Represents a backtick character in a line. +// +// To narrow which backticks to lint and the likely intended use of a stray +// backtick, we compute whether the backtick can be used as an opening and/or +// closing backtick. +// +// If the character before a backtick is one of the `separators`, the backtick +// is considered opening. Likewise, if the character after a backtick is one of +// the `separators`, the backtick is considered closing. +type backtick struct { + index int + opening bool + closing bool + pair backtickPair +} + +var separators = stringset.New(" ", ":", ",", ".", ";", "-") +var reBacktick = regexp.MustCompile("`") + +var closedBackticks = &lint.DescriptorRule{ + Name: lint.NewRuleName(192, "closed-backticks"), + LintDescriptor: func(d desc.Descriptor) []lint.Problem { + problems := []lint.Problem{} + + for _, comment := range utils.SeparateInternalComments(d.GetSourceInfo().GetLeadingComments()).External { + for _, line := range strings.Split(comment, "\n") { + // Pad the line with whitespace, so it's easier to lint + // backticks at the start and end of the line. + line = " " + line + " " + + // Find all the backticks in the line and compute whether each + // one can be used as an opening and/or closing backtick. + backticks := []backtick{} + for _, loc := range reBacktick.FindAllStringIndex(line, -1) { + backticks = append(backticks, backtickAtIndex(line, loc[0])) + } + + // Compute whether the backticks are paired. + backticks = computeBacktickPairs(filterUnusableBackticks(backticks)) + + // Add a problem for each backtick that is missing a pair. + for _, backtick := range backticks { + if backtick.pair != paired { + problems = append(problems, lint.Problem{ + Message: "Inline code should be surrounded by backticks.", + Suggestion: "`" + suggestionWord(line, backtick) + "`", + Descriptor: d, + }) + } + } + } + } + return problems + }, +} + +func backtickAtIndex(line string, index int) backtick { + return backtick{ + index: index, + opening: separators.Contains(string(line[index-1])), + closing: separators.Contains(string(line[index+1])), + } +} + +// Filter out backticks that cannot be used as opening or closing backticks. +func filterUnusableBackticks(backticks []backtick) []backtick { + list := []backtick{} + for _, backtick := range backticks { + if backtick.opening || backtick.closing { + list = append(list, backtick) + } + } + return list +} + +// Compute whether each backtick is paired with an adjacent backtick. +// +// A backtick pair consists of a set of adjacent backticks, where the first is +// opening and the second is closing. Each backtick can be in at most one pair. +// +// This fills in the `pair` field for each backtick. It assumes every backtick +// is either opening, closing, or both. +func computeBacktickPairs(backticks []backtick) []backtick { + computed := []backtick{} + + prevBacktickOpen := false + for i, backtick := range backticks { + backtick.pair = computePairState(backtick, prevBacktickOpen) + + // If this backtick got paired, mark the previous one as paired as well. + if backtick.pair == paired { + computed[i-1].pair = paired + } + + // If paired, this backtick cannot be used as an `opening` for another + // backtick. + prevBacktickOpen = false + if backtick.opening && backtick.pair != paired { + prevBacktickOpen = true + } + + computed = append(computed, backtick) + } + return computed +} + +func computePairState(backtick backtick, prevBacktickOpen bool) backtickPair { + // If the backtick is only opening, it needs a closing backtick. + if backtick.opening && !backtick.closing { + return missingClosing + } + // Otherwise, the backtick is closing. It is paired if the last backtick was + // opening. + if prevBacktickOpen { + return paired + } + return missingOpening +} + +// Extract the word around an unpaired backtick. +// +// Even though inline code can include separators (e.g. colons, spaces), just +// suggest the string of characters up to the first separator character. +func suggestionWord(line string, backtick backtick) string { + switch backtick.pair { + case missingOpening: + return reverseString(firstWordBeforeSeparator(reverseString(line[:backtick.index]))) + case missingClosing: + return firstWordBeforeSeparator(line[backtick.index+1:]) + default: + return "" + } +} + +func firstWordBeforeSeparator(s string) string { + index := strings.IndexFunc(s, func(r rune) bool { + return separators.Contains(string(r)) + }) + if index != -1 { + return s[:index] + } + return s +} + +func reverseString(s string) string { + runes := []rune(s) + for i, j := 0, len(runes)-1; i < j; i, j = i+1, j-1 { + runes[i], runes[j] = runes[j], runes[i] + } + return string(runes) +} diff --git a/rules/aip0192/closed_backticks_test.go b/rules/aip0192/closed_backticks_test.go new file mode 100644 index 000000000..4f0fa33f0 --- /dev/null +++ b/rules/aip0192/closed_backticks_test.go @@ -0,0 +1,90 @@ +// Copyright 2023 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package aip0192 + +import ( + "testing" + + "github.com/googleapis/api-linter/rules/internal/testutils" +) + +func TestClosedBackticks(t *testing.T) { + for _, test := range []struct { + name string + Comment string + problems testutils.Problems + }{ + {"ValidWordChars", "`foo_bar_baz_1`", nil}, + {"MissingFrontBacktickWordChars", "foo_bar_baz_1`", testutils.Problems{{Suggestion: "`foo_bar_baz_1`"}}}, + {"MissingBackBacktickWordChars", "`foo_bar_baz_1", testutils.Problems{{Suggestion: "`foo_bar_baz_1`"}}}, + + {"ValidBetweenWords", "foo `bar` baz", nil}, + {"MissingFrontBacktickBetweenWords", "foo bar` baz", testutils.Problems{{Suggestion: "`bar`"}}}, + {"MissingBackBacktickBetweenWords", "foo `bar baz", testutils.Problems{{Suggestion: "`bar`"}}}, + + {"ValidNonWordChars", "`foo:bar/baz->qux.quux[0]()`", nil}, + {"MissingFrontBacktickNonWordChars", "foo:bar/baz->qux.quux[0]()`", testutils.Problems{{Suggestion: "`quux[0]()`"}}}, + {"MissingBackBacktickNonWordChars", "`foo:bar/baz->qux.quux[0]()", testutils.Problems{{Suggestion: "`foo`"}}}, + + {"ValidContainsSpace", "`foo + bar`", nil}, + {"MissingFrontBacktickContainsSpace", "foo + bar`", testutils.Problems{{Suggestion: "`bar`"}}}, + {"MissingBackBacktickContainsSpace", "`foo + bar", testutils.Problems{{Suggestion: "`foo`"}}}, + + {"ValidNonAscii", "`汉语`", nil}, + {"MissingFrontBacktickNonAscii", "汉语`", testutils.Problems{{Suggestion: "`汉语`"}}}, + {"MissingBackBacktickNonAscii", "`汉语", testutils.Problems{{Suggestion: "`汉语`"}}}, + + {"ValidQuotes", "`\"foo\"`", nil}, + {"MissingFrontBacktickQuotes", "\"foo\"`", testutils.Problems{{Suggestion: "`\"foo\"`"}}}, + {"MissingBackBacktickQuotes", "`\"foo\"", testutils.Problems{{Suggestion: "`\"foo\"`"}}}, + + {"ValidSeparatedByColons", "foo:`bar:baz`:qux", nil}, + {"MissingFrontBacktickSeparatedByColons", "foo:`bar:baz:qux", testutils.Problems{{Suggestion: "`bar`"}}}, + {"MissingBackBacktickSeparatedByColons", "foo:bar:baz`:qux", testutils.Problems{{Suggestion: "`baz`"}}}, + + {"ValidComma", "`name`, a string", nil}, + {"MissingFrontBacktickComma", "name`, a string", testutils.Problems{{Suggestion: "`name`"}}}, + {"MissingBackBacktickComma", "`name, a string", testutils.Problems{{Suggestion: "`name`"}}}, + + {"ValidMultipleCodeText", "`name`: `string`", nil}, + {"MissingFrontBacktickMultipleCodeText", "name`: string`", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, + {"MissingBackBacktickMultipleCodeText", "`name: `string", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, + {"MissingFrontAndBackBacktickMultipleCodeText", "name`: `string", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, + + {"ValidContainsColon", "`name: string`", nil}, + + {"ValidContainsSeparator", "`.`", nil}, + {"MissingFrontBacktickContainsSeparator", ".`", testutils.Problems{{Suggestion: "``"}}}, + {"MissingBackBacktickContainsSeparator", "`.", testutils.Problems{{Suggestion: "``"}}}, + + {"ValidEmptySpace", "``", nil}, + {"ValidEmptySpaces", "`` ``", nil}, + {"UnpairedEmptySpace", "`` `", testutils.Problems{{Suggestion: "``"}}}, + + {"IgnoreTripleBacktick", "```", nil}, + {"IgnoreBackticksWithinWords", "a`b`c`d", nil}, + } { + t.Run(test.name, func(t *testing.T) { + f := testutils.ParseProto3Tmpl(t, ` + // {{.Comment}} + message Foo {} + `, test) + m := f.GetMessageTypes()[0] + if diff := test.problems.SetDescriptor(m).Diff(closedBackticks.Lint(f)); diff != "" { + t.Errorf(diff) + } + }) + } +} From fc0804c72afb65ff811cf2862e5aca33fcc571fe Mon Sep 17 00:00:00 2001 From: Anthony Amadeo Date: Wed, 29 Nov 2023 03:07:47 +0000 Subject: [PATCH 2/3] Minor edits --- rules/aip0192/closed_backticks.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rules/aip0192/closed_backticks.go b/rules/aip0192/closed_backticks.go index cc78b2563..0d0f6a564 100644 --- a/rules/aip0192/closed_backticks.go +++ b/rules/aip0192/closed_backticks.go @@ -129,10 +129,7 @@ func computeBacktickPairs(backticks []backtick) []backtick { // If paired, this backtick cannot be used as an `opening` for another // backtick. - prevBacktickOpen = false - if backtick.opening && backtick.pair != paired { - prevBacktickOpen = true - } + prevBacktickOpen = backtick.opening && backtick.pair != paired computed = append(computed, backtick) } From 0953c5b6900da750c2539586a62915e820dae672 Mon Sep 17 00:00:00 2001 From: Anthony Amadeo Date: Wed, 29 Nov 2023 14:37:52 +0000 Subject: [PATCH 3/3] More minor edits --- rules/aip0192/closed_backticks.go | 8 ++++---- rules/aip0192/closed_backticks_test.go | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/rules/aip0192/closed_backticks.go b/rules/aip0192/closed_backticks.go index 0d0f6a564..621558f9d 100644 --- a/rules/aip0192/closed_backticks.go +++ b/rules/aip0192/closed_backticks.go @@ -70,10 +70,10 @@ var closedBackticks = &lint.DescriptorRule{ backticks = append(backticks, backtickAtIndex(line, loc[0])) } - // Compute whether the backticks are paired. + // Compute which backticks are paired. backticks = computeBacktickPairs(filterUnusableBackticks(backticks)) - // Add a problem for each backtick that is missing a pair. + // Add a problem for any backtick that is missing a pair. for _, backtick := range backticks { if backtick.pair != paired { problems = append(problems, lint.Problem{ @@ -110,7 +110,7 @@ func filterUnusableBackticks(backticks []backtick) []backtick { // Compute whether each backtick is paired with an adjacent backtick. // -// A backtick pair consists of a set of adjacent backticks, where the first is +// A backtick pair consists of two adjacent backticks, where the first is // opening and the second is closing. Each backtick can be in at most one pair. // // This fills in the `pair` field for each backtick. It assumes every backtick @@ -122,7 +122,7 @@ func computeBacktickPairs(backticks []backtick) []backtick { for i, backtick := range backticks { backtick.pair = computePairState(backtick, prevBacktickOpen) - // If this backtick got paired, mark the previous one as paired as well. + // If this backtick got paired, mark the previous one paired as well. if backtick.pair == paired { computed[i-1].pair = paired } diff --git a/rules/aip0192/closed_backticks_test.go b/rules/aip0192/closed_backticks_test.go index 4f0fa33f0..a0831b267 100644 --- a/rules/aip0192/closed_backticks_test.go +++ b/rules/aip0192/closed_backticks_test.go @@ -58,10 +58,10 @@ func TestClosedBackticks(t *testing.T) { {"MissingFrontBacktickComma", "name`, a string", testutils.Problems{{Suggestion: "`name`"}}}, {"MissingBackBacktickComma", "`name, a string", testutils.Problems{{Suggestion: "`name`"}}}, - {"ValidMultipleCodeText", "`name`: `string`", nil}, - {"MissingFrontBacktickMultipleCodeText", "name`: string`", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, - {"MissingBackBacktickMultipleCodeText", "`name: `string", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, - {"MissingFrontAndBackBacktickMultipleCodeText", "name`: `string", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, + {"ValidMultipleInlineCode", "`name`: `string`", nil}, + {"MissingFrontBacktickMultipleInlineCode", "name`: string`", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, + {"MissingBackBacktickMultipleInlineCode", "`name: `string", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, + {"MissingFrontAndBackBacktickMultipleInlineCode", "name`: `string", testutils.Problems{{Suggestion: "`name`"}, {Suggestion: "`string`"}}}, {"ValidContainsColon", "`name: string`", nil},