Enable rule.unused-paramater #17803

Gusted · 2021-11-24T15:48:11Z

As title, it enables a new linter rule.
Over the few weeks that I've traversed trough the code-base I've seen a little bit too many issues that were caused by not used the correct paramaters, this could be prevented if a rule was erroring that the code has a unused paramater. With this rule you can be sure that you only use the paramaters that are needed, and within code-review, the reviewer knows that a certain paramater is not used within the scope of that function.
Most are replaced with _ some of them are "fixed". Added few //FIXME/TODO for things that I saw while going to a bit too many files.

Just to note, I'm still using _ and not completely removing the parameter, as if I was reading such code it makes it more clear that it won't be used.

- As title, it enables a new linter rule. - Over the few weeks that I've traversed trough the code-base I've seen a little bit _too many_ issues that were caused by not used the correct paramaters, this could be prevented if a rule was erroring that the code has a unused paramater. With this rule you can be sure that you only use the paramaters that are needed, and within code-review, the reviewer knows that a certain paramater is not used within the scope of that function. - Most are replaced with `_` some of them are "fixed". Added few //FIXME/TODO for things that I saw.

wxiaoguang · 2021-11-24T15:50:21Z

For code like setup("hooks/post-receive.log", c.Bool("debug")), I think we should keep them and improve them, instead of removing the parameter.

Gusted · 2021-11-24T15:52:20Z

For code like setup("hooks/post-receive.log", c.Bool("debug")), I think we should keep them and improve them, instead of removing the parameter.

Yeah, its a bit of a harder task to do so. As you will now include file logging in the console logging as a optional thing. I wasn't able to do within 15 minutes, which is why its now a removed parameter.

wxiaoguang · 2021-11-24T15:53:20Z

And I worry about we shouldn't just remove these unused parameters.

Some unused parameters may indicate some old bugs.

wxiaoguang · 2021-11-24T15:56:06Z

modules/git/diff.go

@@ -293,7 +293,7 @@ func GetAffectedFiles(oldCommitID, newCommitID string, env []string, repo *Repos
 	err = NewCommand("diff", "--name-only", oldCommitID, newCommitID).
 		RunInDirTimeoutEnvFullPipelineFunc(env, -1, repo.Path,
 			stdoutWriter, nil, nil,
-			func(ctx context.Context, cancel context.CancelFunc) error {
+			func(_ context.Context, _ context.CancelFunc) error {


For example, I am not sure whether it is correct to ignore the cancel context.CancelFunc, usually it might be called somewhere.

So maybe we could spend enough time to review the suspicious usages one by one.

In this case it's fine. This internal function doesn't need to call cancel or to pay attention to the provided context as if the context is cancelled the stdout pipe should get closed and then the whole thing will stop.

Gusted · 2021-11-24T16:00:12Z

And I worry about we shouldn't just remove these unused parameters.
Some unused parameters may indicate some old bugs.

Damn, you're quick at spotting the real reason of this PR - I totally agree with you!

So, I first wanted to open up a issue about it that we could investigate the unused paramaters and see if they should be used. Yet nobody wants to scroll trough a list of 500 errors. So this PR was created so anyone can take a quick look and see if they recognize anything.

I don't even expect this PR to be merged as a whole, but rather as a "mega-thread" for possible bugs and mini-PR's whose fixes bugs that are now discovered.

wxiaoguang · 2021-11-24T16:01:41Z

modules/markup/common/footnote.go

@@ -232,7 +232,7 @@ func (b *footnoteBlockParser) Open(parent ast.Node, reader text.Reader, pc parse
 	return item, parser.HasChildren
 }

-func (b *footnoteBlockParser) Continue(node ast.Node, reader text.Reader, pc parser.Context) parser.State {
+func (b *footnoteBlockParser) Continue(_ ast.Node, reader text.Reader, _ parser.Context) parser.State {


Here Continue(node ast.Node and above Open(parent ast.Node, after the refactoring, it seems more difficult to understand.

Is it possible to use _node ast.Node and _parent ast.Node to pass the lint?

Is it possible to use _node ast.Node and _parent ast.Node to pass the lint?

modules/markup/common/footnote.go:286:32: parameter '_node' seems to be unused, consider removing or renaming it as _

Unfortunately not, but we can make use of here by removing _ such that only the type is displayed. Or we could use //revive:disable.....
But that is just more hacks IMO

Well .... maybe we can suggest the lint authors to accept such styles .... let lint users decide whether they accept _var variables.

Agree, would prefer _node over _ because it preserves the meaning while marking the parameter unused. We have this exact setup also for JS:

gitea/.eslintrc

Line 305 in 49933c7

no-unused-vars: [2, {args: all, argsIgnorePattern: ^_, varsIgnorePattern: ^_, caughtErrorsIgnorePattern: ^_, ignoreRestSiblings: false}]

Is it possible to configure the go linter like that as well?

Yeah that would makes sense, that it could be configured within the settings, I've opened a Issue for it mgechev/revive#613

Well .... maybe we can suggest the lint authors to accept such styles .... let lint users decide whether they accept _var variables.

Not much luck on that one.

Suggested change

func (b *footnoteBlockParser) Continue(_ ast.Node, reader text.Reader, _ parser.Context) parser.State {

func (b *footnoteBlockParser) Continue(ast.Node, reader text.Reader, parser.Context) parser.State {

I think such thing can be used to have the readability.

func (b *footnoteBlockParser) Continue(ast.Node, reader text.Reader, parser.Context) parser.State

I'm not sure the compiler lets you mix named and unnamed parameters

It's still better to keep the names.

Open(parent ast.Node, ...
Continue(node ast.Node, ...

They have different meanings.

They have different meanings.

These parameters mean nothing to those methods (because they are unused)
What is the interest of documenting (putting a name to) something that is not used?

As user of the method it seems, to me, more clear a _ than an actual name.
_ means "do not take care of this parameter, it has no influence in the method behavior", then as user of the method I know (instantly) that I can pass anything as argument. Contrary, if the parameter has a name, I will need to take care of providing an argument that fits the expectations (e.g. a valid context or an actual parent ast.Node)... for nothing because the parameter is not used.

modules/notification/action/action.go

modules/notification/webhook/webhook.go

Gusted · 2021-11-24T17:31:29Z

modules/session/db.go

@@ -91,7 +91,7 @@ type DBProvider struct {

 // Init initializes DB session provider.
 // connStr: username:password@protocol(address)/dbname?param=value
-func (p *DBProvider) Init(maxLifetime int64, connStr string) error {
+func (p *DBProvider) Init(maxLifetime int64, _ string) error {


This also looks suspicious given the comment above it.

Yeah, the parameter meaning lost.

It is the comment that is misleading because connStr has no effect in the method behavior (i.e. the parameter means nothing to this method)

Yeah the comment is old.

zeripath · 2021-11-26T07:52:28Z

models/branches.go

@@ -581,7 +580,7 @@ func RemoveDeletedBranch(repoID int64, branch string) error {
 }

 // RemoveOldDeletedBranches removes old deleted branches
-func RemoveOldDeletedBranches(ctx context.Context, olderThan time.Duration) {
+func RemoveOldDeletedBranches(olderThan time.Duration) {


Please don't remove the context here.

We need to keep this because we're going to start pushing context through the stack in order to simplify things like process hierarchies, logging and db transactions.

zeripath · 2021-11-26T07:56:16Z

models/repo_generate.go

@@ -80,7 +80,7 @@ func GenerateTopics(ctx context.Context, templateRepo, generateRepo *Repository)
 }

 // GenerateGitHooks generates git hooks from a template repository
-func GenerateGitHooks(ctx context.Context, templateRepo, generateRepo *Repository) error {
+func GenerateGitHooks(templateRepo, generateRepo *Repository) error {


Please don't remove context here.

zeripath · 2021-11-26T07:58:44Z

modules/git/commit_info_nogogit.go

@@ -99,7 +99,7 @@ func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath
 	return commitsInfo, treeCommit, nil
 }

-func getLastCommitForPathsByCache(ctx context.Context, commitID, treePath string, paths []string, cache *LastCommitCache) (map[string]*Commit, []string, error) {
+func getLastCommitForPathsByCache(commitID, treePath string, paths []string, cache *LastCommitCache) (map[string]*Commit, []string, error) {


Please don't remove context

zeripath · 2021-11-26T07:58:59Z

modules/git/commit_info_nogogit.go

@@ -31,7 +31,7 @@ func (tes Entries) GetCommitsInfo(ctx context.Context, commit *Commit, treePath
 	var revs map[string]*Commit
 	if cache != nil {
 		var unHitPaths []string
-		revs, unHitPaths, err = getLastCommitForPathsByCache(ctx, commit.ID.String(), treePath, entryPaths, cache)
+		revs, unHitPaths, err = getLastCommitForPathsByCache(commit.ID.String(), treePath, entryPaths, cache)


Please don't remove context

zeripath · 2021-11-26T08:07:03Z

modules/lfs/filesystem_client.go

@@ -39,7 +39,7 @@ func (c *FilesystemClient) objectPath(oid string) string {
 }

 // Download reads the specific LFS object from the target path
-func (c *FilesystemClient) Download(ctx context.Context, objects []Pointer, callback DownloadCallback) error {
+func (c *FilesystemClient) Download(_ context.Context, objects []Pointer, callback DownloadCallback) error {


The lack of use of ctx here is possibly an error and it likely should be being checked before callback is called.

zeripath · 2021-11-26T08:07:38Z

modules/lfs/filesystem_client.go

@@ -58,7 +58,7 @@ func (c *FilesystemClient) Download(ctx context.Context, objects []Pointer, call
 }

 // Upload writes the specific LFS object to the target path
-func (c *FilesystemClient) Upload(ctx context.Context, objects []Pointer, callback UploadCallback) error {
+func (c *FilesystemClient) Upload(_ context.Context, objects []Pointer, callback UploadCallback) error {


Similarly here.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 24, 2021

wxiaoguang reviewed Nov 24, 2021

View reviewed changes

modules/notification/action/action.go Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Gusted mentioned this pull request Nov 24, 2021

Allow use of _xxx with unused-parameter rule mgechev/revive#613

Closed

Gusted commented Nov 24, 2021

View reviewed changes

modules/notification/webhook/webhook.go Show resolved Hide resolved

Gusted commented Nov 24, 2021

View reviewed changes

zeripath reviewed Nov 26, 2021

View reviewed changes

Gusted closed this Mar 5, 2022

go-gitea locked and limited conversation to collaborators Apr 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable rule.unused-paramater #17803

Enable rule.unused-paramater #17803

Gusted commented Nov 24, 2021

wxiaoguang commented Nov 24, 2021

Gusted commented Nov 24, 2021

wxiaoguang commented Nov 24, 2021

wxiaoguang Nov 24, 2021

zeripath Nov 26, 2021

Gusted commented Nov 24, 2021

wxiaoguang Nov 24, 2021

Gusted Nov 24, 2021

wxiaoguang Nov 24, 2021

silverwind Nov 24, 2021

Gusted Nov 24, 2021 •

edited

Loading

Gusted Nov 25, 2021

chavacava Nov 26, 2021 •

edited

Loading

wxiaoguang Nov 26, 2021

chavacava Nov 26, 2021

This comment has been minimized.

Gusted Nov 24, 2021

lunny Nov 25, 2021

chavacava Nov 26, 2021

zeripath Nov 26, 2021

zeripath Nov 26, 2021

zeripath Nov 26, 2021

zeripath Nov 26, 2021

zeripath Nov 26, 2021

zeripath Nov 26, 2021

zeripath Nov 26, 2021

	func (b *footnoteBlockParser) Continue(_ ast.Node, reader text.Reader, _ parser.Context) parser.State {
	func (b *footnoteBlockParser) Continue(ast.Node, reader text.Reader, parser.Context) parser.State {

Enable rule.unused-paramater #17803

Enable rule.unused-paramater #17803

Conversation

Gusted commented Nov 24, 2021

wxiaoguang commented Nov 24, 2021

Gusted commented Nov 24, 2021

wxiaoguang commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gusted commented Nov 24, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gusted Nov 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chavacava Nov 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gusted Nov 24, 2021 •

edited

Loading

chavacava Nov 26, 2021 •

edited

Loading