gordonklaus / ineffassign Goto Github PK

View Code? Open in Web Editor NEW

391.0 391.0 22.0 55 KB

Detect ineffectual assignments in Go code.

License: MIT License

Go 100.00%

ineffassign's People

Contributors

Stargazers

Watchers

ineffassign's Issues

False positive or explain: could not import C (no metadata for C)

Hi, and first thank you for this program.

I have run a Go Report on one of my project, and your program is outputting the following warning:

btrfs-diff-go/pkg/btrfs-diff.go
        Line 16: warning: could not import C (no metadata for C) (ineffassign)

It is related to the following line :

package btrfsdiff

// We get the constants from this header.

// #include <btrfs/send.h>
// #include <btrfs/send-utils.h>
// #include <btrfs/ioctl.h>
// #cgo LDFLAGS: -lbtrfs
import "C"

And then the C import is used like in the following lines :

var commandsDefs *[C.__BTRFS_SEND_C_MAX]commandType = initCommandsDefinitions()

Can you explain to me why the warning, or try to fix it, if it is a false positive.

Thank you in advance.

If I can be of any help, let me know.

False positive

While scanning the Go source, this tool reports the following line as an ineffectual assignment to p:
https://github.com/golang/go/blob/35bb62e60a7779ff82c3067903b3306ff8666471/src/cmd/internal/obj/wasm/wasmobj.go#L221

Obviously, this is not the case and removing that line causes incorrect behaviour (and might even be dangerous).

Full warning:

cmd/internal/obj/wasm/wasmobj.go:225:3:warning: ineffectual assignment to p (ineffassign)

inconsistent behavior with bool initialization

This tool has allowed me to slay many a bug today. Thank you!!!

I also noticed this inconsistent behavior.

package main

import "fmt"

func main() {

	s := ""
	s = "hello"
	fmt.Println("s:", s)

	n := 0
	n = 42
	fmt.Println("n:", n)

	b := false
	b = true
	fmt.Println("b:", b)
}

I can init and update s and n as expected, but I'm getting ineffectual assignment to b.

I am running go1.12.5.

Thanks!

Uncaught ineffectual assignment when using closure

Hi there!

First of all, thanks for creating this awesome linter! It has caught many bugs in our code.

A short while ago, I ran into an issue where an ineffectual assignment was only caught during a code review because the linter did not catch it. So I was wondering whether this is a known false-negative case and whether it can be fixed.

The case has to do with the use of closures. I have recreated three scenarios as tests for testdata.go.

Cases where the ineffectual assignment is not detected:

func _() {
	x := 0
	func() {
		x = 0
	}()
	x = 0 //x
	x = 0
}

func _() {
	x := 0
	_ = func() {
		x = 0
	}
	x = 0 //x
	x = 0
}

func _() {
	x := 0
	x = 0 //x
	x = 0
	_ = func() {
		x = 0

		_ = x
	}
	_ = x
}

Case where it is correctly detected:

func _() {
	x := 0
	_ = func() {
		x := 0

		_ = x
	}
	x = 0 //x
	x = 0

	_ = x
}

As you can see, if a closure uses a variable, the linter then does not detect any ineffectual assignments for that variable. To be clear, it is not about whether the assignment was ineffectual because of what happens within the closure (I can imagine that that is tricky if not impossible to detect). In all of the three missed cases, the ineffectual assignment is directly followed by another assignment.

I'd love to hear if there is anything that can be done about this.

Minor: Support go standard use of ./...

Most go tools support the use of ./... to mean scan from the current directory down as far as you can go. However, ineffassign responds with this: "app/dir/...: no such file or directory"

So instead I had to use ./* which seems to work fine by the way. However, for consistency with other go tools can you add support for ./... too?

What is causing this ineffassign?

Hi there,

I can see in my goreportcard result that I have an ineffassign: https://goreportcard.com/report/github.com/TomWright/dasel

It's highlighting line 281 (line 2 in this snippet):

cmd.AddCommand(
	putStringCommand(),
	putBoolCommand(),
	putIntCommand(),
	putObjectCommand(),
	putDocumentCommand(),
)

From the README:

An assignment is ineffectual if the variable assigned is not thereafter used.

I don't understand why this line has been highlighted since:

No variable is assigned on that line
The return value is passed straight into a func where it is immediately used

Have I missed something or would this be considered a bug?

Minor: ./* doesn't work on Windows

The usage example in the README says you can use ineffassign ./* to analyze the current directory recursively. However, this does not seem to work on Windows, and using ineffassign ./ instead should always work to recursively analyze the current working directory.

False positive #2

This finds a false positive on line 25:

// +build ignore

package main

import (
	"math"
	"math/rand"
)

func main() {
	const tiny = 1e-30

	t := 0

	f := t
	if f == 0 {
		f = tiny
	}
	C := f
	D := 0.0
	delta := 0.0

	for i := 0; i < math.MaxInt64; i++ {
		t = rand.Intn()

		D = t.B + t.A*D
		if D == 0 {
			D = tiny
		}
		C = t.B + t.A/C
		if C == 0 {
			C = tiny
		}
		D = 1 / D
		delta = C * D
		f = f * delta
		if math.Abs(delta-1) <= eps {
			break
		}
	}
	return f
}

Possible false positive

package main

type A struct {
	S string
}

func main() {
	aList := []A{
		{S: "Foo"},
		{S: "Bar"},
	}

	var result *A
	for _, i := range aList {
		if i.S == "foobar" {
			result = &i
			break
		}
	}

	if result == nil {
		vsf := A{S: "baz"}
		result = &vsf
	}
}

Reported error:

main.go:23:3: ineffectual assignment to `result` (ineffassign)

Why is this not reported ?

Let's consider the code below:

package main

import "fmt"

type inner struct {
    s string
}

type outer struct {
    inner []inner
}

func main() {
    o := outer{
        inner: []inner{{
            s: "a",
        }},
    }
    for _, i := range o.inner {
        i.s = "b"
    }
    fmt.Println(o)
}

Shouldn't this code be reported ?
i.s = "b" will have no effect here.

False Positive when using Defer

Example Code:

package main

import "fmt"

func main() {
	ok := true
	defer fmt.Println(ok)
	err := someOperation()
	if err != nil {
		ok = false
		return
	}
}

func someOperation() error {
	return fmt.Errorf("Oh no!")
}

Running ineffassign returns ineffectual assignment to ok

Why this situation report ineffassign?

I have codes like this

	format = skipWhiteSpace(format)
	for token, formatRemain, succ := getFormatToken(format); len(token) != 0; format = formatRemain {
		if !succ {
			isDuration, isDate = false, false
			break
		}
		if _, ok := durationTokens[token]; ok {
			isDuration = true
		} else if _, ok := dateTokens[token]; ok {
			isDate = true
		}
		if isDuration && isDate {
			break
		}
		token, formatRemain, succ = getFormatToken(format)
	}
	return
}

And ineffassign tell me No.2 line warning: ineffectual assignment to formatRemain.
But I think this variable I have already used.

I was wondering if you could tell me what I have done wrong?

False positive: string concatenation

Because ineffassign doesn't consider types in its checks, it makes the mistake of assuming that the + operator is associative. This is not the case for all types - most notably strings.

Here's an example of a small program that does an assignment that is not ineffectual, but does get flagged as such by ineffassign:

package main

import (
	"fmt"
)

const prefix = `prefix `

func main() {
	prefixMe := `a string to prefix`
	// the below line is flagged
	prefixMe = prefix + prefixMe
	fmt.Println(prefixMe)
}

(playground)

Uncaught

package foo

func foo() {
    x := 1
    x = 2
    return &x
}

The initial assignment is ineffective, but isn't caught here, due to the fact that x escapes.

false positive: assigning to return variable with panic

example program:

package main

import (
	"log"
)

func main() {
	out := func() (ok bool) {
		ok = true
		defer func() {
			if r := recover(); r != nil {
				log.Println("panic happened!")
			}
		}()

		panic("oh crud")
		return false
	}()

	log.Println(out)
}

ineffassign prints: ineffectual assignment to ok

but if you remove that assignment (line 9), the program will print false instead of true

Panic

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x10c3a5d]

goroutine 1 [running]:
main.(*branchStack).get(0xc420341258, 0x0, 0xc42034c2a0)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:512 +0x6d
main.(*builder).Visit(0xc420341200, 0x111a2a0, 0xc4203ea560, 0x1119ce0, 0xc420341200)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:226 +0x15c3
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a2a0, 0xc4203ea560)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:52 +0x66
go/ast.walkStmtList(0x1119ce0, 0xc420341200, 0xc4203ea580, 0x2, 0x2)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:32 +0x81
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a260, 0xc4203e65a0)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:224 +0x1b61
main.(*builder).walk(0xc420341200, 0x111a260, 0xc4203e65a0)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:122 +0x5a
main.(*builder).Visit(0xc420341200, 0x111a860, 0xc4203c1d80, 0x0, 0x0)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:139 +0x236f
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a860, 0xc4203c1d80)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:52 +0x66
go/ast.walkStmtList(0x1119ce0, 0xc420341200, 0xc42033b300, 0x9, 0x10)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:32 +0x81
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a260, 0xc4203e65d0)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:224 +0x1b61
main.(*builder).walk(0xc420341200, 0x111a260, 0xc4203e65d0)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:122 +0x5a
main.(*builder).fun(0xc420341200, 0xc4203ea820, 0xc4203e65d0)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:379 +0x1aa
main.(*builder).Visit(0xc420341200, 0x111a6e0, 0xc4203e6600, 0x0, 0x0)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:130 +0xc84
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a6e0, 0xc4203e6600)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:52 +0x66
go/ast.walkDeclList(0x1119ce0, 0xc420341200, 0xc4203b6580, 0x7, 0x8)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:38 +0x81
go/ast.Walk(0x1119ce0, 0xc420341200, 0x111a660, 0xc4203b6780)
	/usr/local/Cellar/go/1.10.2/libexec/src/go/ast/walk.go:353 +0x2650
main.(*builder).walk(0xc420341200, 0x111a660, 0xc4203b6780)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:122 +0x5a
main.checkPath(0xc42038eb60, 0x67, 0x3, 0xc42034db01, 0x1063946, 0xc42038ebd0, 0xc42038eb60, 0x67, 0xc42034db50)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:79 +0xfa
main.walkPath.func1(0xc42038eb60, 0x67, 0x111bd60, 0xc42039fee0, 0x0, 0x0, 0xc, 0xc42034dc48)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:61 +0x165
path/filepath.walk(0xc42038eb60, 0x67, 0x111bd60, 0xc42039fee0, 0xc4200a8060, 0x0, 0x0)
	/usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:357 +0x402
path/filepath.walk(0xc42030df80, 0x5a, 0x111bd60, 0xc42039fe10, 0xc4200a8060, 0x0, 0x0)
	/usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:381 +0x2c2
path/filepath.walk(0xc42008e120, 0x50, 0x111bd60, 0xc420096b60, 0xc4200a8060, 0x0, 0x20)
	/usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:381 +0x2c2
path/filepath.Walk(0xc42008e120, 0x50, 0xc4200a8060, 0x50, 0x0)
	/usr/local/Cellar/go/1.10.2/libexec/src/path/filepath/path.go:403 +0x106
main.walkPath(0xc42008e120, 0x50, 0xc42008e120)
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:45 +0x9a
main.main()
	/Users/aat/.go/src/github.com/alecthomas/gometalinter/_linters/src/github.com/gordonklaus/ineffassign/ineffassign.go:34 +0x7a

False positive

Example from my code:

    for _, server := range servers {
        f := false
        if server.State == stateUnconn || server.State == stateFailed {
            if f == false {
                f = true
                vy++
            }

Error message:
f assigned and not used (ineffassign)

f has already been assigned in the outer loop so error should not happen when f is set to true.

false report for assigining to the receiver pointer

there is a false report in where it assigns to a receiver

https://github.com/kubernetes/test-infra/blob/master/def/configmap/main.go#L64

func (mkv *multiKeyValue) Set(v string) error {
	p := strings.SplitN(v, "=", 2)
	if len(p) != 2 {
		return fmt.Errorf("%s does not match label=value", v)
	}
	if mkv == nil {
		mkv = &multiKeyValue{
			p[0]: p[1],
		}
	} else {
		(*mkv)[p[0]] = p[1]
	}
	return nil
}

ineffassign incorrectly claims that an assignment is not used

func (f *Foo) Bar() (bzs []Baz, err error) {
    if bs, err := ioutil.ReadFile(os.Getenv(bazesPathKey)); err == nil {
        // ineffassign claims that this is not used, but it's a return value
        err = json.Unmarshal(bs, &bzs)
    }
    return
}

False positive

https://goreportcard.com/report/github.com/tinode/chat

Line 1021: warning: err assigned and not used (ineffassign)

This is the line it complains about:
https://github.com/tinode/chat/blob/master/server/db/rethinkdb/adapter.go#L1021

Thanks.

Option (or default) to not recursively check all directories

This is very slow with vendored source trees, and can be easily replicated with find . -type d | xargs ineffassign. It also makes integrating the tool into an editor likely to be much slower.

Most Go tools allow both: <dir> will check just that dir and <dir>/... will recursively check all dirs underneath. This would be ideal.

[Feature request] Add version and expose through flag

Hi, it would be useful for debugging if ineffassign was versioned and one could get that version through --version flag. Right now it's hard to know what version of ineffassign is installed. Ideally one would get the commit of the build but I don't know if that can be done easily and keep the installation through go get.

False positive #2

While scanning the Go source, this tool reports the following lines as an ineffectual assignment to v:

This is not the case as the code is testing runtime finalizers.

Full warnings:

runtime/mfinal_test.go:60:4:warning: ineffectual assignment to v (ineffassign)
runtime/mfinal_test.go:98:3:warning: ineffectual assignment to v (ineffassign)

and

runtime/stack_test.go:126:4:warning: ineffectual assignment to s (ineffassign)

False positive in the-eye-team/reddit-dl

I noticed the usage here https://goreportcard.com/report/github.com/The-Eye-Team/reddit-dl#ineffassign. In it, I have code very similar to the following..

l := true

if complicatedCondition() {
	l = false
}
if anotherComplicatedCondition() {
	l = false
}
if l {
	// do some code that handles the default case
}

and ineffassign is labeling the l = false lines as ineffectual assigns. I believe this to be a bug. Conversation welcome :)

Getting warning on err not being used by err is being returned from the function

Hi, im getting a warning of unused variable: Line 331: warning: err assigned and not used (ineffassign)

Bur error is actually being used, is the return of the function.

here is the code:

			resp := friendResp{}
			err = json.Unmarshal(body, &resp)
			user.Friendship = resp.Friendship
		}
	}
	return err
}

Im using goreportcard.com to run the test. thanks.

Ineffassign and declaring variables using the `:=` operator

I was wondering: ineffassign will flag variable declarations using the := declarations like this:

dir := ""

As an ineffectual assignment.
This could be changed to:

var dir string

To make the error go away, but isn't just a case of style preference ?

Case in hand, David Crawshaw (from the Go team) disagrees with this behaviour, see this comment.

Would you consider white-listing this pattern ?

Incorrect ineffectual assignment in switch

I found a false ineffectual assignment in a case similar to this:

package main

import (
        "fmt"
)

func main() {
        var a int
        switch n := -1; {
        case n == -1:
                n += 1
                fallthrough
        case n == 0:
                a = 10
        }
        fmt.Println(a)
}

It says that n += 1 is ineffectual when it is very much effectual. Without it a is 0, with it it's 10.
Same example on go playground here.

(Also, thanks for good linting in general, very useful for catching bugs. :) )

false positive found

Line https://github.com/ericlagergren/decimal/blob/522450d1e655d86ef13b0bfad42d090a0240ce03/decimal.go#L430 is marked as an ineffectual assign.

Uncaught ineffective assignment

package main

import "fmt"

func main() {
    var i int
    i = 3
    i = 5
    fmt.Println(i)
}

$ ineffassign main.go does not produce any results. It should catch that the i = 3 assignment was ineffective (its value was never used).

I created the issue because I discovered an unchecked error in my code because of this pattern, i.e.:

err := someCall()
err = anotherCall()
if err != nil { ... }

False positive with slice pointing to an array

This program triggers a false positive reported by ineffassign: main.go:9:3: ineffectual assignment to iv.
iv and ivSlice point to the same data so this is not a ineffective assignment.

package main

import "fmt"

func main() {
	var iv [16]byte
	ivSlice := iv[:]
	for i := range ivSlice {
		iv = [16]byte{} // Reset
		ivSlice[i] = 'A'
		fmt.Printf("%x\n", ivSlice)
	}
}

it would be nice if ineffassign detected cases within a for loop.

noticed this the other day, and was surprised the linter didn't pick it up:

type X struct {
    A int
}

xs := []X{{A:1},{A:2},{A:3}}

for _, x := range xs {
  // would expect to see a warning here.
  x.A = 5
}
log.Println(xs)

nil-assigned value does not always need to be used

package main

import "fmt"

func main() {
    var kvs [][]string
    fmt.Println(kvs)
    kvs = nil
}

ineffassign ineffassign.go
/home/gyuho/ineffassign.go:8:2: kvs assigned and not used

We assign nil to kvs, which is already used in previous line. And the tool assumes it's a new assignment. Could you fix this?

Thanks.

tc