mvdan / unparam Goto Github PK

Find unused parameters in Go

License: BSD 3-Clause "New" or "Revised" License

Go 100.00%

go linter unused parameters arguments functions results

unparam's Introduction

unparam

go install mvdan.cc/unparam@latest

Reports unused function parameters and results in your code.

To minimise false positives, it ignores certain cases such as:

Exported functions (by default, see -exported)
Unnamed and underscore parameters (like _ and _foo)
Funcs that may satisfy an interface
Funcs that may satisfy a function signature
Funcs that are stubs (empty, only error, immediately return, etc)
Funcs that have multiple implementations via build tags

It also reports results that always return the same value, parameters that always receive the same value, and results that are never used. In the last two cases, a minimum number of calls is required to ensure that the warnings are useful.

False positives can still occur by design. The aim of the tool is to be as precise as possible - if you find any mistakes, file a bug.

unparam's People

Stargazers

Watchers

unparam's Issues

Compatibility issue with go <1.8

I would like to have unparam merged into gometalinter and it will not be integrated if it is not backward compatible reference to goetmalinter issue: alecthomas/gometalinter#256
The issue is that sort.Slice is only available in go >= 1.8, not as int gometalinter issue described sort.Sort.

Would be nice to have unparam merged there.
Great work, thanks!

false positive with func called via reflect.ValueOf(x).Call

Hi,

I have the following function:

	g.EXPECT().Begin(gomock.Any()).Times(errorTask).DoAndReturn(
		func(_ context.Context) (db.Tx, error) {
			...
			return tx, nil
		},
	)

On my mac unparam does not report any problems but on travis ci it reports parameter (*suite).TestSuccess$1 - result 1 (error) is always nil. I use retool to pin the latest version of this writing (2b8063f). Any idea what might be causing this discrepancy? The tool should not lint this function in the first place because it is unnamed right?

Support Go 1.17 slice-to-array conversion

$ go version
  go version go1.17beta1 linux/amd64

$ more ./go/tmp/main.go 
  package main

  import (
  	"fmt"
  )

  func main() {
  	s := []int{1, 2, 3, 4}
  	fmt.Println(*(*[4]int)(s))
  }

$ go run ./go/tmp/main.go 
  [1 2 3 4]

$ unparam ./go/tmp/main.go 
  panic: in command-line-arguments.main: cannot convert *t0 ([]int) to *[4]int
  [...]

This is using latest unparam from master. I'm ready to provide any additional details.

Detect unused params used in recursive calls

Just found this in code of mine.

func Foo(unused bool) {
    if someCond {
        Foo(unused)
    }
}

Be smarter about what func signatures to ignore

For example, if both a named signature and a func are unexported, we could look at the package's source code to check that the func is never used as that signature:

package foo

type t func(path string) // if this is removed, str is flagged.

func f(str string) {
        println("")
}

Other extra analysis could be added to fix some false negatives, probably. Ideas welcome.

Forked from #6.

Potential false negative, didn't report error result in unexported method that always returned nil.

This issue is inspired by a nice simplification I manually discovered recently, shurcooL/issues@beebdcc.

In it, the method reactions had return signature ([]reactions.Reaction, error), but the only return statement was always returning nil error.

I was going to open an issue in @dominikh's go-tools repo for consideration, but then realized that this should fit right into unparam's scope. From reading the README, I don't have a good answer for why this simplification opportunity wouldn't be reported.

Is this an enhancement opportunity, or am I missing a reason it couldn't/shouldn't have been detected?

I tested with latest unparam on the parent of that commit, and didn't get anything:

$ unparam github.com/shurcooL/issues/githubapi
$

With -debug, the output is:

$ unparam -debug
func ghUser
parameter user : *github.com/google/go-github/github.User
  skip - used somewhere in the func body
func ghRepoSpec
parameter repo : github.com/shurcooL/issues.RepoSpec
  skip - used somewhere in the func body
func ghIssueState
parameter state : github.com/shurcooL/githubql.IssueState
  skip - used somewhere in the func body
func init
func (service).reactions
parameter rgs : reactionGroups
  skip - used somewhere in the func body
func internalizeReaction
parameter reaction : github.com/shurcooL/githubql.ReactionContent
  skip - used somewhere in the func body
func (service).markRead
parameter ctx : context.Context
  skip - used somewhere in the func body
parameter repo : github.com/shurcooL/issues.RepoSpec
  skip - used somewhere in the func body
parameter id : uint64
  skip - used somewhere in the func body
func ghEventType
parameter typename : string
  skip - used somewhere in the func body
func externalizeReaction
parameter reaction : github.com/shurcooL/reactions.EmojiID
  skip - used somewhere in the func body
func ghActor
parameter actor : *githubqlActor
  skip - used somewhere in the func body
func ghColor
parameter hex : string
  skip - used somewhere in the func body

Potential false positive or confusing/inconsistent report.

Using latest versions of everything, Go 1.9rc1 on latest macOS.

$ unparam github.com/shurcooL/home
users.go:79:19: ctx is unused

This is where it refers to:

func (Users) Edit(ctx context.Context, er users.EditRequest) (users.User, error) {
	return users.User{}, errors.New("Edit is not implemented")
}

Which is fine, ctx is indeed unused and I have no problems doing this:

func (Users) Edit(_ context.Context, er users.EditRequest) (users.User, error) {
	return users.User{}, errors.New("Edit is not implemented")
}

However, it's a bit surprising/inconsistent in that er is also unused, yet there is no report for that. How come?

Also, the README says:

To minimise false positives, it ignores:

Unnamed and underscore parameters

Funcs whose signature matches a reachable func type

Funcs whose signature matches a reachable interface method

Funcs that have empty bodies

Funcs that will almost immediately panic or return constants

This is obviously an unimplemented method that returns an error right away. Shouldn't it be ignored because of "Funcs that will almost immediately panic or return constants"? Or not, because it returns errors.New, which isn't a constant?

It's completely possible everything is working as intended and there is no problem, but I wanted to report this on the off chance something here is unintentional. If things are working as expected, perhaps you can clarify which clause explains this behavior.

Common linter interface

I propose that all Go linters that work on vanilla x/tools/go/loader (go/ast + go/types) and x/tools/go/ssa implement an interface, to allow running multiple of them at once while sharing parsing, type-checking and SSA work.

The interface could be something like:

type Checker interface{
    Check(*loader.Program, *ssa.Program) ([]Warning, error)
}

type Warning struct {
    Pos token.Pos
    Msg string
}

Each linter would have an importable package with a type satisfying this interface.

detect cases where no caller uses a return value

Via https://go-review.googlesource.com/c/55232, the signature of lookupName is:

func (l *Location) lookupName(name string, unix int64) (offset int, isDST bool, ok bool) {

But the only caller has this:

offset, _, ok := local.lookupName(zoneName, t.unixSec())

If every caller discards a return parameter, we could probably avoid using it in the function signature. (This would be difficult to implement, apparently)

Allow setting of tests struct field on the Checker type

golangci-lint uses unparam, but it's currently impossible to set the tests flag on the Checker type in unparam, so that configuration cannot be defined in golangci-lint (I created an issue for it here: golangci/golangci-lint#2046)

I'd like to create an exported function on the Checker type called Tests that allows setting of that tests field the same way you do with the Packages function no that type so people who import your code can configure that behaviour.

Don't report funcs that have multiple implementations

We already ignore stub functions, but build flags and GOOS/GOARCH combinations can mean that a function has multiple implementations, and all of them must have the same signature.

Unless all of them have the same unused parameter, we shouldn't warn at all.

Example:

foo_linux.go:

func foo(a, b int) int {
    return a + 10
}

foo_windows.go:

func foo(a, b int) int {
    return a + b
}

unparam -help doesn't print usage.

$ unparam -help
Usage of unparam:
  -tests
    	include tests (default true)

That doesn't answer the questions of "what parameters does unparam take? how do I use it? can I specify packages, or does it always use current directory? do I need to give it individual .go files?"

I had to look at the source code and/or guess to figure it out.

Compare with:

$ unconvert -help
usage: unconvert [flags] [package ...]
  -all
    	type check all GOOS and GOARCH combinations
  -apply
    	apply edits to source files
  -cpuprofile string
    	write CPU profile to file
  -safe
    	be more conservative (experimental)
  -v	verbose output

unconvert helpfully tells me that usage is unconvert [flags] [package ...], so I know how to use it.

I think doing this would be sufficient to fix this problem:

$ unparam -help
usage: unparam [flags] [package ...]
  -tests
    	include tests (default true)

Is this positive case?

I got a hit where row is unused:

manager.go:417:31: (*Manager).termSize - result row is never used (unparam)
func (m *Manager) termSize() (row uint, column uint) {

However, I did use it as an early declaration. The code is:

func (m *Manager) termSize() (row uint, column uint) {                         
        t := term.NewTerminal(term.NoTerminal)                                 
        row, column = t.Size()                                                 
                                                                               
        if m.maxColumn != 0 {                                                  
                column = m.maxColumn                                           
        }                                                                      
                                                                               
        return row, column                                                     
}

Is this positive?

Assigning an unused parameter to a blank identifier doesn't make it used.

This seems like an unintentional behavior, but please correct me otherwise.

In most other contests, assigning something to the blank identifier makes it used. It's often used to "force" a use to avoid compilation errors, when there's no other normal use because the code is in development.

This doesn't seem to work for unparam. Consider the program:

package p

import "fmt"

func foo(v string) {
	fmt.Println("this does some stuff")
	fmt.Println("not a stub, etc.")
	if true {
		fmt.Println("really, there's complex logic!")
	}

	_ = v // TODO: Actually use v.
}

I would expect the _ = v line to cause v to be marked as "used" and not get printed as an unused parameter, but that's not what happens with latest unparam:

$ unparam
main.go:5:10: foo - v is unused

Or are there good reasons why assigning to a blank identifier should not be considered a "use"? /cc @dominikh

False positive on implementing interface

golangci-lint version
# golangci-lint has version 1.39.0 built from 9aea4aee on 2021-03-26T08:02:53Z

// .golangci.yml
linters:
  disable-all: true
  enable:
    - unparam

// lower implements encoding.TextUnmarshaler to force lowercase
type lower string

// unparam: (*lower).UnmarshalText - result 0 (error) is always nil
func (l *lower) UnmarshalText(b []byte) error {
    s := strings.ToLower(string(b))

    *l = lower(s)
    return nil
}

Flags necessary elements of function signature

Looking at https://github.com/akerl/voyager/blob/b3c2a92371cac8d4a59d5f058d71e3cc011c9310/prompt/wmenu.go#L17-L20 , I get the following error:

~/.go/src/github.com/akerl/voyager/prompt master*
❯ unparam -debug
func WithWmenu$1
parameter opts : []github.com/akerl/voyager/vendor/github.com/dixonwille/wmenu.Opt
  skip - used somewhere in the func body
wmenu.go:17:39: WithWmenu$1 - result 0 (error) is always nil

The code in question:

	menu.Action(func(opts []wmenu.Opt) error {
		c <- opts[0].ID
		return nil
	})

While it's correct that I'm not ever setting the return to something other than nil, the error is a crucial part of the function signature, given that it's needed by menu.Action()

Flag to ignore unused error return parameters

Hi Daniel!

When designing a function I want to return an error that can occur. Often I do not use that error however as it does not matter in that situation. Then unparam flags that as an unused parameter.

It would be nice to have the option to ignore unused error return parameters.

A typical example would be a Close() function with an error returned. Most of the time I don't care if I was able to close something or not.

PS: Cool stuff you make here!

Go-RPC

Go-RPC needs methods return error but sometimes there is no error with the method and unparam create an error. Consider the following method as an example:

// Multiply multiplies given parameters and stores it result
func (t *Arith) Multiply(args *Args, reply *int) error {
	*reply = args.A * args.B
	return nil
}

mvdan.cc certificate expired

Seems like your certificate for mvdan.cc expired as of Tuesday, February 11, 2020 at 2:08:46 PM.
Can you please update it, so we can keep using your great packages for making better software? :)

Panic on Go 1.8 struct conversion ignoring field tags.

$ go get -u github.com/mvdan/unparam
$ go get -u github.com/shurcooL/users/fs
$ unparam github.com/shurcooL/users/fs
panic: in github.com/shurcooL/users/fs.fromUserSpec: cannot convert *t0 (github.com/shurcooL/users.UserSpec) to github.com/shurcooL/users/fs.userSpec

goroutine 775 [running]:
golang.org/x/tools/go/ssa.emitConv(0xc4274f5400, 0x13fe440, 0xc42f56dda0, 0x13fa400, 0xc42711d2f0, 0xc42f56dda0, 0xc42f4fee40)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/emit.go:242 +0x9e5
golang.org/x/tools/go/ssa.(*builder).expr0(0xc42e085d00, 0xc4274f5400, 0x13fae40, 0xc420013dc0, 0x7, 0x13fa400, 0xc42711d2f0, 0x0, 0x0, 0xc420030800, ...)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:573 +0x30f0
golang.org/x/tools/go/ssa.(*builder).expr(0xc42e085d00, 0xc4274f5400, 0x13fae40, 0xc420013dc0, 0x104b0c0, 0xc420030800)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:530 +0x299
golang.org/x/tools/go/ssa.(*builder).stmt(0xc42e085d00, 0xc4274f5400, 0x13fb500, 0xc4200fea60)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2045 +0x1f6c
golang.org/x/tools/go/ssa.(*builder).stmtList(0xc42e085d00, 0xc4274f5400, 0xc420011650, 0x1, 0x1)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:790 +0x61
golang.org/x/tools/go/ssa.(*builder).stmt(0xc42e085d00, 0xc4274f5400, 0x13fadc0, 0xc4200e6cf0)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2102 +0x2df7
golang.org/x/tools/go/ssa.(*builder).buildFunction(0xc42e085d00, 0xc4274f5400)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2195 +0x35c
golang.org/x/tools/go/ssa.(*builder).buildFuncDecl(0xc42e085d00, 0xc426f49620, 0xc4200e6d20)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2225 +0x12e
golang.org/x/tools/go/ssa.(*Package).build(0xc426f49620)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2341 +0x684
golang.org/x/tools/go/ssa.(*Package).(golang.org/x/tools/go/ssa.build)-fm()
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2260 +0x2a
sync.(*Once).Do(0xc426f4964c, 0xc42c49af98)
	/usr/local/go/src/sync/once.go:44 +0xbe
golang.org/x/tools/go/ssa.(*Package).Build(0xc426f49620)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2260 +0x55
golang.org/x/tools/go/ssa.(*Program).Build.func1(0xc42c3ac8e0, 0xc426f49620)
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2244 +0x2b
created by golang.org/x/tools/go/ssa.(*Program).Build
	/Users/Dmitri/Dropbox/Work/2013/GoLand/src/golang.org/x/tools/go/ssa/builder.go:2246 +0x141

Problem line:

https://github.com/shurcooL/users/blob/16fca626b3e90416822651350babb0667b2bbe33/fs/schema.go#L79

It's the new-to-Go-1.8 struct conversion that ignores struct field tags.

How to suppress the error in this situation?

When I used a third-party library and provided a callback function for its API, I couldn't handle every parameter properly. Sometimes I just want to deal with some of these parameters.

For example:

func (ui *UI) historyInputCapture(event *tcell.EventKey) *tcell.EventKey {
	switch event.Key() {
	case tcell.KeyCtrlB, tcell.KeyPgUp:
		ui.pageUp(10)
	case tcell.KeyCtrlF, tcell.KeyPgDn:
		ui.pageDown(10)
	case tcell.KeyRune:
		switch event.Rune() {
		case 'k':
			ui.pageUp(1)
		case 'j':
			ui.pageDown(1)
		case 'g':
			ui.pageHome()
		case 'G':
			ui.pageEnd()
		}
	}

	return nil
}

Here return nil means I don't want the event to be passed on.
But unparam gave me an error:

ui/ui.go:180:58: (*UI).historyInputCapture - result 0 (*github.com/gdamore/tcell.EventKey) is always nil (unparam)
func (ui *UI) historyInputCapture(event *tcell.EventKey) *tcell.EventKey {
                                                         ^

How to suppress the error in this situation?

fails on itself at tip

unparam fails at tip.
This shows it failing on itself. I've also seen it fail on a package that imports "context" with a similar message.

> gotip download                                                                                                      
Updating the go development tree...                                                                                   
remote: Total 1085 (delta 602), reused 993 (delta 602)                                                                
Receiving objects: 100% (1085/1085), 1.79 MiB | 9.23 MiB/s, done.                                                     
Resolving deltas: 100% (602/602), completed with 133 local objects.                                                   
From https://go.googlesource.com/go                                                                                   
 * branch            master     -> FETCH_HEAD                                                                         
   0fa53e4..474ebb9  master     -> origin/master                                                                      
Previous HEAD position was 0fa53e4 spec: fix link for instantiations                                                  
HEAD is now at 474ebb9 syscall: avoid writing to p when Pipe(p) fails  
...
> git clone https://github.com/mvdan/unparam 
> cd unparam
> gotip run .                                                                                                         
2021/12/09 09:32:12 internal error: package "flag" without types was imported from "mvdan.cc/unparam"                 
exit status 1

Also flag dead arguments

Like:

func Foo(a int) {
    a = 3
    println(3)
}

In other words, parameters whose values are never used/read, but they're still used in a statement somehow (like overwritten, see above).

don't warn when removing all result parameters would add complexity to the code

For example, say that the result of foo here is never used, as it's always called like foo() or _ = foo():

func foo() bar {
    if cond {
        return foo2()
    }
    return foo3()
}

However, following unparam's advice to remove the result parameter will make the code longer:

func foo() {
    if cond {
        foo2()
        return
    }
    foo3()
}

A couple of examples from go version devel +4a52452a2f Wed Feb 28 22:05:23 2018 +0000 linux/amd64:

regexp/syntax/parse.go:297:27: (*parser).concat - result 0 (*regexp/syntax.Regexp) is never used
regexp/syntax/parse.go:317:30: (*parser).alternate - result 0 (*regexp/syntax.Regexp) is never used

Remove the "always receives X" check entirely

We made it more conservative in #14, but it's still prone to false positives.

I've gone through all the warnings from this linter that I have applied, and this kind were almost non-existent.

Does anyone find it useful? Does anyone find it annoying? Input welcome.

main package has no godoc

Effective Go says:

Every package should have a package comment, a block comment preceding the package clause.

The command mvdan.cc/unparam does not have a package comment.

This can be seen by visiting https://pkg.go.dev/mvdan.cc/unparam or running go doc mvdan.cc/unparam inside the module.

proposal: don't group exported and unexported methods of an interface together?

(Wrong repo, sorry for the noise.)

Detect unexported func parameters that always receive the same value

A rather different check to what this tool usually does. But it seems in the "unnecessary parameter" realm like unused parameters.

implement using honnef.co/go/tools/lint

This would be helpful for users like CockroachDB's metacheck: https://github.com/cockroachdb/cockroach/blob/master/pkg/cmd/metacheck/main.go which wants to run many checks without having to load the program more than once.

Possible to skip directories through flag?

I'm running unparam through golangci-lint. One of my packages has some issues, and I'd like to skip that for the unparam check, but leave it for the remaining ones.

Is this possible with the existing flags?

Ignoring leading underscore _parameters

In Elixir and Rust, variable names prefixed with an underscore are meant to flag unused variables. In fact, not using the underscore is considered an error by the compiler

The advantage is documentation: still giving a name to what's received while also flagging it as unused.

Similar to #35, an easy way to quiet unparam would be:

func run(cmd *cobra.Command, _args []string) {
  cmd.Usage()
}

What's your take on this?

False positive about result err is always nil

The following code:

package main

import (
        "fmt"
)

func main() {
	modesp := map[int]func(int) (int, int, error){
		1: func(i int) (next int, m int, err error) {
			next = i + 1
			return -1, -1, fmt.Errorf("unexpected char[%v] after %% at %d", next, i)
		},
		2: func(i int) (next int, m int, err error) {
			if i == 1 {
 				next = 1
				m = 1
				return
			}

			return
		},
	}
       _ = modesp
}

returns the following unparam issue：

main.go:13:36: main$2 - result err is always nil

I have update unparam use：

go get -u -v mvdan.cc/unparam

false negative

Is this a false negative ?

// +build !test

package main

import (
	"log"
)

func test(a func(tx int) error) {
	log.Print(a)
}

func main() {
	test(func(tx int) error {
                // tx is not used
		return nil
	})
	return
}

debug output: 
func test
  skip - dummy implementation
func main
func main$1
  skip - dummy implementation

If I just wanted to satisfy the interface, I would write this instead:

func main() {
	test(func(int) error {
                // tx is not used
		return nil
	})
	return
}

Or this

func main() {
	test(func(_ int) error {
                // tx is not used
		return nil
	})
	return
}

disable warnings on funcs which use go:linkname

//go:linkname reflect_typedmemclrpartial reflect.typedmemclrpartial
func reflect_typedmemclrpartial(typ *_type, ptr unsafe.Pointer, off, size uintptr) {
    if writeBarrier.needed && typ.ptrdata != 0 {
        bulkBarrierPreWrite(uintptr(ptr), 0, size)
    }
    memclrNoHeapPointers(ptr, size)
}

off here is an unused parameter, but it cannot be removed, as otherwise the go:linkname would break. I guess the user could rename the parameter to _, but that would be somewhat awkward.

panic: interface conversion: ast.Expr is ast.IndexExpr, not ast.Ident

The generics support in golang.org/x/tools/go/ssa seems to be mostly ready (see golang/go#48525 (comment)), so I've updated it to the latest master commit, compiled unparam, and tried using it on some generic code. The result is a panic:

panic: interface conversion: ast.Expr is *ast.IndexExpr, not *ast.Ident

goroutine 1 [running]:
mvdan.cc/unparam/check.recvPrefix(...)
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check .go:880
mvdan.cc/unparam/check.(*Checker).declCounts(0xc000124960, {0xc0001c0c80, 0x2a}, {0xc0001c9fe8 , 0x6})
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check .go:855 +0x3f4
mvdan.cc/unparam/check.(*Checker).multipleImpls(0xc000124960, 0xc0001dbe00, 0xc002cc2000)
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check.go:893 +0x105
mvdan.cc/unparam/check.(*Checker).checkFunc(0xc000124960, 0xc002cc2000, 0xc0001c0c80?)
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check.go:493 +0x27e
mvdan.cc/unparam/check.(*Checker).Check(0xc000124960)
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check.go:377 +0x76b
mvdan.cc/unparam/check.(*Checker).lines(0xc000124960, {0xc000010190?, 0x4c736f?, 0x40c8c5?})
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check.go:107 +0x16a
mvdan.cc/unparam/check.UnusedParams(0x0, 0x0, 0x0, {0xc000010190, 0x2, 0x2})
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/check/check.go:44 +0xe5
main.main1()
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/main.go:37 +0x151
main.main()
        /home/ainar/go/pkg/mod/mvdan.cc/[email protected]/main.go:23 +0x19

The reproducer code:

package main

type set[T comparable] struct {
	m map[T]struct{}
}

func (s set[T]) has(v T) (ok bool) {
	_, ok = s.m[v]

	return ok
}

func main() {
	s := set[int]{}
	_ = s.has(0)
}

Versions:

go version -m ./bin/unparam

./bin/unparam: go1.18.2
        path    mvdan.cc/unparam
        mod     mvdan.cc/unparam        v0.0.0-20220316160445-06cc5682983b      h1:C8Pi6noat8BcrL9WnSRYeQ63fpkJk3hKVHtF5731kIw=
        dep     golang.org/x/mod        v0.6.0-dev.0.20220419223038-86c51ed26bb4        h1:6zppjxzCulZykYSLyVDYbneBfbaBIQPYMevg0bEwv2s=
        dep     golang.org/x/sys        v0.0.0-20220513210249-45d2b4557a2a      h1:N2T1jUrTQE9Re6TFF5PhvEHXHCguynGhKjWVsIUt5cY=
        dep     golang.org/x/tools      v0.1.11-0.20220517032152-814e0d74b53f   h1:uWV+yNYrw4eleKDoYWdMeHM4sAErndcO+Z71LxvDNvA=
        build   -compiler=gc
        build   CGO_ENABLED=1
        build   CGO_CFLAGS=
        build   CGO_CPPFLAGS=
        build   CGO_CXXFLAGS=
        build   CGO_LDFLAGS=
        build   GOARCH=amd64
        build   GOOS=linux
        build   GOAMD64=v1

unparam does not support build tags

unparam currently doesn't support build tags, which means packages which require a non-implicit build tag can't be linted, e.g.:

main.go

package main

import (
	"./sub"
	"fmt"
)

func main() {
	fmt.Printf("Hello, %s!\n", sub.Get())
}

sub/sub_a.go

//+build a

package sub

func Get() string {
	return "world"
}

sub/sub_b.go

//+build b

package sub

func Get() string {
	return "everyone"
}

sub/sub.go

package sub

// some shared code

$ ls
main.go  sub
$ unparam .
.../main.go:9:29: Get not declared by package sub
couldn't load packages due to errors: .
$ go run -tags a main.go 
Hello, world!
$ go run -tags b main.go 
Hello, everyone!

Don't say "always receives <value>" when the value is passed exactly once

Otherwise, this check is too subjective. If the same value is sent 3 or more times however, that's when it starts being a clear issue.

goreleaser: error: unknown long flag '--snapshot', try --help

Are there any updates recently? The github actions are broken.

workflow:
https://github.com/mudclient/go-mud/actions/runs/37026103/workflow

unparam doesn't detect this offender

This one.

Back when I had it at the top of the file rather than the bottom, it did though! But now, zero output. My version is -- ah no --version flag but ~2-3 days fresh pretty much. I can't wait to learn the cause of that odd bug 😁

Ignore funcs that get func call values as parameters?

func Foo(a, b int) {
    println(a)
}

func Bar() (int, int) {
    return 2, 3
}

func main() {
    Foo(Bar())
}

Should we report the unused b parameter in Foo? Right now we do.

Points in favor of doing it:

Technically an unused parameter
Code can be rewritten to accomodate, e.g. a, _ := Bar(); Foo(a)
Simpler linter implementation, as we don't need to look at func calls

Disadvantages:

Unlike the rest of the current warnings, the suggested change is more intrusive

What I mean by more intrusive is, say, if besides Foo we have Foo2 which does use both parameters. We would make the actuall calls to Foo and Foo2 different, whereas they were almost the same before.

I tend to think we should do the suggestion, but not 100% sure.

Unnamed parameters and test doubles

Hi,

First: congrats, this tool is very nice!

Second, I'd like to discuss a common case scenario in the projects I work with.

Let's say we have an interface:

type x interface {
    func y(a int, b string) error
}

Sometimes I have a dummy implementation of this interface in a test where some (or all) parameters are not used. What I thought is that making them not used explicitly could be achieved unamming those params, like:

func (m myType) y(int, string) error {
    // no names ___^_____^ 
    // do nothing
    return nil
}

To not trigger an unparam error, maybe some flag could be added so if the param does not have a name then it wouldn't error.

What do you think?

Detect return parameters that always have the same value

We can start with constants. For example:

func foo() int {
    if someCond() {
        return 2
    }
    return 2
}

False positive about unused return values

The following code

package main

import "github.com/aws/aws-sdk-go-v2/aws"

func main() {
	_ = aws.EndpointResolverFunc(func(string, string) (aws.Endpoint, error) {
		return aws.Endpoint{URL: "url"}, nil
	})
}

returns the following unparam issue:

main.go:6:67: main$1 - result 1 (error) is always nil

However, I cannot remove that error return value because that would change the function signature and it wouldn't match the one expected by the AWS SDK anymore.

Detect return parameters that are just a receiving parameter

In other words, when the values must already be in the hands of the caller.

For example, the receiver:

func (f *foo) bar() foo {
    doWork()
    return f
}

A receiver's field:

func (f *foo) bar() int {
    doWork()
    return f.intField
}

A received parameter:

func bar(n int) int {
    doWork()
    return n
}

Of course, the value must not have been modified under any of those cases.

Don't consider the blank identifier as having a name.

Results that are named after blank identifier are currently considered as "having a name", but in reality, they should be treated as not having a name. The _ name is not descriptive, and there can be more than 1 of them.

Got

foobar - result _ is always nil

Expected

foobar - result 1 (error) is always nil

Cause

unparam/check/check.go

Lines 665 to 671 in 0c3aec2

 func paramDesc(i int, v *types.Var) string { 

 name := v.Name() 

 if name != "" { 

 return name 

 } 

 return fmt.Sprintf("%d (%s)", i, v.Type().String()) 

 }

I believe that code should be changed:

func paramDesc(i int, v *types.Var) string {
	name := v.Name()
-	if name != "" {
+	if name != "" && name != "_" {
		return name
	}
	return fmt.Sprintf("%d (%s)", i, v.Type().String())
}

Issue on function that have parameters that shouldn't be removed

Type

Bug report

Current

Actually when I try to validate a command created using cobra CLI library I receive a failure for unused param, eg:

package main

import (
	"github.com/spf13/cobra"
)

func main() {
	rootCmd.Execute()
}

var rootCmd = &cobra.Command{
	Use:   "demo",
	Short: "demo",
	Run: func(cmd *cobra.Command, args []string) {
		cmd.Usage()
	},
}

Generate:

main.go:14:32: init$1 - args is unused

but since the Run is a function that need args we can't skip it.

Command with debug flag report:

func init$1
parameter cmd : *github.com/spf13/cobra.Command
  skip - used somewhere in the func body
parameter args : []string
func main
main.go:14:32: init$1 - args is unused

Expected

No issue detected

Additional note

This behaviour seems an issue added some days ago, since was working "as expected" some day ago: https://travis-ci.org/mavimo/hetzner-kube/jobs/441696541#L554 but is broken now https://travis-ci.org/mavimo/hetzner-kube/jobs/447106130#L504

panics when encountering generics

The branch is tidwall-piece-request-order if you want to reproduce. Cheers

anacrolix@anacrolix-mbp-2018:~/go/src/github.com/anacrolix/torrent$ unparam .
panic: no concrete method: func (*github.com/tidwall/btree.BTree[github.com/anacrolix/torrent/request-strategy.pieceRequestOrderItem]).Ascend(pivot github.com/anacrolix/torrent/request-strategy.pieceRequestOrderItem, iter func(item github.com/anacrolix/torrent/request-strategy.pieceRequestOrderItem) bool)

goroutine 2630 [running]:
golang.org/x/tools/go/ssa.(*Program).declaredFunc(0xc000403d40, 0xc000526780)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:124 +0xf9
golang.org/x/tools/go/ssa.(*Program).addMethod(0x12ee198?, 0xc001fb7380, 0xc001fc41e0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:86 +0x14a
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee198?, 0xc001181600?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:173 +0x785
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee210?, 0xc001252180?}, 0x1)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:228 +0x669
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee170?, 0xc001192e00?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:221 +0x588
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee198?, 0xc00102ba90?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:193 +0x307
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee148?, 0xc0010a3c60?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:203 +0x3a6
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee210?, 0xc000f47f80?}, 0x1)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:228 +0x669
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee170?, 0xc001413280?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:221 +0x588
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee198?, 0xc00155da60?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:193 +0x307
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee238?, 0xc000ff8e28?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:233 +0x708
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee198?, 0xc000d81390?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:181 +0x1af
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee238?, 0xc0013f3e78?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:233 +0x708
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee198?, 0xc000d814c0?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:181 +0x1af
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee238?, 0xc0013f3fe0?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:233 +0x708
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee198?, 0xc0007be730?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:182 +0x1ce
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee210?, 0xc0010f8300?}, 0x1)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:228 +0x669
golang.org/x/tools/go/ssa.(*Program).needMethods(0xc000403d40, {0x12ee170?, 0xc000f2f400?}, 0x0)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:221 +0x588
golang.org/x/tools/go/ssa.(*Program).needMethodsOf(0xc000403d40, {0x12ee170?, 0xc000f2f400?})
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/methods.go:145 +0x70
golang.org/x/tools/go/ssa.(*Package).build(0xc001e2c120)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2281 +0x111
sync.(*Once).doSlow(0xc00095ffb8?, 0x11ef62c?)
	/Users/anacrolix/src/go.master/src/sync/once.go:68 +0xc2
sync.(*Once).Do(...)
	/Users/anacrolix/src/go.master/src/sync/once.go:59
golang.org/x/tools/go/ssa.(*Package).Build(...)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2269
golang.org/x/tools/go/ssa.(*Program).Build.func1(0x0?)
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2253 +0x4c
created by golang.org/x/tools/go/ssa.(*Program).Build
	/Users/anacrolix/go/pkg/mod/golang.org/x/[email protected]/go/ssa/builder.go:2252 +0x19c

Possible to ignore stubbed functions?

Is there anyway I can ignore stubbed functions? Specifically when they return errors? Here's an example:

func futureFeature(x int, y int) (z int, err error) {
   _, _ = x, y
    return 0, nil
}

This will warn me that err is always nil. I don't want to change the function signature as that would take away from the benefits of stubbing. There is no interface this function is adhering to so it won't get ignored.

Detect when args are always redundant because of another argument

For example, take this Golang CL: https://go-review.googlesource.com/c/go/+/100455

The func was always called like:

mkinlcall(n, n.Left, n.Isddd())
mkinlcall(n, f, n.Isddd())
etc

Since the func always receives n, receiving n.Method() is redundant.

Other redundant cases that come to mind, when a func receives x:

x.Field
x[i]
x[i:j]
*x

And of course, any combination of the above.

I'm leaving x.PureMethod() and PureFunc(x) out of the list as that's a much more complex one - we'd have to figure out that the func has no side effects. I don't think SSA does this just yet.

"X always receives Y" false positive with runtime constant

os/exec/exec.go:731:    return dedupEnvCase(runtime.GOOS == "windows", env)
os/exec/exec.go:736:func dedupEnvCase(caseInsensitive bool, env []string) []string {

$ unparam -tests=false std
os/exec/exec.go:736:19: caseInsensitive always receives false

Some ideas:

Never suggest this for non-trivial expressions, e.g. forbid binary and unary expressions
Blacklist expressions containing certain elements like runtime.GOOS
Blacklist all constants that depend on build tags and/or GOOS/GOARCH (how?)

	func paramDesc(i int, v *types.Var) string {
	name := v.Name()
	if name != "" {
	return name
	}
	return fmt.Sprintf("%d (%s)", i, v.Type().String())
	}

mvdan / unparam Goto Github PK

unparam's Introduction

unparam

unparam's People

Stargazers

Watchers

Forkers

unparam's Issues

Got

Expected

Cause

Type

Current

Expected

Additional note

Recommend Projects

Recommend Topics

Recommend Org