dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

Enforce that enum values are monotonically increasing #63926

Open RikkiGibson opened 2 years ago

RikkiGibson commented 2 years ago

Describe the problem you are trying to solve

In Roslyn, we often add new entries to ErrorCode in feature branches. When the feature branches merge, there are issues with duplicates, which are caught already by CA1069.

Even when there aren't duplicates, though, we can have issues with misordering of the error codes:

namespace Microsoft.CodeAnalysis.CSharp;

public enum ErrorCode
{
    // ...
    ERR_CannotUseRefInUnmanagedCallersOnly = 8977,

    ERR_IncorrectNullCheckSyntax = 8990,
    ERR_MustNullCheckInImplementation = 8991,
    ERR_NonNullableValueTypeIsNullChecked = 8992,
    WRN_NullCheckedHasDefaultNull = 8993,
    ERR_NullCheckingOnByRefParameter = 8994,
    WRN_NullCheckingOnNullableType = 8995,

    #endregion

    #region diagnostics introduced for C# 11.0

    ERR_CannotBeMadeNullable = 8978,
    // ...
}

https://github.com/dotnet/roslyn/blob/d47bc385061323c9d1350e1a4de1cb7a256759e1/src/Compilers/CSharp/Portable/Errors/ErrorCode.cs#L2008-L2021

Here, a feature branch was merged and included some new entries in the "middle" of the sequence. This makes navigating and interpreting the error code list more difficult, so we'd like to avoid it.

I think this analyzer would have to be opt-in, similar to CA1069, since many enums intentionally do not adhere to such ordering.

Describe suggestions on how to achieve the rule

For flags enums, it's not clear when/if this rule should be enforced. Perhaps you could identify when a field initializer consists only of references to other enum fields and exclude it from the rule.

dotnet-issue-labeler[bot] commented 2 years ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-meta See info in area-owners.md if you want to be subscribed.

Issue Details
### Describe the problem you are trying to solve In Roslyn, we often add new entries to ErrorCode in feature branches. When the feature branches merge, there are issues with duplicates, which are caught already by [CA1069](https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1069). Even when there aren't duplicates, though, we can have issues with misordering of the error codes: ```cs namespace Microsoft.CodeAnalysis.CSharp; public enum ErrorCode { // ... ERR_CannotUseRefInUnmanagedCallersOnly = 8977, ERR_IncorrectNullCheckSyntax = 8990, ERR_MustNullCheckInImplementation = 8991, ERR_NonNullableValueTypeIsNullChecked = 8992, WRN_NullCheckedHasDefaultNull = 8993, ERR_NullCheckingOnByRefParameter = 8994, WRN_NullCheckingOnNullableType = 8995, #endregion #region diagnostics introduced for C# 11.0 ERR_CannotBeMadeNullable = 8978, // ... } ``` https://github.com/dotnet/roslyn/blob/d47bc385061323c9d1350e1a4de1cb7a256759e1/src/Compilers/CSharp/Portable/Errors/ErrorCode.cs#L2008-L2021 Here, a feature branch was merged and included some new entries in the "middle" of the sequence. This makes navigating and interpreting the error code list more difficult, so we'd like to avoid it. I think this analyzer would have to be opt-in, similar to CA1069, since many enums intentionally do not adhere to such ordering. ### Describe suggestions on how to achieve the rule - If an enum field is smaller than its immediately preceding field (if another field precedes it), report a diagnostic. - Code fix would move the violating field to appear after another field in the enum which is the closest possible in value without being larger. In the above example, `ERR_CannotBeMadeNullable` would move to appear directly after `ERR_CannotUseRefInUnmanagedCallersOnly`. For flags enums, it's not clear when/if this rule should be enforced. Perhaps you could identify when a field initializer consists only of references to other enum fields and exclude it from the rule.
Author: RikkiGibson
Assignees: -
Labels: `area-Meta`, `untriaged`, `code-analyzer`
Milestone: -
ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

Issue Details
### Describe the problem you are trying to solve In Roslyn, we often add new entries to ErrorCode in feature branches. When the feature branches merge, there are issues with duplicates, which are caught already by [CA1069](https://docs.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1069). Even when there aren't duplicates, though, we can have issues with misordering of the error codes: ```cs namespace Microsoft.CodeAnalysis.CSharp; public enum ErrorCode { // ... ERR_CannotUseRefInUnmanagedCallersOnly = 8977, ERR_IncorrectNullCheckSyntax = 8990, ERR_MustNullCheckInImplementation = 8991, ERR_NonNullableValueTypeIsNullChecked = 8992, WRN_NullCheckedHasDefaultNull = 8993, ERR_NullCheckingOnByRefParameter = 8994, WRN_NullCheckingOnNullableType = 8995, #endregion #region diagnostics introduced for C# 11.0 ERR_CannotBeMadeNullable = 8978, // ... } ``` https://github.com/dotnet/roslyn/blob/d47bc385061323c9d1350e1a4de1cb7a256759e1/src/Compilers/CSharp/Portable/Errors/ErrorCode.cs#L2008-L2021 Here, a feature branch was merged and included some new entries in the "middle" of the sequence. This makes navigating and interpreting the error code list more difficult, so we'd like to avoid it. I think this analyzer would have to be opt-in, similar to CA1069, since many enums intentionally do not adhere to such ordering. ### Describe suggestions on how to achieve the rule - If an enum field is smaller than its immediately preceding field (if another field precedes it), report a diagnostic. - Code fix would move the violating field to appear after another field in the enum which is the closest possible in value without being larger. In the above example, `ERR_CannotBeMadeNullable` would move to appear directly after `ERR_CannotUseRefInUnmanagedCallersOnly`. For flags enums, it's not clear when/if this rule should be enforced. Perhaps you could identify when a field initializer consists only of references to other enum fields and exclude it from the rule.
Author: RikkiGibson
Assignees: -
Labels: `area-System.Runtime`, `untriaged`, `code-analyzer`
Milestone: -