Skip to content

Commit

Permalink
lint: add a linter to prohibit map[...]bool in favor of map[...]struct{}
Browse files Browse the repository at this point in the history
The linter is currently limited to `sql/opt/norm` package. This commit
was prompted by an article mentioned in the Golang Weekly newsletter.
The rationale is that `map[...]struct{}` is more efficient, but in some
cases the bool map value is actually desired, so this commit introduces
an opt-out `//nolint:maptobool` comment.

Release justification: low-risk performance improvement.

Release note: None
  • Loading branch information
yuzefovich committed Aug 19, 2022
1 parent 2e8c0f6 commit fd6be33
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 3 deletions.
6 changes: 3 additions & 3 deletions pkg/sql/opt/norm/groupby_funcs.go
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ func (c *CustomFuncs) AreValuesDistinct(
func (c *CustomFuncs) areRowsDistinct(
rows memo.ScalarListExpr, cols opt.ColList, groupingCols opt.ColSet, nullsAreDistinct bool,
) bool {
var seen map[string]bool
var seen map[string]struct{}
var encoded []byte
for _, scalar := range rows {
row := scalar.(*memo.TupleExpr)
Expand Down Expand Up @@ -263,7 +263,7 @@ func (c *CustomFuncs) areRowsDistinct(
}

if seen == nil {
seen = make(map[string]bool, len(rows))
seen = make(map[string]struct{}, len(rows))
}

// Determine whether key has already been seen.
Expand All @@ -274,7 +274,7 @@ func (c *CustomFuncs) areRowsDistinct(
}

// Add the key to the seen map.
seen[key] = true
seen[key] = struct{}{}
}

return true
Expand Down
41 changes: 41 additions & 0 deletions pkg/testutils/lint/lint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2073,6 +2073,47 @@ func TestLint(t *testing.T) {
}
})

// TestMapToBool asserts that map[...]bool is not used. In most cases, such
// a map can be replaced with map[...]struct{} that is more efficient, and
// this linter nudges folks to do so. This linter can be disabled by
// '//nolint:maptobool' comment.
// TODO(yuzefovich): expand the scope where the linter is applied.
t.Run("TestMapToBool", func(t *testing.T) {
t.Parallel()
cmd, stderr, filter, err := dirCmd(
pkgDir,
"git",
"grep",
"-nE",
`map\[.*\]bool`,
"--",
"sql/opt/norm*.go",
":!*_test.go",
)
if err != nil {
t.Fatal(err)
}

if err := cmd.Start(); err != nil {
t.Fatal(err)
}

if err := stream.ForEach(stream.Sequence(
filter,
stream.GrepNot(`nolint:maptobool`),
), func(s string) {
t.Errorf("\n%s <- forbidden; use map[...]struct{} instead", s)
}); err != nil {
t.Error(err)
}

if err := cmd.Wait(); err != nil {
if out := stderr.String(); len(out) > 0 {
t.Fatalf("err=%s, stderr=%s", err, out)
}
}
})

// RoachVet is expensive memory-wise and thus should not run with t.Parallel().
// RoachVet includes all of the passes of `go vet` plus first-party additions.
// See pkg/cmd/roachvet.
Expand Down

0 comments on commit fd6be33

Please sign in to comment.