Do not return nonexistent kinds in SyntaxFacts' methods

[DoctorKrolic]

Fixes: #72300

[AlekseyTs]

Do not return unexisting kinds in SyntaxFacts' methods

Could you please elaborate what the problem is and open an issue that describes a user-facing problem? Even though there is no member in the enum, the number is still reserved. And

[DoctorKrolic]

Could you please elaborate what the problem is

I was working on a fix for #72258. I decided to populate all C# keywords' texts upfront, so I can later return them as completions. In order to achieve that I used SyntaxFacts.GetKeywordKinds() method and then saved keyword text by calling SyntaxFacts.GetText(kind) on each kind. When it came to verifying my work I noticed that there was 1 completion with empty text. A quick debug session showed that GetKeywordKinds has unexisting kind 8441 as part of returned collection of kinds. This led to this PR being opened. Then Youssef suggested adding guarding assert and I added such for all methods with similar integer looping logic. Failed CI then showed that SyntaxFacts.GetPunctuationKinds() also returns unexisting kinds and a lot of them. After fixing it I thought that corresponding SyntaxFacts.IsXXX checks should also not assume anything about unexisting kinds. SyntaxFacts.IsKeywordKind() already specifies all contextual keywords 1 by 1, so it didn't need fixing. SyntaxFacts.IsPunctuation() on the other hand did a range check, so I modified it to exclude unexisting kinds there. Now we are at this state.

open an issue that describes a user-facing problem

Even though there is no member in the enum, the number is still reserved

It is perfectly fine to reserve number ranges for different kinds of enum values. I would say, it is also fine to return unexisting kinds in an internal API, since this fact can then be shared as a common knowledge in the team, so additional checks can be made on caller side if necessary. However, this is public API and I can say with a very high level of certainty that consumers of public API by default assume that if API returns an enum value, this is an actual existing value of that enum. So I would classify this as a bug regardless of the user scenario it was discovered in (and I provided the way I personally discovered it)

[AlekseyTs]

So I would classify this as a bug regardless of the user scenario it was discovered in (and I provided the way I personally discovered it)

I am afraid we still need an issue, especially as you classify the behavior as a bug.

[DoctorKrolic]

I am afraid we still need an issue, especially as you classify the behavior as a bug.

Filled #72300 and #72301. I'll put this PR into a draft state until both issues are discussed and approved as actual bugs. Personally I feel very strongly about the first one being a bug and less strongly about the second one (but it is still not normal behavior IMO)

[DoctorKrolic]

@AlekseyTs #72300 has been classified as a bug. I've reverted other changes in this PR, so it only fixes that issue. This is now ready for review

[AlekseyTs]

Done with review pass (commit 6)

[AlekseyTs]

Done with review pass (commit 8)

[AlekseyTs]

Done with review pass (commit 9)

[AlekseyTs]

Done with review pass (commit 10)

[DoctorKrolic]

The [SyntaxKind.ElifKeyword - SyntaxKind.RestoreKeyword] range already contains SyntaxKind.HiddenKeyword, so I removed the duplicate. Found while working on tests

[AlekseyTs]

LGTM (commit 11), assuming CI is passing.

[AlekseyTs]

@dotnet/roslyn-compiler For the second review.

[jcouv]

LGTM Thanks (iteration 11)

[jcouv]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[jcouv]

Merged/squashed. Thanks for the contribution!