feat: add nakedefer linter

[rawen17]

nakedefer is a linter that finds defer functions which return anything.
https://github.com/GaijinEntertainment/go-nakedefer

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

• It must have a link to the linter repository.
• It must provide a short description about the linter.

Linter

• It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
• It must have a valid license and the file must contain the required information by the license, ex: author, year, etc.
• The linter repository must have a CI and tests.
• It must use go/analysis.
• It must have a valid tag, ex: v1.0.0, v0.1.0.
• It must not contain init().
• It must not contain panic(), log.fatal(), os.exit(), or similar.
• It must not have false positives/negatives. (the team will help to verify that)
• It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

• They must have at least one std lib import.
• They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

• The linter must be added to the list of available linters (alphabetical case-insensitive order).
  • enable and disable options
• If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
  • The values must be different from the default ones.
  • The default values must be defined in a comment.
  • The option must have a short description.

Others Requirements

• The files (tests and linter) inside golangci-lint must have the same name as the linter.
• The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
• The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
• The load mode (WithLoadMode(...)):
  • if the linter doesn't use types: goanalysis.LoadModeSyntax
  • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
• The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

• The linter should not use SSA. (currently, SSA does not support generics)
• The linter repository should have a readme and linting.
• The linter should be published as a binary. (useful to diagnose bug origins)

---

The golangci-lint team will edit this comment to check the boxes before and during the review.

This checklist does not imply that we will accept the linter.

[ldez]

Hello,

maybe I have missed something: why only defer that uses a function returning a function is a problem?

IMO using a function that returns something (not only a function) for a defer is a problem.

[xobotyi]

@ldez sometimes we're using factory for deferred functions, this linter tracks situation when deferred function returns a function and ensures that returned function is invoked, i.e. defer deferredFnFactory() would raise an error

[xobotyi]

Overall you're right, and maybe we have to make the linter stricter🤔

[rawen17]

Hello!
I\ve fixed this problem. Now defer linter сhecks that deferred call does not return anything.

[ldez]

@rawen17 you have to sign the CLA #3282 (comment)

[rawen17]

I've signed it already

[ldez]

vbuianov seems not to be a GitHub user.

The email that you are using to commit is not set up in your GitHub account, so you haven't signed.

[rawen17]

Sorry, now is signed

[Antonboom]

@rawen17, please, select more specific naming.

defer does not reflect the essence of the linter, as well as messages from it.

Moreover, it would be great to see among the examples something less abstract and closer to real life. Now it raise more questions than answers.

E.g., why is it invalid?

func funcDeferAnonymousReturnIntAndErr() {
	defer func() (int, error) { // want "deferred call should not return anything"
		return 1, nil
	}()
}

If you explain more clearly the reason of the linter, I can help with the name.

Thanks 👍

[rawen17]

@Antonboom, hi!
This linter was been created to prevent cases when defer functions return any types
There are two cases:

• defer function doesn't return anything - valid for linter
• defer function returns one or more types - invalid for linter
  In this case there is a function which has defer anonymous function inside and it returns two types (int and error)
  That's why it is invalid for linter

func funcDeferAnonymousReturnIntAndErr() {
	defer func() (int, error) { // want "deferred call should not return anything"
		return 1, nil
	}()
}

May be i misunderstood your question
If you mean why defer function must not return anything
i think, it is best practice because it error-prone cases

[Antonboom]

Okay, does the linter ignore io.Closer and other functions from popular interfaces that are usually deferred?

I propose something like:

• nakedefer (like nakedret)
• novaluedefer (like nonamedreturn)
• puredefer, cleandefer, etc.

[bombsimon]

Okay, does the linter ignore io.Closer and other functions from popular interfaces that are usually deferred?

Curious about this as well. If not, I see a lot of this pattern incoming:

defer func() {
    _, _ = whatIActuallyWanted()
}()

[rawen17]

In test/testdata/defer.go there are 2 examples

funcReturnErr - can be like io.Closer method Close which returns error

1. Valid for linter

func funcDeferNotReturnAnyType2() {
	defer func() {
		_ = funcReturnErr()
	}()
}

2. Invalid for linter

func funcDeferReturnErr() {
	defer funcReturnErr() // want "deferred call should not return anything"
}

[rawen17]

Change name linter 'defer' to 'nakedefer'
Add 'exclude' in config to exclude function names

[ldez]

@rawen17 can you rebase and fix the conflict?

[ldez]

@rawen17 why did you close the PR?

[rawen17]

No, it is my fault, i will try fix it
Can i open this PR again?

[ldez]

yes you can reopen this PR, but you need to add commits 😉

[rawen17]

@ldez Hello!
Can you please review this PR and tell me what i have to do for your approval?

[ldez]

Sorry for the late reply.

I share the point of view of @bombsimon maybe it can be a good idea to add io\.Close as default (only if no user configuration)

[Antonboom]

@xobotyi @rawen17 hi!

Could you please add default value for -e flag?
See comments above.

I run linter on large code base and receive warnings about:

defer t.Stop() // *time.Timer
defer response.Body.Close()
defer db.Close() // *pg.DB

P.S. It's also cool to not ignore README badges like "Go Report", "CI Build Status", etc.

P.P.S. Could be helpful

• errcheck excludes

• golangci-lint config

--exclude-use-default Use or not use default excludes:
  # EXC0001 errcheck: Almost all programs ignore errors on these functions and in most cases it's ok
  - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked