staticcheck: Add per file (or generated) ignores

[klauspost]

I would like to add ignores similar to "File-based linter directives", but in the configuration or in another file.

Background: I am dealing with generated files that violate a check (SA4003) in rare cases. When using the tool with go generate I don't have a reasonable (not hacky, or requiring a dedicated command installed) way to add a //lint:file-ignore SA4003 to the file.

For now, I have to disable the code check for my entire code base. I may be missing an obvious way to do this, but if not, it would help a lot to have that option.

[ainar-g]

Do ed and sed fall under “dedicated command installed”? They are installed on pretty much any unix-like system.

[klauspost]

@ainar-g that would be somewhere between hack and dedicated command. The point is that it would be additional complexity in our setup, and adding platform restrictions for a rather small thing.

[ainar-g]

I used to use this hack with errcheck back in the days:

$ errcheck | grep -v badfile.go && exit 1 || exit 0

grep will exit with 0 if it finds any lines that do not contain badfile.go, so in this case we must exit with 1, since that means that we have errors outside of badfile.go, otherwise grep hasn't found anything outside of badfile.go, and so we exit cleanly.

This is still a hack, but it doesn't require any modification of the generated file itself.

[dominikh]

I feel your pain, but I'd strongly recommend fixing the generator instead, either by not emitting the comparison, or (more likely) to emit the lint:file-ignore directive.

There is the -ignore command line flag, but it has been deprecated. See #386 for the discussion on that.

[temoto]

First, thanks for great tool.

As you may know, stringer can generate String() methods like this

func (i Field) String() string {
	switch {
	case 0 <= i && i <= 3:
		return _Field_name_0[_Field_index_0[i]:_Field_index_0[i+1]]
//...
}

In this case my type is alias to byte and staticcheck likely wants it to become case i <= 3.

stringer + unsigned type feels like big enough special case. It would be far worse if Go didn't passive-aggressively push signed int everywhere through ugly casting from essentials, like len(). (sorry for redundant text, I hate using inappropriate types)

On the constructive side, can we have something like following please?

# staticcheck.conf
file_ignore = ["subpak/*_string.go SA4003"]

That is:

• acknowledge issue locality to subset of files
• but "inject ignore lines" without waiting for generator to agree or update their template
• while avoiding problems of post-modification generated code with sed

[narqo]

There is the -ignore command line flag, but it has been deprecated.

It seems -ignore is no longer available for 2019.2. Which is quite unfortunate (and somewhere breaking).

As for now the ignore for generated files forces a project to choose between having a linter or keep using a generator (whose maintainers might not be interested in adding some random, non-standard meta comments to generated files)

[Invertisment]

How on the world you expect to force every maintainer of every generator ever to obey this stupid rule?
What if they haven't ever heard of staticcheck and they use their own linting solution? What if the library's maintainers are dead? What if I want to use an older version with a special type of a bug? It's nuts.

What if I'd write my own linter and force everyone to add a comment to all generated files in the world?
Something of this this sort:
// I am stupid because I have to add this to all of my files

This is quite sad to read. The shortsightedness and stupidity.
Who can give me this power? I would like to add a disclaimer to every source file so that everyone would know that I touched or didn't touch that code. I don't understand any logic behind your statement about removing ignore functionality.

How is having a config file for each linter different than forcing all library maintainers to change their habits and code? I think config file is better, it's easier to migrate if you mess up the comment format. The push should be soft and voluntary and not hard, as we see here.

It's not even about the command line flag. Even if it's deprecated, so be it. You could at least migrate the ignoring capabilities to the config file. These comments about unusefulness are waste of everyone's time. Including yours.

Summary:

• Config file is better than having everyone agree. Config files can be migrated, all generators in the world are too hard to change if you suddenly change your mind.
• It's impossible to force all generator builders to agree on a comment format.
• ignore functionality (forget the flag) shouldn't have been removed because even if it was a "bug" it became a useful feature to users.

[klauspost]

@Invertisment - While I think you could/should be a bit more diplomatic, but largely I agree with your points.

Remember, however, this is @dominikh project, and he can make the decisions he wants. To me, adding this functionality is a perfectly pragmatic solution. Someone may have the time for fixing it upstream, but I don't. So I can't use this tool.

[Invertisment]

You are right, it completely depends on the author.

Also I didn't consider a different usecase:
The files or filenames could be piped through stdin.Then the caller will handle any file filtering he needs.
I think piping currently is not supported.

[dominikh]

Your rant refers to both "generator" and "library", and it's hard for me to tell if these two are being conflated, or if you're ranting about two different things. I'll assume you're talking about two different issues and will respond as such.

How on the world you expect to force every maintainer of every generator ever to obey this stupid rule?

I expect the majority of code generators to fall into one of two categories:

1. Does not trigger any staticcheck warnings. We already ignore all stylistic issues and simplifications in generated code. We only flag code that that is likely to be buggy. The majority of generators don't trigger any false positives.

2. Generates entire packages, not single files. If the generator creates an entire package, then it's trivial to add a staticcheck.conf file to that package and disable the offending checks.

It's also rather simple to pipe the output of a code generator through a post-processing step that adds the required ignore directives to the file. A simple combination of cat and echo will do.

What if the library's maintainers are dead?

Good news! You get to be the new maintainer.

What if I want to use an older version with a special type of a bug?

This seems like an overly specific and peculiar scenario. It's assuming that a) you want to use an older version b) the older version doesn't already have the necessary directives c) the older version triggers a warning by staticcheck.

This seems extremely unlikely. If it ever does occur, then the solutions I outlined above apply.

I don't understand any logic behind your statement about removing ignore functionality.

That is unfortunate. I think I've been rather clear about how the -ignore flag isn't maintainable, and how there are other,better ways of ignoring issues.

How is having a config file for each linter different than forcing all library maintainers to change their habits and code?

Library authors are free not to use staticcheck. If they do decide to use staticcheck, they are clearly okay with its requirements.

Them not using staticcheck (and in turn our linter directives) doesn't affect the users of said libraries, unless they're linting their dependencies. If you are linting your dependencies you implicitly expect them to conform to your project's standards. If they don't, it's on you to fix that.

In my opinion, it should be obvious that using -ignore on code in dependencies is even less viable than using it on code you own. Dependencies are much too prone to change, and using -ignore risks missing new, genuine bugs.

You could at least migrate the ignoring capabilities to the config file.

You can already disable checks on a per-subtree basis by using configuration files.

It's impossible to force all generator builders to agree on a comment format.

While it's not easy, it's by no means impossible. There is a limited number of successful linters, and a limited number of well-designed linter directives. We have proposed our format, and it's my hope that other linters will follow suit. The only other relevant format that currently exists is nolint, which IMO is inferior to our format.

As Go's tooling ecosystem evolves, and tools like gopls become more commonplace, we'll definitely have to agree on a way of ignoring problems. If we come up with a better format in the process, staticcheck will support it.

It's nuts.
I am stupid because I have to add this to all of my files
The shortsightedness and stupidity.
These comments about unusefulness are waste of everyone's time. Including yours.

You're welcome to take part in a civilised discussion. You're not welcome to come to my project and insult me. Rants like these are a much larger waste of my time than having a proper discussion, especially if they disregard all arguments already made.

If you don't like the way I run my project, then you're free not to use it. You can even tell your friends how much I suck. But I won't be insulted in my own home. Please take that into consideration should you wish to provide input in the future.

It's my goal to write a tool that is both useful and matches my ideas on maintainability. Arguments can change my mind on certain things, being hostile can't.

[narqo]

Does not trigger any staticcheck warnings. We already ignore all stylistic issues and simplifications in generated code. We only flag code that is likely to be buggy. The majority of generators don't trigger any false positives.

Do you have somewhere a description of different categories of checks?

In our project, for example, we ignore the following staticcheck's failures (with staticcheck | grep -v _easyjson.go hacks):

data_easyjson.go:57:32: &*x will be simplified to x. It will not copy x. (SA4001)
data_easyjson.go:1049:3: this value of key is never used (SA4006)

I'd say the both checks should be treated as "simplifications", taking into account the second one is triggered by the following generated code:

key := in.UnsafeString()
switch key {
default:
	in.SkipRecursive()
}

Obviously, these both cases can (and likely should) be improved. My question is about priorities: do I have to postpone staticcheck integration into our 300+K lines of non-vendored-code project, fork all generators, fix them, wait for approvals from maintainers, schedule the update, move forward with staticcheck. Or should I integrate the tool now, making it save man-hours for code-reviews and go with other issues?

[dominikh]

Do you have somewhere a description of different categories of checks?

http://staticcheck.dev/docs/checks has a list of all checks, and descriptive headers for the categories. I've copied them out for your convenience:

S1???	Code simplifications
SA1???	Various misuses of the standard library
SA2???	Concurrency issues
SA3???	Testing issues
SA4???	Code that isn't really doing anything
SA5???	Correctness issues
SA6???	Performance issues
SA9???	Dubious code constructs that have a high probability of being wrong
ST1???	Style violations

They can be summarized as S for simplifications, SA for correctness, ST for style (and U for unused identifiers).

I'd say the both checks should be treated as "simplifications", taking into account the second one is triggered by the following generated code:

&*x is definitely more of a bug than a simplification. Manually written code may incorrectly use the pattern, expecting it to create a copy. It's also weird that easyjson is generating it, IMO, and there is an open (albeit thoroughly ignored by upstream) issue at mailru/easyjson#167

The fact that we flag the switch statement you demonstrated is a false noisy positive that we need to fix (this is a known issue, though we neglected to actually file it #533.). It, too, is not generally a simplification. Its aim is to catch code such as the following:

foo, err := fn()
bar, err := fn()
if err != nil { ... }

My question is about priorities

I couldn't deduce whether this is a rhetorical question.

[mvdan]

We ran into this issue at work when we started enforcing staticcheck on more of our codebase. One of the generators was unmaintained, and generated capitalized errors. We fixed it by switching to a fork of the generator that had been updated to fix those errors.

I generally agree with the current approach. If the generated code we use has any potential bugs, I want to know about it.

It also occurs to me that it should be possible to add lint:file-ignore comments to generated files as necessary. For example:

//go:generate generator -o=out.go
//go:generate sed -i '1i //lint:file-ignore\n' out.go

This example uses sed and needs to know what files to modify. For more complex use cases one could write a simple script or Go program to add the necessary lines. That's certainly a bit of a hassle, so hopefully most developers will instead try to fix the generators :)

[klauspost]

Ran into this issue again. With ST1016 (methods on the same type should have the same receiver name) on generated code.

Unless something has been added, again, I am without many options other than globally disable this check. I don't control the generator, and honestly I wouldn't expect the author to fix this.

I don't want to hack in sed like above since it will restrict development platforms.

Needless to say this is frustrating.

[klauspost]

Since we already have a config file, why can't we be given the option to add it like @temoto proposes above?

[dominikh]

Beginning with Staticcheck 2020.1, ST1016 no longer considers receivers in generated code. That seems preferable to everyone having to add their own ignores.

As for temato's option: it suffers from most of the same problems that the -ignore flag did, which we removed for a reason. This is especially true when we support globs.

[klauspost]

Cool. I will try the update 👍

[klauspost]

Well, re my issue, now I'm left with more generated files problems:

Click to expand!

λ staticcheck ./...
browser\ui-assets.go:227:5: should not use underscores in Go names; var _productionIndex_bundle20200220t144345zJs should be _productionIndexBundle20200220t144345zJs (ST1003)
browser\ui-assets.go:242:132273: string literal contains Unicode format and control characters, consider using escape sequences instead (ST1018)
browser\ui-assets.go:242:258794: string literal contains the Unicode format character U+180E, consider using the '\u180e' escape sequence instead (ST1018)
browser\ui-assets.go:291:6: should not use underscores in Go names; func productionIndex_bundle20200220t144345zJsBytes should be productionIndexBundle20200220t144345zJsBytes (ST1003)
browser\ui-assets.go:295:6: should not use underscores in Go names; func productionIndex_bundle20200220t144345zJs should be productionIndexBundle20200220t144345zJs (ST1003)
browser\ui-assets.go:306:5: var _productionLoaderCss should be _productionLoaderCSS (ST1003)
browser\ui-assets.go:406:6: func productionLoaderCssBytes should be productionLoaderCSSBytes (ST1003)
browser\ui-assets.go:410:6: func productionLoaderCss should be productionLoaderCSS (ST1003)

Again, unable to skip it. As for your reasons, you say yourself that:

Alternately, if said code exists in isolated packages, then configuration files can be used instead.

So I can...

1. Disable these checks for the entire repo, or
2. .... ?

The configuration exists in a repo. The code referred to is in the same repo. The code does not live in isolation. I hope you will reconsider this.

[gordallott]

Hi,

I'm probably going to drop staticcheck from our repo because of this. We have some extremely large generated files that we can't stop staticcheck from trying to parse, It manages to eat up north of 64GB of ram on my machine doing so.

The inability to tell staticcheck to not look at a package simply means that staticcheck does not scale to large projects with large demands. No one expects staticcheck to do with with 100mb+ go files, but we all expect to be able to control the behaviour of our tooling in simple and easy ways

staticcheck fails in this reguard. Comment ignores are worthless because they come after a parse that has taken several minutes to happen.

[dominikh]

@gordallott That is a very valid issue, but it is different from the one discussed here. Even if #429 were addressed, staticcheck would still need to parse, type-check and analyze the package, to deduce facts about it, to use these facts when analyzing those packages that depend on the large asset package. Ignores, however, are processed after the fact. They are merely used to reduce the list of problems found after we're done analyzing everything.

Instead, we need a mechanism that skips packages entirely, and treats them as black boxes, only relying on the type information exposed by the compiler's export data. This will introduce some inaccuracy when analyzing dependents, and we must carefully evaluate whether this will lead to any false positives. The likelihood of that is low, however. Asset packages are unlikely to contain functions that act on control flow or otherwise modify the behavior of a program, other than providing binary data. Note, however, that we cannot do any better than this. We cannot ignore a package completely (that is, not even load its type information from export data), unless we also ignore all packages that depend on the package, recursively. Doing so seems undesirable to me.

I don't think, however, that handling this should require user intervention, i.e. an explicit list of packages to skip. We're well able to detect the size of files and skip packages that are absurdly large. We can also detect patterns common to asset generation without having to parse the file. I would rather focus my time on handling these cases automatically, than introduce a new knob that people need to discover first.

I will file a separate issue for this soon, for further discussion.

[dominikh]

@klauspost Configuration files apply to trees of packages; a single repository can contain more than one configuration file. Consider this structure:

.
├── pkg1
├── pkg2
├── pkg3
│   └── assets
│       └── staticcheck.conf
└── staticcheck.conf

You can add a configuration file to the asset package and disable the undesired checks. For more information on configuration files, their tree structure, and how to inherit settings from higher up the tree, see https://staticcheck.dev/docs/#configuration

[klauspost]

@dominikh You are assuming that projects use a certain structure. There are a lot of existing projects where a huge chunk of code is in a single package. The receiver issue was in the package above.

While we can agree that isn't usually a good structure for projects I have seen it many times both in public and non-public codebases.

So you are proposing 2) Export generated code and move to a separate package?

IMO having to move and export generated code to separate packages just because you cannot tweak which files are picked up or which checks are applied on a file by staticcheck seems like a suboptimal solution.

[mfridman]

We ran into this issue following an update of our codebase to the new Go protobuf API.

Problem is much of the tooling/ecosystem will take (considerable) time to catch up. So if a project references the old Go protobuf API (which is still backwards compat, and this is important!) staticcheck gives warnings.

An example is Twirp. For every generated *.twirp.go staticcheck nudges an upgrade:

foo.twirp.go:25:8: package github.com/golang/protobuf/jsonpb is deprecated: Use the "google.golang.org/protobuf/encoding/protojson" package instead.  (SA1019)
foo.twirp.go:26:8: package github.com/golang/protobuf/proto is deprecated: Use the "google.golang.org/protobuf/proto" package instead.  (SA1019)

Adding a bunch of staticcheck.conf to the codebase isn't ideal. I've often seen scripts/tooling that blows away the entire directory and adds the generated files. So now we need to think about the longevity of staticcheck.conf..

Hope you'll reconsider.

[peterbourgon]

Just want to add a voice of support for @dominikh in this discussion. All IMO — it's important that linters are not configurable, and that flagging exceptions incurs some costs. The tool loses its efficacy when it's easy to opt out of its findings. If your org or project has systemic opt-out needs that are too large for per-incident //lint:ignore directives, and somehow can't be segregated into packages that the tool can skip over, then forking the tool and modifying it for your specific environment seems like a perfectly appropriate option.