gordonklaus / ineffassign

Detect ineffectual assignments in Go code.
MIT License
394 stars 22 forks source link

Ineffassign and declaring variables using the `:=` operator #21

Closed jawher closed 6 years ago

jawher commented 6 years ago

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 ?

crawshaw commented 6 years ago

I went to file this issue and found @jawher beat me to it. :-)

@dgryski mentions that besides this, ineffassign catches real bugs. You have considered proposing rolling it into go vet?

gordonklaus commented 6 years ago

Up until now, I have actually considered this a feature rather than a bug. But it's more important not to flag valid code.

I considered it a feature because, although assignment of a zero value and declaration are functionally equivalent, they can communicate different intents: Assignment says "this is a value that will be used"; whereas declaration says "this is a variable that has yet to be assigned to". This is the way I've been writing code since before I wrote ineffassign; it's not mere style.

But it's not the place of this tool to enforce this convention. I'll try to white-list this pattern.

@crawshaw Rob Pike thought it should be part of the compiler, but of course that's a no-go for compatibility reasons. I tried mashing it into go vet a while ago but it didn't fit the pattern of the other checks (it wants full control of AST walking, I recall was the problem). I think we can look forward to Go 2 doing this in the compiler.

crawshaw commented 6 years ago

Russ Cox mentioned recently that vet is a bit constraining in how checks need to be written. I think a good argument could be made for reworking vet to allow your checks. It would make for a good Go issue you could describe what exactly is needed. :-)

I bring it up because as of Friday, go vet is now run automatically when you run go test, so I expect it to be far more widely used in Go 1.10. Any 100% correct check that can go in it would be wonderful. (While Go 2 is years away.)

gordonklaus commented 6 years ago

@crawshaw Thanks for the recommendation, I'll write up an issue when I find the time.

Ineffassign as it currently stands isn't a 100% check. It still flags a lot of innocuous (if sloppy) code. But it does flag enough real bugs that it probably ought to be in go vet.