dotnet / dotnet/format

Automatic fix for IDE0072 is changing code behaviour

Open
#2,219 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
No language data
Stars
1.9k
Forks
173
Avg merge
10d 13h
Merged PRs (30d)
1

Description

I might be in a minority for that, but i think `dotnet format` should not populate missing switch branches with `throw new NotImplementedException()`. Scenario:

I had this code in codebase:
```csharp
query = tasksSortBy switch
{
TasksSortBy.Name => query.OrderBy(e => e.Name),
TasksSortBy.Completed => query.OrderBy(e => e.Completed),
_ => query.OrderBy(e => e.CreatedAt)
};
```

Even though there is TasksSortBy.Unspecified - this code is just fine, since we have the wildcard `_` in the place to handle it. But i still want to keep the diagnostic to avoid missing enum values that are "real". So the diagnostic is fine, maybe Unspecified is too much (but it's a good practise from gRPC protocol, so we are using it there). My issue is with the automatic fix. After running the `dotnet format` i ended up with:

```csharp
query = tasksSortBy switch
{
TasksSortBy.Name => query.OrderBy(e => e.Name),
TasksSortBy.Completed => query.OrderBy(e => e.Completed),
TasksSortBy.Unspecified => throw new NotImplementedExcetion(),
_ => query.OrderBy(e => e.CreatedAt)
};
```

which for me it is outright dangerous. Fortunately for us - the integration test suite quickly caught the issue in CI pipeline, but the developer did not check the code after running the `dotnet format` (which is understandable, it should be just format). In this case `format` command changed the code behaviour. I'm opening this thread to figure out if more people think like me here :).

To be clear - the "solution" isn't that clear either, but from my perspective it would be better if `dotnet format` left the code "uncompilable" at this point. So:

```csharp
query = tasksSortBy switch
{
TasksSortBy.Name => query.OrderBy(e => e.Name),
TasksSortBy.Completed => query.OrderBy(e => e.Completed),
TasksSortBy.Unspecified => // TODO : implement that
_ => query.OrderBy(e => e.CreatedAt)
};
```

is much better, since it's forcing the developer to implement the missing branch. What do you all think about that?

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.