feat: add errchkjson linter #2349
Conversation
|
@ldez Thanks for finding all the typos. I will rebase all the changes as soon as I am back at my keyboard. |
breml
force-pushed
the
add-errchkjson
branch
2 times, most recently
from
4c161ac to
8dc2b87
Compare
|
@ldez What do you think about this comment on reddit?
My tests suggest, that there is no problem, if the err is omitted with enc := json.NewEncoder(os.Stdout)
enc.Encode(xyz) // error is omittedIs there a way to throw away a report from an other linter (in this case errcheck), because errchkjson is more specific? |
This check is already done by another linter: https://staticcheck.io/docs/checks/#SA9005 |
A linter cannot interact with another linter. |
|
IMO, for the golangci-lint context, your linter must not handle the error and let the other linters, that are specific to this, handle the error. |
This flag is needed to disable this check in order to prevent duplicate check results with staticcheck in golangci-lint. See: * golangci/golangci-lint#2349 (comment) * https://staticcheck.io/docs/checks/#SA9005
I do not agree with this, because Therefore I tried to find a way, how these two linters can co-exist and both perform their job to the best extend possible. The latest commit is the current solution I found so far. |
Prevent redundant checking with staticcheck. See: * golangci#2349 (comment) * https://staticcheck.io/docs/checks/#SA9005
if errchkjson is enabled and omit-safe is not activated. In this case, the check for the errors in errchkjson is superior to the "generic" check in errcheck. Therefore we instruct errcheck to ignore these cases and let errchkjson handle them.
Update pkg/golinters/errchkjson.go Update test/testdata/errchkjson.go Update test/testdata/errchkjson_omit_safe.go Co-authored-by: Ludovic Fernandez <[email protected]>
|
Is there anything missing on this PR? What is the best way to move forward with this? |
| // Modify errcheck config if this linter is enabled and OmitSafe is false | ||
| if isEnabled(linters, a.Name) && !omitSafe { | ||
| if errcheckCfg == nil { | ||
| errcheckCfg = &config.ErrcheckSettings{} | ||
| } | ||
| errcheckCfg.ExcludeFunctions = append(errcheckCfg.ExcludeFunctions, | ||
| "encoding/json.Marshal", | ||
| "encoding/json.MarshalIndent", | ||
| "(*encoding/json.Encoder).Encode", | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
As I already said, a linter cannot interact with another linter.
The fact to create links between 2 linters means that 1 linter can be a part of the other linter.
There was a problem hiding this comment.
@ldez Thanks for having a look at my PR again, I really appreciate it.
As I already said, a linter cannot interact with another linter.
I do not exactly understand what you mean by "cannot". My understanding of "cannot" is, that it is technically not possible. But I don't think this is true, because the implementation of the PR does work as intended.
Or do you mean "cannot" in the sense of "MUST NOT" (or "SHALL NOT") as defined in https://datatracker.ietf.org/doc/html/rfc2119? That is: it is prohibited by guideline (even though, it could technically be possible).
If it is the later, I would like to know, if this guideline is written down somewhere and I would like to better understand the reasoning behind it. I understand, that in some cases the exclude patterns have been used to handle exactly such situations, e.g. for duplicates between gosec and errcheck (see EXC0008).
The fact to create links between 2 linters means that 1 linter can be a part of the other linter.
So what is your recommendation? I can see the following options (in no particular order):
- Add the functionality of errchkjson to errcheck. I do not know, if this special handling is in the scope of errcheck, because it defines its scope pretty general with "... checking for unchecked errors ..." and as far as I can tell, there is currently no special handling based on the input arguments in errcheck. Additionally, errchkjson also performs checks on the input types for the json encoding functions (can a type be encoded) and this is cleary out of scope for errcheck and would have to stay in this linter.
- Extend errchkjson to also include everything that is done by errcheck so the user is free to choose one linter or the other depending of the fact, if he wants to have the special handling of the json encoding functions. In my opinion, this does not make any sense.
- We add the necessary exceptions to the ExcludePatterns in
pkg/config/issues.go, but I am not sure how exactly this would work given the fact, that the exclude pattern should only be added under special circumstances (both linters enabled and omitSafe not true). - Add the errchkjson linter (without adding config settings to errcheck) and mention in the documentation / .golangci-lint.yaml, how the user can add the necessary excludes. This would work, but it is not really user friendly.
- Keep the current implementation.
Add errchkjson.
This linter checks the types, that are passed to json encoding function (e.g.
json.Marshal) and has two main functions:complex128,funcorchan).errorreturned from the json encoding function can be ignored, because there is no way for json.Marshal to have an error.For more details, see the README.md.
(This linter has been laying around on my harddisk for 2 years, after the successful inclusion of bidichk in golangci-lint (#2330) I felt motivated to polish this one and propose it for inclusion as well.)