Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add makezero linter #1520

Merged
merged 9 commits into from
Dec 5, 2020
Merged

Add makezero linter #1520

merged 9 commits into from
Dec 5, 2020

Conversation

ashanbrown
Copy link
Contributor

@ashanbrown ashanbrown commented Nov 22, 2020

Would love to hear some thoughts on this linter. In the interest of full disclosure, it was rejected by gocritic (see go-critic/go-critic#875). In practice, it has been immensely useful to my team. Thanks for your consideration. Details follow:

Makezero discourages the creation of slices with nonzero capacity but zero length. Its primary goal is to prevent appending to an initialized slice with nonzero length, but it can be used to disable uninitialized slices with nonzero length altogether. A common error among go programmers, particular when egged on by the useful prealloc linter, is this one:

x := make([]int, len(y))
for _, v := range y {
  x = append(x, v)
}

In this case, the programmer has forgotten to set the initial length of the array to 0 with make([]int, 0, len(y). In practice, my company has found it useful to any make calls with nonzero length, because the need to optimize by avoiding append statements is limited. We also like seeing append statements because they discourage the additional indirection introduced by indices needed to access elements in a preallocated non-zero length array (i.e. we don't see the index variable i all over the place like you'd see in C-language programs). We favorable readability of code over performance in almost all cases, adding nolint directives when we favor performance.

See https://github.com/ashanbrown/makezero for more details.

@ashanbrown ashanbrown marked this pull request as ready for review November 22, 2020 03:42
Copy link
Member

@bombsimon bombsimon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. golangci-lint uses a bunch of opinionated linters and I'm not sure if the idea was to ever enable all. I see no reason to not add linters that could help with these things without the motive being that everyone should always use them. See exhaustivestruct for example, I don't think this is usable in general for most projects.

I see no reason to not merge this PR so I will approve it. However, if you're interested in feedback, I would stop using the terminology size since Go refers to this as capacity both in the documentation (see here) and in the AST.

The make function takes a type, a length, and an optional capacity.

@ashanbrown ashanbrown marked this pull request as draft November 22, 2020 14:46
@ashanbrown
Copy link
Contributor Author

Just noticed that this panics when running against golangci-lint itself so I'll have to look into that before this is merged.

@ashanbrown ashanbrown marked this pull request as ready for review November 23, 2020 00:43
@sayboras
Copy link
Member

sayboras commented Nov 23, 2020

I think that there might be existing linter/rule to check this one, as Goland is showing similar warning for such cases for me, it could be Goland's thingy only 🤔. Give me sometime to check on this.

It's ok to have some overlapping between linters, but we should try not to confuse end users. Recently, there is one good question about differences between linters (e.g. deadcuode vs unused) in golangci-lint slack channel.

@sayboras
Copy link
Member

I think that there might be existing linter/rule to check this one, as Goland is showing similar warning for such cases for me, it could be Goland's thingy only thinking. Give me sometime to check on this.

My bad, I was confused with another check

@ashanbrown
Copy link
Contributor Author

I made a small change to makezero that fixed parsing of golangci-lint itself, so I think this is in a state to be merged.

@ashanbrown ashanbrown merged commit cf32a7b into golangci:master Dec 5, 2020
@ashanbrown ashanbrown deleted the asb/makezero branch December 5, 2020 15:37
@ldez ldez added the linter: new Support new linter label Dec 6, 2020
@davidnewhall

This comment has been minimized.

@davidnewhall
Copy link

davidnewhall commented Dec 28, 2020

I found the bug introduced in #1569 and left a comment there. @ashanbrown - Please consider fixing that. Thank you!

EDIT: a fix is already in main branch. The bug will hopefully be fixed in whatever version comes after 1.34.0.

@ldez ldez added this to the v1.34 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants