dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.05k stars 4.04k forks source link

Code cleanup rules are way too aggressive #66905

Open vsfeedback opened 1 year ago

vsfeedback commented 1 year ago

This issue has been moved from a ticket on Developer Community.


[severity:It's more difficult to complete my work] [regression] While there have come several new code cleanup rules, they are too aggressive compared to what's configured.

As an example we're not requiring conditional expressions on return, so we have configured it as silent in our EditorConfig files, however the Apply conditional expression preferences fixer will apply conditional expressions on all of our returns, no matter what.

So the code

if (response == null)
{
    throw new ArgumentNullException(nameof(response));
}

if (successResultFunc == null)
{
    throw new ArgumentNullException(nameof(successResultFunc));
}

return response.State switch
{
    ServiceState.Error => CreateBadRequestResult(response),
    ServiceState.Ok => successResultFunc(response.Result),
    ServiceState.NotFound => new NotFoundResult(),
    _ => throw new ArgumentOutOfRangeException(nameof(response), response.State, $"{nameof(response)}.{nameof(response.State)} is not valid."),
};

becomes

return response == null
    ? throw new ArgumentNullException(nameof(response))
    : successResultFunc == null
    ? throw new ArgumentNullException(nameof(successResultFunc))
    : response.State switch
    {
        ServiceState.Error => CreateBadRequestResult(response),
        ServiceState.Ok => successResultFunc(response.Result),
        ServiceState.NotFound => new NotFoundResult(),
        _ => throw new ArgumentOutOfRangeException(nameof(response), response.State, $"{nameof(response)}.{nameof(response.State)} is not valid."),
    };

which is very unreadable. Conditional expressions are great in some circumstances but terrible in other (like this).

This issue is not confined to just this fixer. The problem applies to almost all fixers, they're ignoring the severity. In addition, both the Fix all warnings and errors set in EditorConfig and Fix analyzer warnings and errors in EditorConfig causes the same problem as well, even though their names say explicit that they should take care of warnings/errors they're making havoc for suggestion/silent configurations as well.

Expected solution:

  1. There should be possible to configure for all fixers the minimum level of severity that should trigger the rule (both on global level and per fixer)
  2. The EditorConfig fixers should honor their description; only apply Error and Warning issues.

Original Comments

Feedback Bot on 11/25/2022, 00:57 AM:

(private comment, text removed)

Feedback Bot on 11/25/2022, 06:32 AM:

(private comment, text removed)


Original Solutions

(no solutions)

parched commented 1 year ago

At the very least the text "Fix all warnings and errors set in EditorConfig" could be changed to "Fix all silent, suggestions, warnings and errors set in EditorConfig".

But I'd much prefer it did only apply to warnings and errors.

Or even better, there should be a way exclude any fixes that delete code, then we could leave these as warnings in the IDE but we can still enforce them during CI.

This what I have in our editorconfig, I thought it would stop VS from deleting our code, but one of my team mates got a big surprise when he saved one file he was writing with lots of strings and they were all deleted because he hadn't yet written the code that used them.

# IDE0051: Remove unused private member
dotnet_diagnostic.IDE0051.severity = suggestion  # prevent auto save format from removing code

# IDE0058: Remove unnecessary expression value
# IDE0059: Remove unnecessary expression value
# We don't want this automatically fixed as it can mask errors
csharp_style_unused_value_expression_statement_preference = discard_variable:suggestion
csharp_style_unused_value_assignment_preference = discard_variable:suggestion
dotnet_diagnostic.CS0219.severity = suggestion  # prevent auto save format from removing code
noSet commented 10 months ago

Or change to the following two configurations:

parched commented 2 months ago

Duplicate of https://github.com/dotnet/roslyn/issues/66045?