dominikh / go-tools Goto Github PK

Vendor support seems to be partially broken. No imports can be found when $PWD is above the /vendor/ directory when invoking any tools. It's working fine when $PWD is in a sibling or a child of a sibling of /vendor/

Some checks should work differently, or be disabled entirely, depending on the targeted Go version. Examples are #4 and dominikh/go-simple#27.

The flag would default to the Go version used to build the tool.

Currently, all files in testdata are checked by all checkers. This complicates testing things. The code either needs to be more complex than is necessary to test the check in order to avoid "unexpected problems" triggered by other checks, or it needs to explicitly expect the errors reported by other checks.

Instead, we should only run the relevant check, or at least only care about the errors reported by that check.

if foo == a {
} else if foo == b || foo == c {
} else {
}

should be

switch foo {
case a:
case b, c:
default:
}

I'd say this should be suggested as soon as foo == appears at least twice.

Hi,

I would like to add a check for blank identifiers like the example below:

data, _ := json.Marshal(myStruct)

This is based on advice from @ardan-bkennedy

Would this be best placed in the staticchecker or simple, my mind says staticchecker?

This is suggested:

var i int
i = 3

But this isn't:

var i int
i2 := 4
i = 3

Reading your code, this seems to be on purpose. Why?

I can understand that "jumping" other statements makes the check more complex, as it's not certain that the code would still compile correctly. But it should be fine as long as all the statements you have to "jump" don't contain any reference to the declared variable.

Package time documents that:

Do not use == with Time values.

(Source: https://godoc.org/time#Time.Equal.)

This probably extends to != for inequality checks too.

While the tables give a good overview of what checks exist, it doesn't do a great job of explaining the more advanced checks. We should have separate sections for checks worth explaining in detail.

It should be rather straightforward to implement errcheck using our framework. Do it for fun, and also to experiment with some SSA aided features. For example, we could rather easily ignore unchecked errors on (*os.File) if the file is known to be opened read-only.

We may either try to upstream our ideas, assuming adding SSA there is feasible, or maintain our own errcheck check in staticcheck.

Hi,

Love gosimple & staticcheck! <3

Unfortunately this is beeing happening since yesterday, randomly:

package honnef.co/go/simple/cmd/gosimple: unrecognized import path "honnef.co/go/simple/cmd/gosimple" (https fetch: Get https://honnef.co/go/simple/cmd/gosimple?go-get=1: dial tcp: lookup honnef.co on 10.12.0.2:53: dial udp 10.12.0.2:53: i/o timeout)

Using the github namespace does not work either:

$ go get github.com/dominikh/go-tools/cmd/gosimple
can't load package: package github.com/dominikh/go-tools/cmd/gosimple: code in directory /Users/gonzalo/dev/sp/go/src/github.com/dominikh/go-tools/cmd/gosimple expects import "honnef.co/go/tools/cmd/gosimple"

Regards,

Using == on two Values does not compare the underlying values they represent, but rather the contents of the Value structs. To compare two Values, compare the results of the Interface method.

Via golang/go#18871

For interface values, we can track the known set of types. For example, crypto/md5.New returns a hash.Hash, but we know that it will always be a crypto/md5.digest.

I don't know if you've considered this before. This would be insanely useful and probably feasible.

One tool that does this is unconvert:

 $ unconvert -h
[...]
  -apply
        apply edits to source files

One added difficulty that I can see is that you might be adding/removing lines, and thus might introduce new empty lines. I run into the same here when removing statements: mvdan/goreduce#3

Example bad code:

for _, pr := range prs {
	go func(prNumber int) {
		addPRLabel(gh, prNumber, pr.Base.Ref) // oops
		wg.Done()
	}(pr.Number)
}

a la golang/go@99df7c9

var a int
var foo string = "bar"
var another = 123

should be

var (
    a          int
    foo string = "bar"
    another    = 123
)

On top of simpler lines, this introduces alignment. I'd say three contiguous var/const lines should trigger this. Empty lines or comments in between them should be ignored for the count.

Work on the following issues:

• dominikh/go-staticcheck#70 - Checks equivalent to Aspirator
• dominikh/go-staticcheck#68 - Ticker not stopped
• dominikh/go-staticcheck#67 - Detect invocations of os.Exit in func assigned to flag.Usage or flag.FlagSet.Usage.
• dominikh/go-staticcheck#65 - SA5006 triggers when ranging over slice of size 0
• dominikh/go-staticcheck#63 - detect math.Pow() with small positive constants
• dominikh/go-staticcheck#61 - Don't report discarded pure function results in benchmarks
• dominikh/go-staticcheck#60 - SA2002 (T.Fatal called in non-main goroutine) can be fooled by depth
• dominikh/go-staticcheck#57 - vrp: support propagating ranges backwards
• dominikh/go-staticcheck#56 - VRP: support declaring function's return value as a function of one of its inputs
• dominikh/go-staticcheck#54 - Detect useless continue/break statements
• dominikh/go-staticcheck#53 - Avoid flagging the same mistake twice
• dominikh/go-staticcheck#46 - Detect branches with identical code
• dominikh/go-staticcheck#39 - Check sequential tests with identical conditions
• dominikh/go-staticcheck#37 - VRP: implement all arithmetic operators
• dominikh/go-staticcheck#36 - VRP: support slices started
• dominikh/go-staticcheck#35 - VRP: support pointers
• dominikh/go-staticcheck#34 - VRP: support strings started
• dominikh/go-staticcheck#33 - VRP: special cases for the standard library
• dominikh/go-staticcheck#32 - VRP: Get tighter ranges when integer overflow occurs
• dominikh/go-staticcheck#23 - Support more scenarios in SA1001
• dominikh/go-staticcheck#17 - Detect type inconsistencies
• dominikh/go-staticcheck#15 - Check for alignment of variables used in atomic access.

I use Go's vendoring (with Glide) to retrieve version pinned package dependencies, into a vendor subdirectory.
gosimple and staticcheck both require the vendor directory to be correct populated before I can successfully run them.
I have project specific packages in subdirectories which I would like to include when I run gosimple.
However, using gosimple ./... also includes the vendor subdirectory, so I get a lot of messages on 3rd party packages that I'm not going to fix ;-)

Is there an elegant way to run gosimple over ./... yet exclude the vendor directory?
Thanks.

# version of unused
$ (command cd $(go list -f '{{.Dir}}' honnef.co/go/tools/cmd/unused) && git rev-parse HEAD)
1ba7faeb10037a3afd6711b66c299ffb74f08959

My specific example is unused; output from unused always uses the absolute path of a file. To my eye this is unnecessarily noisy. Might a better pattern here be go (build|install) which outputs errors relative to the current working directory.

e.g. currently

$ pwd
/home/myitcv/mygo/src/github.com/myitcv/g/cmd/gai
$ unused
/home/myitcv/mygo/src/github.com/myitcv/g/cmd/gai/gai.go:27:2: const maxGoType is unused (U1000)
/home/myitcv/mygo/src/github.com/myitcv/g/cmd/gai/gai.go:442:6: func resolvePkgSpec is unused (U1000)

vs the much cleaner:

$ unused
gai.go:27:2: const maxGoType is unused (U1000)
gai.go:442:6: func resolvePkgSpec is unused (U1000)

Thoughts?

On a semi-related note (in so far as using the output from go (build|install) as a "pattern" is concerned), would it also make sense to group the lines of output by package import path using "comment headers" in case more than one package is implied/specified via the command line?

$ go list
github.com/myitcv/g/cmd/gai

$ go install  # current directory implied; single package specified
gai.go:6: syntax error: non-declaration statement outside function body

$ go install ./...  # could match multiple; group by package import path
# github.com/myitcv/g/cmd/gai
gai.go:6: syntax error: non-declaration statement outside function body

It doesn't really make sense for gosimple to complain about generated code. The suggestions made by gosimple do not address issues of correctness, and generating code is oftentimes easier when ignoring certain quality standards that apply to hand-written code. Suggestions like for {} instead of for true {} really don't improve generated code.

/cc @tamird because of S1013 ignores in cockroachdb.
/cc @shurcooL for his opinions on generated code

Some cases I've encountered inside const ( ):

Foo = 0
Bar = 1
Etc = 2

could be

Foo = iota
Bar
Etc

And:

Foo Type = 1
Bar Type = 2
Etc Type = 3

could be

_ Type = iota
Foo
Bar
Etc

Could also apply to more complex operations like shifts, but these would already be very useful.

switch a {
case b:
    foo()
    break
case c:
    bar()
}

Breaks are implicit, so I don't see how a break at the end of a switch case block could ever be useful. Similar to a return at the end of a func that has no named return values.

We shouldn't flag useless calls of pure functions when the function is empty. We should consider code like the following as empty, too:

func Relabel(path string, fileLabel string, relabel string) error {
	return nil
}

such functions are stubs and most likely only exist because more useful implementations are guarded by build tags.

Currently we report that x <= 0 for unsigned x is equivalent to x == 0. That suggestion, however, might do more harm than good. Unlike x < 0, it isn't pointing at likely wrong code; actually x <= 0 is more future proof, as it will continue to work as expected when x is changed to be a signed type.

Currently, only regular expressions are supported, which leads to a lot of escaping when matching function call suggestions. Additionally, most rules do not need regular expressions.

We would like to use rdeps for limiting the amount of test one needs to run on pre-push hook. If one modifies a package X, we want to run tests for X and for all packages that depend on X.

Currently, with dependency tree X <= Y <= Z, rdeps X yields Y, and there is no easy way to get both Y and Z. We would like to implement a -indirect (or -recursive?) flag, that will result in rdeps -indirect X returning both Y and Z.

I'll happy to implement this and open a pull request, I just want to hear your opinion on that.

https://play.golang.org/p/fOLYrGF3kl

package main

import (
	"fmt"
	"errors"
)

type ObjCollector struct {
   	Objs []Obj
}

func (c *ObjCollector) collect(obj Obj) {
	c.Objs = append(c.Objs, obj)
}

type Obj struct {
   	Err error
	Str string
	Map map[string]string
}

func (obj *Obj) run(c *ObjCollector) {
	defer c.collect(*obj)
	
	obj.Err = errors.New("Something") // Wrong because Err here still nil and has no pointer
	obj.Str = "Something" // Wrong because of value
	obj.Map["some"] = "Something" // Ok, because map is a pointer
	
	// obj.Err and obj.Str are not used anymore in this function
}

func main() {
	c:=ObjCollector{}
	obj := Obj{
		Map: make(map[string]string),
	}
	obj.run(&c)
	
	fmt.Println(c.Objs)
}

The existance of the _ doesn't change anything, foo will still be set to the zero value if they key doesn't exist. It can always be simplified to foo := someMap[key].

var a int
a = 3

can be reduced to either

a := 3

or

var a int = 3

Depends on the context. The latter is sometimes needed, e.g. for interfaces. In any case, two lines are unnecessary verbosity.

Currently, we only check and build the function bodies of directly checked packages, not of imported dependencies. This makes the tool run faster. In the past, we didn't ever need to look into function bodies of dependencies. Nowadays, however, we compute purity (and soon always-nil errors) of functions, and that information is useful for dependencies, too.

Benchmark how much slower checking all bodies is.

Like a gometalinter of sorts, but just for your packages since they could likely share the initial loading work with little effort.

I remember seeing this done (by cockroachdb?) but can't find it now. In any case, it would be useful as a go-gettable main package.

Add a check that would've told us about d060be8.

https://github.com/cockroachdb/cockroach/blob/master/pkg/roachpb/data.go#L552:L581

TL;DR in some cases, we have no choice but to put an interface value in a sync.Pool, and this lint should not complain about that. Case in point is hash.Hash.

Some functions, such as IsNil, should use type information to be accurate.

type AnonType struct {
    Message string
}

type TopType struct {
    AnonType
}

func foo(t TopType) {
    _ = t.AnonType.Message
}

t.AnonType.Message can be t.Message. I see no reason why the former would be desirable.

From the docs:

Write must not modify the slice data, even temporarily

Suppose we have:

// DoWork does work and calls fn when it's done
DoWork(fn func())

And we use it like:

var wg sync.WaitGroup
wg.Add(1)
fn := func() {
    wg.Done()
}
DoWork(fn)
wg.Wait()

I wrote code like this, only later realising I could simply do DoWork(wg.Done).

I propose that gosimple detect func literals that simply call other functions with the exact same arguments (be it zero, or be it non-zero in the same order).

The only problem here is that this can't be applied if, for example, wg is modified between fn := func()... and DoWork. If it is, making this change could break a program, as we would change the value actually being used.

If your current analysis can detect that sort of thing, I think this check would be really useful. Especially since method func values are not something that people are used to.

The code if <-ch || <-ch was used in a test to check that two calls to some function, that sent a value to ch, didn't return true. We could run into the same issue with function calls.

This is a tough one. It's obviously a false positive, and valid code, but not flagging any such channel receives (or even function calls) would hide many more true positives than it would prevent false positives.

This is a potential check to consider. I imagine it would belong to gosimple, but I'll let you triage.

According to https://golang.org/ref/spec#Type_assertions:

For an expression x of interface type and a type T, the primary expression

x.(T)

asserts that x is not nil and that the value stored in x is of type T. [...] The value of ok is true if the assertion holds.

Therefore, if statements that have this kind of "shape":

if _, ok := x.(T); x != nil && ok {

Can be simplified to:

if _, ok := x.(T); ok {

Here's a real world case example I found that triggered the idea.

https://github.com/google/go-github/blob/2a4b92075e5c5e42e/examples/basicauth/main.go#L40

I imagine this will occur with low-medium frequency, more so in code written by people less experienced with Go.

Unused detected a field in an anonymous struct as unused when not set in the Go program, but it was used in a template.

I had an example, where I was using a value in a header template that was being imported by all templates, the template had a condition if the value was empty it would substitute another value for it. The zero value was useful.

Because the template was a part of the header and used by all templates, all handlers were required to pass a struct that defined the value (and they all did, except one), but unused reported it as being unused.

I've since refactored the code slightly to embed the required pages but thought I'd raise this if this is a condition that should've been handled differently.

package main

import (
        "html/template"
        "log"
        "os"
)

func main() {
        p := struct {
                Name string
        }{}
        t, err := template.New("foo").Parse(`Hello, {{.Name}}!`)
        if err != nil {
                log.Fatal(err)
        }
        err = t.ExecuteTemplate(os.Stdout, "foo", p)
        if err != nil {
                log.Fatal(err)
        }
}

We would like to provide a frontend for the linters that allows for a complete workflow. It should allow ignoring individual false positives, do incremental runs, track metrics across version control revisions, etc.

Random ideas in no particular order:

• Display explanations of checks
• Group results by checks/files

I'm wondering if is it possible to add flags to output absolute path instead of relative.
I use vim with plugin called Neomake. It can execute linters like yours. But it expect absolute path

Particularly:

kisielk/errcheck#115
kisielk/errcheck#116

Versions:

• golang.org/x/tools at 1dce95f7
• honnef.co/go/tools/cmd/unused at e7d68f2
• The following operations were executed on darwin/amd64.

I executed unused on the golang.org/x/tools/container/intsets package:

unused golang.org/x/tools/container/intsets

Which reports:

util.go:8:6: func popcountHD is unused (U1000)

This is a false positive IMHO, because popcountHD is used in the file util.go. Note that this file is guarded by the following build tags:

// +build !amd64 appengine
// +build !gccgo

This leads me to the conclusion that build tags somehow prevent unused from analyzing all the files correctly and therefore lead to false positives.

panic: unexpected Object type: builtin unsafe.Alignof

goroutine 1 [running]:
honnef.co/go/tools/ssa.memberFromObject(0xc4267e7260, 0x1745b00, 0xc4200c12c0, 0x0, 0x0)
	/Users/ryan/go/src/honnef.co/go/tools/ssa/create.go:103 +0x4c2
honnef.co/go/tools/ssa.(*Program).CreatePackage(0xc4265f9d60, 0xc4200c02d0, 0x0, 0x0, 0x0, 0xc420440250, 0x1, 0xc4267e7200)
	/Users/ryan/go/src/honnef.co/go/tools/ssa/create.go:196 +0x7bb
honnef.co/go/tools/ssa/ssautil.CreateProgram(0xc420014a00, 0x40, 0x127d3f6)
	/Users/ryan/go/src/honnef.co/go/tools/ssa/ssautil/load.go:34 +0x31a
honnef.co/go/tools/lint.(*Linter).Lint(0xc4267df830, 0xc420014a00, 0x0)
	/Users/ryan/go/src/honnef.co/go/tools/lint/lint.go:110 +0x4c
honnef.co/go/tools/lint/lintutil.(*runner).lint(0xc4200165a0, 0xc420014a00, 0x0)
	/Users/ryan/go/src/honnef.co/go/tools/lint/lintutil/util.go:199 +0x95
honnef.co/go/tools/lint/lintutil.ProcessFlagSet(0x1528f87, 0xb, 0x173d6a0, 0xc420152b80, 0xc42001e240)
	/Users/ryan/go/src/honnef.co/go/tools/lint/lintutil/util.go:148 +0x761
honnef.co/go/tools/lint/lintutil.ProcessArgs(0x1528f87, 0xb, 0x173d6a0, 0xc420152b80, 0xc4200190d0, 0x1, 0x1)
	/Users/ryan/go/src/honnef.co/go/tools/lint/lintutil/util.go:191 +0xa0
main.main()
	/Users/ryan/go/src/honnef.co/go/tools/cmd/staticcheck/staticcheck.go:21 +0x283

I started getting this panic with every repo I've tried to scan. Scans yesterday succeeded, but today every repo panics. I see the last commit was Feb 27th, 2017, so I'm baffled. gosimple and unused are also panicking.

Given how expensive it it vs. calling regexp.Compile() first and then re.Match(), this seems like an obvious mistake to flag.

For example, https://play.golang.org/p/49sgROkOS1 (takes too long for playground).

We should provide a flag to output results in JSON. Such output would allow other tools to more easily consume our output.

CheckIneffectiveAppend (SA4010) doesn't seem all that useful when we also have CheckUnreadVariableValues (SA4006). The latter doesn't report results of append that are used in other appends only, but that doesn't seem that useful to begin with.

All actually unused results of append are already reported by SA4006, and if the user fixed those instances, a repeated run would also catch the ones that were missed before.

func createHandler() func(http.ResponseWriter, *http.Request) {
}

should be

func createHandler() http.HandlerFunc {
}

I'd do this for all named and exported interfaces in the std. If it's at all useful, I gather them in a go generate for interfacer here: https://github.com/mvdan/interfacer/blob/master/std.go