errcheck should not detect "defer" with error

[jeevatkm]

It's common in go to use defer with Close() methods.

file, err := os.Open("/path/to/file")
if err != nil {
    return err
}
defer file.Close()

However errcheck detects this as warning.

[dominikh]

This is a duplicate of of #55 and #77 – Check these two issues for why the warnings are appropriate and won't be removed.

[jeevatkm]

@dominikh I have read those two issues. I can see you explanation but that's not a complete solution to the defer.

I would suggest, please think of providing an option to exclude defer check instead of whitelisting. It is up to user to choose the behavior that suites for their workflow, quality check, etc.

I appreciate your thoughts on error check is important for application but differs between requirements.

For example, I handle this defer warning like this. Because think needs to closed quietly (please note not always).

// CloseQuietly closes `io.Closer` quietly. Very handy and helpful for code
// quality too.
func CloseQuietly(v interface{}) {
    if d, ok := v.(io.Closer); ok {
        _ = d.Close()
    }
}

[dominikh]

Providing a generic "don't check defers" mode isn't helpful. To the contrary, it is harmful, because it will ignore the kind of real errors that I explained in the linked issues. You're still encouraged to ignore specific functions from specific packages, not defer as a whole.

And if you do insist on ignoring defers, you can already do so with basic UNIX tools: errcheck | grep -v "defer ".

[jeevatkm]

Thanks for your response. I agree, it is not good that's why I have requested for options.

please think of providing an option to exclude defer check instead of whitelisting

One possible option is giving something like on end of line or above the line. Then errcheck will not ignore that line.

// errcheck ignore

above one is just a suggestion, please come up with something!

[dominikh]

Littering code with comments to tell linters what to do is also not an option (there are existing issues on this, too. Search for "pragma" to find them). Can you state what precisely is the issue with the current mechanism of ignoring certain functions, via the -ignore flag?

[jeevatkm]

Currently I don't ignore anything from errcheck. Only thing I couldn't do ignore certain lines. I'm unable to achieve it -ignore & ignorepkg option.

Because ignore helps to exclude in (I don't want to exclude entire package, entire file and globally, I want errcheck everything except certain lines)-

• ignore certain package or regex with-in package
• ignore globally with regex

As you stated before in the conversion, second bullet point is same as of whitelist-ing globally.

[dominikh]

It doesn't make sense to "ignore certain lines". Either a function's error should be checked, or it shouldn't. Whether the function is deferred or not doesn't change whether its error is meaningful or not. And the -ignore flag allows you to ignore the specific functions that don't need their errors checked.

[jeevatkm]

I'm sorry, not sure what you mean?

It doesn't make sense to "ignore certain lines"

I hope you know about code quality tool sonarqube, it supports many programming language for code quality check. It does provide option like // NOSONAR at the end of line for exclusion.

[dominikh]

I hope you know about code quality tool sonarqube, it supports many programming language for code quality check. It does provide option like // NOSONAR at the end of line for exclusion.

That does not change the fact that a) an error is either always or never worth checking. That means you don't need to ignore lines, you need to ignore functions b) we (the Go community) object to the proliferation of pragmas.

[jeevatkm]

I'm completely inline with b) we (the Go community) object to the proliferation of pragmas. 👍

do you mean exclude Close function?

[dominikh]

Ignore the Close function of the specific type in the specific package of which the error return does not need to be checked.

[kisielk]

I agree that this has already been covered. Your example in the ticket is a good reason why defer should not be ignored. If file.Close() returns an error it could mean that the contents couldn't be flushed to disk and the file is corrupted, etc. That's something a program may care about. Hence errcheck will warn about this since it's a detail you potentially missed.

I think explicitly checking and ignoring he error by assigning it to blank and leaving a comment as to why it's not being checked, or adding it to the ignores for checks in the project, is a good way of documenting that you don't actually care about this for a particular case.

I'm definitely against adding any kind of in-code pragmas for errcheck. Tools fall in and out of favour all the time and I don't think it's a good thing to litter code with comments to accommodate them.

[jeevatkm]

What I have been doing is. I leave a comment on those lines and calling function I mentioned in the comment.

[kisielk]

Yeah, that works.. so no change required here.

[k3a]

Yes, please consider providing an option to disable error check in defer calls. It is 100% correct to open a file read-only and defer-Close it. The same goes for closing receiving streams. Many example and library go codes use defer x.Close and we all don't want to use CloseQuietly() just to shut the the linter up.

[amnonbc]

Yes, please consider providing an option to disable error check in defer calls. It is 100% correct to open a file read-only and defer-Close it. The same goes for closing receiving streams. Many example and library go codes use defer x.Close and we all don't want to use CloseQuietly() just to shut the the linter up.

@k3a I have added this in my forked version of errcheck https://github.com/amnonbc/errcheck

Enjoy!

[thedenisnikulin]

it's just so stupid this linter doesn't support disabling defer check. Like, just because of this one tiny thing, I have to sit here wasting my time to find the least ugly way to bypass this

[maguro]

it's just so stupid this linter doesn't support disabling defer check. Like, just because of this one tiny thing, I have to sit here wasting my time to find the least ugly way to bypass this

It's open source. Fork the code and bend it to your liking.

[amnonbc]

it's just so stupid this linter doesn't support disabling defer check. Like, just because of this one tiny thing, I have to sit here wasting my time to find the least ugly way to bypass this

It's open source. Fork the code and bend it to your liking.

I did https://github.com/amnonbc/errcheck