build: make check improvements

[jgautheron]

How about adding the following automated checks in the make check:

• deadcode (dead project)
• gocyclo (Implement -ignore fzipp/gocyclo#11)
• defercheck (https://github.com/opennota/check/commit/aa37591a11986c7dd6ce70640271a752b37b0ea2)
• structcheck (https://github.com/opennota/check/issues/11)
• varcheck
• errcheck
• dupl
• goconst (produces little value: work on more than strings jgautheron/goconst#1)
• safesql (cli code is unsafe, but safesql doesn't catch it)
• unused
• usedexports (lots of false positives) superceded by unused
• interfacer (produces little value)

[tamird]

Sure. We are already using varcheck and errcheck. I've tried adding the others at various times but have found them to be buggy. Pull requests are, of course, welcome.

[jgautheron]

Alright, I'll see what I can do.

[tamird]

Update on this:

• we're already using varcheck and errcheck
• deadcode doesn't do anything and isn't maintained (running it on cockroach results in no output afaict)
• gocyclo isn't useful until ignore generated code fzipp/gocyclo#9 is solved
• structcheck isn't useful until https://github.com/opennota/check/issues/11 is solved
• defercheck seems not useful in general (at least, we have no violations now, and any future ones are trivial to notice visually)
• dupl produces a huge number of false positives. It is added to the Makefile in Add dupl Makefile target #3131, but not enforced

I'm going to close this issue, since there's no action to be taken here until some upstream changes are made. If you'd like to follow up on #3131 and try to reduce some duplication, that would of course be quite welcome.

[tamird]

Quick addendum, defercheck now produces false positives against us. See https://github.com/opennota/check/issues/22 and https://github.com/opennota/check/issues/23

[jgautheron]

@tamird this one might prove useful as well: https://github.com/mvdan/interfacer

[tbg]

I ran this against our code a couple of days ago, but it didn't turn up
with anything interesting (4 items, all correct but still nothing that made
me want to change code - mostly turning things to fmt.Stringer).

On Wed, Dec 23, 2015 at 10:54 AM Jonathan Gautheron <
notifications@github.com> wrote:

@tamird https://github.com/tamird this one might prove useful as well:
https://github.com/mvdan/interfacer

—
Reply to this email directly or view it on GitHub
#2485 (comment)
.

[jgautheron]

I wrote yesterday a tool that could of some help to you, it's called goconst.
It spots repeated strings that could be replaced by a constant. This is mostly to spot beginner mistakes, but it can be interesting for large codebases.

[tamird]

Sure, added. Reopening.

[jgautheron]

As expected it doesn't return much (-min-occurrences 3).

3 occurrences of "NULL" found:
    cockroachdb/cockroach/cli/sql_util.go:159:10
    cockroachdb/cockroach/sql/parser/datum.go:588:9
    cockroachdb/cockroach/sql/parser/datum.go:615:9
8 occurrences of "tcp" found:
    cockroachdb/cockroach/gossip/resolver/node_lookup.go:115:16
    cockroachdb/cockroach/gossip/resolver/resolver.go:58:9
    cockroachdb/cockroach/gossip/resolver/resolver.go:94:7
    cockroachdb/cockroach/gossip/resolver/socket.go:50:7
    cockroachdb/cockroach/gossip/resolver/socket.go:55:16
    cockroachdb/cockroach/util/unresolved_addr.go:46:7
    cockroachdb/cockroach/util/net.go:112:7
    cockroachdb/cockroach/util/testing.go:96:7
9 occurrences of ", " found:
    cockroachdb/cockroach/gossip/infostore.go:92:13
    cockroachdb/cockroach/sql/parser/update.go:50:12
    cockroachdb/cockroach/sql/parser/select.go:87:12
    cockroachdb/cockroach/sql/parser/select.go:126:12
    cockroachdb/cockroach/sql/parser/select.go:263:12
    cockroachdb/cockroach/sql/parser/select.go:276:12
    cockroachdb/cockroach/sql/parser/create.go:124:12
    cockroachdb/cockroach/sql/parser/expr.go:530:12
    cockroachdb/cockroach/sql/parser/alter_table.go:49:12
6 occurrences of "unix" found:
    cockroachdb/cockroach/gossip/resolver/resolver.go:79:12
    cockroachdb/cockroach/gossip/resolver/resolver.go:94:14
    cockroachdb/cockroach/gossip/resolver/socket.go:43:7
    cockroachdb/cockroach/util/unresolved_addr.go:50:7
    cockroachdb/cockroach/util/net.go:134:7
    cockroachdb/cockroach/util/testing.go:102:7
3 occurrences of "DEFAULT" found:
    cockroachdb/cockroach/sql/set.go:93:18
    cockroachdb/cockroach/sql/parser/expr.go:254:9
    cockroachdb/cockroach/sql/parser/set.go:61:33

[jgautheron]

Another tool I wrote, usedexports doesn't return much as well.

cockroachdb/cockroach/gossip/simulation/network.go:170:1 func "RunUntilFullyConnected" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:71:2 const "OpVarLast" is exported but not used locally
cockroachdb/cockroach/storage/simulation/store.go:35:6 type "Store" is exported but not used locally
cockroachdb/cockroach/storage/simulation/node.go:28:6 type "Node" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:38:6 type "Node" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:121:1 func "GetNodeFromID" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:69:2 const "OpVarRandom" is exported but not used locally
cockroachdb/cockroach/storage/simulation/cluster.go:429:1 func "OutputEpochHeader" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:198:1 func "InfosSent" is exported but not used locally
cockroachdb/cockroach/storage/simulation/range.go:146:1 func "String" is exported but not used locally
cockroachdb/cockroach/storage/simulation/cluster.go:439:1 func "OutputEpoch" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:208:1 func "InfosReceived" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:39:2 const "OpAddNode" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:40:2 const "OpExit" is exported but not used locally
cockroachdb/cockroach/storage/simulation/cluster.go:39:6 type "Cluster" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:107:6 type "ActionDetails" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:110:1 func "GetNodeFromAddr" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:160:1 func "Stop" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:38:2 const "OpSplitRange" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:140:1 func "SimulateNetwork" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:34:6 type "Operation" is exported but not used locally
cockroachdb/cockroach/storage/simulation/range.go:40:6 type "Range" is exported but not used locally
cockroachdb/cockroach/gossip/simulation/network.go:185:1 func "IsNetworkConnected" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:64:6 type "OperationVariant" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:92:6 type "Action" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:68:2 const "OpVarValue" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:70:2 const "OpVarFirst" is exported but not used locally
cockroachdb/cockroach/storage/simulation/script.go:151:6 type "Script" is exported but not used locally

I'm guessing that the exported methods are part of a public API?

[petermattis]

I'm not finding these suggestions very compelling. And it looks like you've got an out of date version of the cockroach source code.

[jgautheron]

Agreed.
Updated my results above with the latest version.

[bdarnell]

Another one to consider: safesql. This enforces that all arguments to sql.DB.Query and similar functions are string literals (instead of e.g. the result of Sprintf) to guard against SQL injection (and enforce correct use of placeholders). We couldn't enforce this right now because of limitations in our placeholder parsing, but once we fix these issues we may want to use this checker to ensure that we're doing things the right way.

[tamird]

I tried safesql:

safesql .
Found 1 potentially unsafe SQL statements:
- /Users/tamird/src/go/src/github.com/cockroachdb/cockroach/cli/sql_util.go:79:23
Please ensure that all SQL queries you use are compile-time constants.
You should always use parameterized queries or prepared statements
instead of building queries from strings.

So long as we have a SQL shell, we won't be able to fix that.

[bdarnell]

Yeah, we'll need some way to mark exceptions (safesql is also currently incompatible with common bulk-insert patterns), but I still think it would be worth doing at some point.

[tamird]

Looks like some of our linters are misbehaving: https://goreportcard.com/report/github.com/cockroachdb/cockroach

[mvdan]

I ran this against our code a couple of days ago, but it didn't turn up with anything interesting (4 items, all correct but still nothing that made me want to change code - mostly turning things to fmt.Stringer).

@tschottdorf I've been thinking of ways to avoid such "false positives" in interfacer, although as you say they are still correct. It's more like they are technically correct, but in practice they make little sense.

I haven't figured out a clean way to get rid of them, though. I could blacklist fmt.Stringer, but that doesn't really solve the problem. I can't wrap my head around what makes cases like these special.

I've had a quick look at the warnings on your code, and filed this issue which should remove quite a few: mvdan/interfacer#17

If you have more ideas, let me know :)

Edit: Another - mvdan/interfacer#18

[nvanbenschoten]

I just stumbled upon https://github.com/dominikh/go-unused, which checks for unused identifiers (constants, variables, functions, and types). Might be worth adding it to the verification suite, although I'm not sure a no-tolerance policy for unused identifiers is a very good idea.

[tamird]

pretty sure @petermattis beat you to it
#5256

On Tue, Mar 15, 2016 at 11:31 PM, Nathan VanBenschoten <
notifications@github.com> wrote:

I just stumbled upon https://github.com/dominikh/go-unused, which checks
for unused identifiers (constants, variables, functions, and types). Might
be worth adding it to the verification suite, although I'm not sure a
no-tolerance policy for unused identifiers is a very good idea.

—
You are receiving this because you modified the open/close state.
Reply to this email directly or view it on GitHub
#2485 (comment)

[jordanlewis]

Seems like this is ready for closing. Let's create new issues for individual checks we want to add going forward.