Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lintcmd: make -check case-insensitive, error on unknown checks #1546

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented May 26, 2024

This changes it so that the check is case-insensitive, and so it will return an error on unknown checks. Previously:

% staticcheck -checks=sa1000 ./lintcmd
[no output, exit 0]

% staticcheck -checks=banana ./lintcmd
[no output, exit 0]

% staticcheck -checks=SA1000 ./lintcmd
lintcmd/cmd.go:270:34: error parsing regexp: missing closing ): `$^(` (SA1000)

This is massively confusing! When I was doing my testing for the other PRs yesterday I found it odd that I didn't get any error for the defer one, since I knew there was at least one module with this error. Turned out it just always passed because I used lower-case.

I wouldn't be surprised if tons of people have made this mistake and have run (or are running) staticcheck runs that don't actually do anything.

All of this also applies to checks = [..] from the config file.

This changes it so that the check is case-insensitive, and so it will
return an error on unknown checks. Previously:

	% staticcheck -checks=sa1000 ./lintcmd
	[no output, exit 0]

	% staticcheck -checks=banana ./lintcmd
	[no output, exit 0]

	% staticcheck -checks=SA1000 ./lintcmd
	lintcmd/cmd.go:270:34: error parsing regexp: missing closing ): `$^(` (SA1000)

This is massively confusing! When I was doing my testing for the other
PRs yesterday I found it odd that I didn't get any error for the defer
one, since I *knew* there was at least one module with this error.
Turned out it just always passed because I used lower-case.

I wouldn't be surprised if tons of people have made this mistake and
have run (or are running) staticcheck runs that don't actually do
anything.

All of this also applies to checks = [..] from the config file.
@dominikh
Copy link
Owner

and so it will return an error on unknown checks

We intentionally don't complain on unknown checks, at least in the staticcheck.conf case. This is both for backwards and forwards compatibility, as well as to accommodate projects that use custom builds of Staticcheck that include custom checks. In neither case do we want to prevent people with other versions of Staticcheck from running Staticcheck. And I'd like to keep -checks and the config behaving the same, to avoid more confusion.

Maybe as a compromise we can emit diagnostics for unknown checks?

This changes it so that the check is case-insensitive

I can see the appeal in that, but for consistency we'd also have to do this for //lint:ignore. Then we'd probably also have to update gopls to interpret its analyses setting, and after all that we'd still be left with case-sensitive anchors on our website, as well as other analysis runners that aren't case-insensitive.

I think I'd prefer it if we stuck to only one way of spelling check names (i.e. the current way), and instead focused on making it more obvious when check names are misspelled.

@arp242
Copy link
Contributor Author

arp242 commented May 27, 2024

Emitting a diagnostic is okay. I'll change that.

Personally I'd strongly prefer case-insensitive checks whenever feasible. To me at least it's much more natural to type "sa1000" rather than "SA1000" (even when knowing about this, I again did it wrong yesterday).

It should probably be case-insensitive in //lint: directives too; I don't mind working on changing that.

We can't make everything 100% case-insensitive, and that's okay; not many people are manually typing anchor tags for the website. If we can do >99% of where people are typing it then that's a win IMO. We can know what's intended with 100% certainty, so it just doesn't make any sense to stand on ceremony and reject it.

This is also why I made -explain case-insensitive in #1544, because I kept typing -explain sa1005, getting and error, and having to re-do it (when I made that PR I thought -checks was case-insensitive, as it didn't error).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants