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

cmd/gofmt: simplify nil checks followed by len check, e.g. x != nil && len(x) ... #14733

Closed
client9 opened this issue Mar 9, 2016 · 5 comments
Closed

Comments

@client9
Copy link

@client9 client9 commented Mar 9, 2016

This a followup from golang-nuts that I posted a few months back.

It would be nice if gofmt -s simplified the following redundant expressions:

if x == nil || len(x) == 0 {}
if x != nil && len(x) ... {  // or any operator len(x) > 0, len(x) != 0, len(x) > 10000

can be simplified into just

if len(x) == 0 {}
if len(x) .... {}

How common is this?

Not uncommon: While this code search on GitHub isn't perfect, it does show up. I'd prefer not to download github to correctly search until I understand the acceptance requirements (defects per 10k? at what level? number of repos affected?)

To quote the original topic:

As a quick test I used grep, and removed false positives.

$ find . -name '*.go' | xargs grep '= nil || len(' | grep Godeps

./Godeps/_workspace/src/github.com/gocql/gocql/marshal.go: if data == nil || len(data) == 0 {
./Godeps/_workspace/src/github.com/pborman/uuid/uuid.go: if uuid == nil || len(uuid) != 16 {
./Godeps/_workspace/src/github.com/pborman/uuid/uuid.go: if uuid == nil || len(uuid) != 16 {
./Godeps/_workspace/src/github.com/ugorji/go/codec/decode.go: if rvbs == nil && bs2 != nil || rvbs != nil && bs2 == nil || len(bs2) != len(rvbs) { ### wow!!

$ find . -name '*.go' | xargs grep '= nil && len(' | grep Godeps

./Godeps/_workspace/src/github.com/araddon/gou/jsonhelper.go:if lst := jsonList(root); lst != nil && len(lst) > idx {
./Godeps/_workspace/src/github.com//elastigo/lib/clusterhealth.go: if f.FilterIndices != nil && len(f.FilterIndices) > 0 {

thats 5 repos out of 60. Granted a small sample but that doesn't seem so rare.

I reran this, a few of them have been fixed up and some new ones added.

Great for learning

Even if not particularly common, people new to Go from ruby, C/C++, java are normally terrified by nil or null and are use to putting in guards. Having this check in gofmt, make learning that the len operator understands the native type and makes code simpler. Plus it definitely makes the code easier to read.

gofmt -st vs. golint

  • The check-nil-then-len anti-pattern is fundamental misunderstanding of the language (that len understands pointers and types) vs a style issues. (more like gofmt, less like golint)
  • It can be coded with no false positives (again more like gofmt, less like golint)
  • that said I think the -s flag in gofmt is less known than golint, and golint is decoupled from release cycles.
  • There may be an issue of false negatives with gofmt. For instance, what should happen with inverted forms such as x == nil || 0 == len(x), or broken code such as if len(x) == 0 || x = nil (which I bet happens). gofmt -s simplification is very simple, while golint is more like "hey take a look at this.."

Next Steps

If this is better ingolint Great, let me know and I'll move the ticket over there.

Likewise, if this needs a formal proposal I'm happy to that as well.

And finally, I'm also happy to fill out the copyright assignment and do the actual work. You win!

ps. While writing this, "golint" got autocorrected as "goofy"

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 9, 2016

Unfortunately, w/o type information, this simplification cannot be done in general; see: http://play.golang.org/p/PMfEn4oT_i
Here, the x in question is a nil pointer to an array, but len(x) always returns the length of the array (statically, evaluated at compile-time: https://golang.org/ref/spec#Length_and_capacity ). So, unless the array is declared with 0 elements (unlikely), the simplification is wrong.

That is, this not an option for gofmt -s, but perhaps golint (if enough type info is available).

@ALTree
Copy link
Member

@ALTree ALTree commented Mar 9, 2016

I disagree with both the arguments you make for putting this into gofmt.

The check-nil-then-len anti-pattern is fundamental misunderstanding of the language

and gofmt is a code formatter. I think almost no one thinks about gofmt as "a tool that fixes patterns that betray a fundamental misunderstanding of the language", gofmt is "that thing that formats go code in an uniform way". Even if you look at the simplifications made by gofmt -s, you only see really really lightweight transformations, mostly syntactic-cosmetic changes. Removing a clause from a boolean expression is a really different thing, IMHO.

It can be coded with no false positives (again more like gofmt, less like golint)

The fact that golint can generate false positives does not imply that something that does not generate false positives is ill-fitted for golint.

@client9
Copy link
Author

@client9 client9 commented Mar 9, 2016

interesting @griesemer I didn't take account the behavior of pointers and len. Should I move this the golint issues?

@griesemer
Copy link
Contributor

@griesemer griesemer commented Mar 9, 2016

@client9 Yes, please: https://github.com/golang/lint/issues . I'm going to close this.

@griesemer griesemer closed this Mar 9, 2016
@client9
Copy link
Author

@client9 client9 commented Mar 9, 2016

@ALTree I'm convinced. golint it is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.