Add nilnil linter

[Antonboom]

https://github.com/Antonboom/nilnil

[ldez]

Hello,

I'm not sure to agree with the rule behind this linter.

And currently, it produces too many false positives, an example:

package foobar

import (
	"net/url"
)

type Foo struct {
	URL *url.URL
}

func example(val string) (*Foo, error) {
	endpoint, err := url.Parse(val)
	if err != nil {
		return nil, err
	}

	if endpoint.Host != "example.com" {
		return nil, nil
	}

	return &Foo{URL: endpoint}, nil
}

foo.go:18:3: return both the `nil` error and invalid value: use a sentinel error instead (nilnil)
                return nil, nil
                ^

[bombsimon]

And currently, it produces too many false positives, an example:

What's false positive here, you do return nil, nil on line 18. The author (and me as well) wants you to return nil, errors.New("invalid domain name")

Also I'm not sure we should reject linters because it's not useful for everyone. I would say that a big part of the linters are super opinionated and will never be enabled by the vast majority. That's also why we don't enable new linters by default.

[ldez]

Also I'm not sure we should reject linters because it's not useful for everyone.

It's not my point, I tested this linter on several real codebases, and it produces, from my point of view, a lot of false positives.

The cases where we need to not return an error are commons.

I have just "blocked" the PR and expressed my doubts, then the discussion is still opened.

That's also why we don't enable new linters by default.

We enable the new linters by default when a user uses enable-all or presets 😉

[bombsimon]

It's not my point, I tested this linter on several real codebases, and it produces, from my point of view, a lot of false positives.

The cases where we need to not return an error are commons.

I think this is similar to wrapcheck and goerr113, there's a lot of places where you can or want to return an inline or unwrapped error but the linters forces you to handle them in a specific way for a consistent code style. This linter seem to prefer returning and checking error instead of nilness which is a pretty common pattern and found in stdlib in a quite a few places as well. I would not call those false positives but I understand what you're saying.

I have just "blocked" the PR and expressed my doubts, then the discussion is still opened.

Yeah I know and that's why I contributed to it with my thoughts to the discussion. 😃 Would of course be nice if more people had opinions about this as well.

We enable the new linters by default when a user uses enable-all or presets wink

I think the outcome of the discussion(s) to keep enable-all was that people using it (including me) accepted to get linters with unintentional behaviors and if so manually disable them after upgrading golangci-lint so that would be the case for us using enable-all here as well.

[Antonboom]

@ldez, what exactly is false positive? And what behavior do you expect?

Please, see linter readme and examples for more details.

There is no technical way to determine when it is valid return nil, nil and when it is not - it depends on the context. The linter suggests that you generally abandon this pattern in favor of errors and checks all danger types (any pointers, maps, interfaces, channels and funcs, but you can configure it).

If you use return nil, nil all over the place, then just do not enable the linter, as @bombsimon wrote above.

[ldez]

What I consider as false-positive you consider that as a feature, then there is no false-positive.
I am still not convinced by this linter because for me it's not a relevant rule, but if some people think that it's useful, I will follow.

[Antonboom]

@ldez thank u!

In fact, the linter did not appear out of nowhere. Using in my working projects return nil, nil resulted in:

• checks like if err != nil ... if val != nil at multiple levels of app (and you have to go through the whole chain to figure out what's the matter)
• occasional panics
• comments like "be careful, the function can return nil, nil" (the same is in the std)

I myself would like to get more evidence base besides personal experience and common sense and I will be grateful for it.

I tried to express my position in readme:

I understand that each case needs to be analyzed separately,
but I hope that the linter will make you think again - 
is it necessary to use an ambiguous API or is it better to do it using a sentinel error?

In any case, you can just not enable the linter.

So no problem. I just awaiting for feedback from other reviewers.

@bombsimon, thank u for support :)

[ldez]

LGTM

[bombsimon]

💯