Add new -default-case-required flag

[aaron-mongodb]

We've been using exhaustive in our work to help validate our switch statements. In reviewing correctness standards we've found it advantageous to require that some switches explicitly have a default case. We'd like to have this feature in our existing switch-linter.

Existing users should see no change of behavior unless if they activate this new switch.

This includes demonstrative tests, including the escape-comment and enforce-comment.

(I see that you ask contributors open an issue first to facilitate discussion for larger changes. If this falls into that category I'm happy to do so; these changes were made in the course of us investigating what we wanted to change. As the code is fairly targeted, and already exists from this investigation, I thought to open the PR first. Please let me know.)

[nishanths]

Thanks for the change. This can be reviewed without creating a separate GitHub issue. I'm happy to merge this. I added a few review comments.

[nishanths]

Can we move -ignore and -enforce from suffix to prefix?

To me, having them as prefixes makes these comments more natural to read
as directives. That is, consider in English:

ignore [something]

versus

[something] ignore

[aaron-mongodb]

I agree that formatting is preferable. I wrote it in this reversed way because "//exhaustive:ignore-default-require(d)" is a prefix of "//exhaustive:ignore", so that induces some significance to the order in which we try to parse directives. (I believe a careful ordering of if statements, like how you suggested for the precedence issue above, should cover it, longest-first). I'm happy to make that change but absent feedback went with no-common-prefix approach.

So if inducing that significance sounds good, let me know and I'll happily make that change. Thanks!

[aaron-mongodb]

I forgot to add: I suppose it is technically backwards-incompatible, because if by some extremely-strange-chance a user already had "//exhaustive:ignore-..." it will no longer do the old behavior. I am OK with that but I'm not the maintainer :), so I should raise that too. (More realistically, this is why I didn't consider renaming the existing "//exhaustive:ignore" directives, so users won't see any change-of-behavior there.)

[nishanths]

Thanks for mentioning and implementing the longest-first ordering. I missed to consider it.

I forgot to add: I suppose it is technically backwards-incompatible, because if by some extremely-strange-chance a user already had "//exhaustive:ignore-..." it will no longer do the old behavior. I am OK with that but I'm not the maintainer :), so I should raise that too.

Thanks for mentioning this, too. I wish that the current code in master was more strict about the requirements here. That is, I wish that the code in master performed the equivalent of a ^//exhaustive:ignore($| |\t) check. Then the following comments would be valid, as desired:

('•' is used to visually represent space below)

//exhaustive:ignore
//exhaustive:ignore•
//exhaustive:ignore•more•text

and the following would be invalid, as desired:

//exhaustive:ignore-more-text

The package doc in master, too, doesn't go into this level of detail. It just says, "a comment that begins with".

Overall, I'm ok with this being a technically breaking change. For the long term, I created issue 70 to consider updating the code in master to have the level of strictness described above, which will be more of a breaking change.

Edit: Updated to reflect the fact that it's not as wide a breaking change as I originally thought. It will only affect users who used exactly a prefix of //exhaustive:ignore-default-case-required as a substitute for //exhaustive:ignore, which should realistically be no-one. Users who used, for example, //exhaustive:ignore-blah will not be affected, though they might be affected by issue 70 mentioned above.

[nishanths]

Just a note, not necessary to update here. To avoid regressions, it would be good to, additionally, programmatically verify this ordering. This could be done, for example, by moving the slice to a top level variable, then adding a unit test like below.

func TestDirectiveComments(t *testing.T) {
	if !orderedLongestFirst(directiveComments) {
		t.Errorf("incorrect order")
	}
}

I can do that in a future commit sometime.

[nishanths]

Thanks for mentioning and implementing the longest-first ordering. I missed to consider it.

I forgot to add: I suppose it is technically backwards-incompatible, because if by some extremely-strange-chance a user already had "//exhaustive:ignore-..." it will no longer do the old behavior. I am OK with that but I'm not the maintainer :), so I should raise that too.

Thanks for mentioning this, too. I wish that the current code in master was more strict about the requirements here. That is, I wish that the code in master performed the equivalent of a ^//exhaustive:ignore($| |\t) check. Then the following comments would be valid, as desired:

('•' is used to visually represent space below)

//exhaustive:ignore
//exhaustive:ignore•
//exhaustive:ignore•more•text

and the following would be invalid, as desired:

//exhaustive:ignore-more-text

The package doc in master, too, doesn't go into this level of detail. It just says, "a comment that begins with".

Overall, I'm ok with this being a technically breaking change. For the long term, I created issue 70 to consider updating the code in master to have the level of strictness described above, which will be more of a breaking change.

Edit: Updated to reflect the fact that it's not as wide a breaking change as I originally thought. It will only affect users who used exactly a prefix of //exhaustive:ignore-default-case-required as a substitute for //exhaustive:ignore, which should realistically be no-one. Users who used, for example, //exhaustive:ignore-blah will not be affected, though they might be affected by issue 70 mentioned above.

[nishanths]

I added a commit that updates the testdata to match the diagnostic message change in 63b22a7.

[nishanths]

Thanks again for the change! I will merge once the CI tests pass, then tag a new version.