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 (2b8063fd56cd7096ee6d62783b62a87a832b06e7). Any idea what might be causing this discrepancy? The tool should not lint this function in the first place because it is unnamed right?

This issue is inspired by a nice simplification I manually discovered recently, https://github.com/shurcooL/issues/commit/beebdcce6ec100215fb607daf02e69cd18dc985a.

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

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?

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 😁

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!

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.

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.

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!

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.

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?

Latest commit (dc49ffed) is failing on our usage with Go 1.19.4. Previous commit (cac433cd) runs fine.

Subject: [PATCH] Change to test data which panics
---
Index: testdata/script/typeparams.txtar
===================================================================
diff --git a/testdata/script/typeparams.txtar b/testdata/script/typeparams.txtar
--- a/testdata/script/typeparams.txtar	(revision dc49ffedcfbc1b49598bb6670d37b92a7a9ad7d6)
+++ b/testdata/script/typeparams.txtar	(date 1671665772622)
@@ -53,6 +53,11 @@
 	return t2
 }
 
+func genericStructFunc[T1 any, T2 any](t Tuple[T1, T2]) T2 {
+	return t.right
+}
+
 // otherwise Tuple isn't reachable.
 var Sink interface{} = new(Tuple[int, string])

Test output from the above:

        > ! exec unparam .
        [stderr]
        panic: /usr/local/go/src/go/types/sizes.go:82: assertion failed
        
        goroutine 1 [running]:
        go/types.assert(0x60?)
        	/usr/local/go/src/go/types/errors.go:27 +0x5b
        go/types.(*StdSizes).Alignof(0xc000138220, {0x79b360?, 0xc0001da060})
        	/usr/local/go/src/go/types/sizes.go:82 +0x1b6
        go/types.(*StdSizes).Offsetsof(0x79b248?, {0xc00010db40, 0x2, 0xc000046c00?})
        	/usr/local/go/src/go/types/sizes.go:123 +0xdc
        go/types.(*StdSizes).Sizeof(0xc000138220, {0x79b248?, 0xc0001d80e0})
        	/usr/local/go/src/go/types/sizes.go:176 +0x285
        mvdan.cc/unparam/check.(*Checker).checkFunc(0xc000141040, 0xc00017cc00, 0xc0001443c0?)
        	/home/duncan/.local/src/github.com/mvdan/unparam/check/check.go:599 +0x99d
        mvdan.cc/unparam/check.(*Checker).Check(0xc000141040)
        	/home/duncan/.local/src/github.com/mvdan/unparam/check/check.go:377 +0x78b
        mvdan.cc/unparam/check.(*Checker).lines(0xc000141040, {0xc00011c010?, 0x4d460f?, 0xc00008da30?})
        	/home/duncan/.local/src/github.com/mvdan/unparam/check/check.go:107 +0x16a
        mvdan.cc/unparam/check.UnusedParams(0x0, 0x0, 0x0, {0xc00011c010, 0x1, 0x1})
        	/home/duncan/.local/src/github.com/mvdan/unparam/check/check.go:44 +0xe5
        mvdan.cc/unparam.main1()
        	/home/duncan/.local/src/github.com/mvdan/unparam/main.go:37 +0x151

//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.

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
}

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.

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

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.

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.