Roslyn: IDE0079 should not warn when suppressing IDE0066

[JohanLarsson]

        private static int M(StringComparison stringComparison)
        {
#pragma warning disable IDE0066 // Convert switch statement to expression
            switch (stringComparison)
#pragma warning restore IDE0066 // Convert switch statement to expression
            {
                case StringComparison.CurrentCulture:
                case StringComparison.CurrentCultureIgnoreCase:
                case StringComparison.InvariantCulture:
                    return 1;
                case StringComparison.InvariantCultureIgnoreCase:
                case StringComparison.Ordinal:
                case StringComparison.OrdinalIgnoreCase:
                    return 2;
                default:
                    throw new ArgumentOutOfRangeException(nameof(stringComparison), stringComparison, null);
            }
        }

Adding the suppressing above gives:

Severity    Code    Description Project File    Line    Suppression State
Message IDE0079 Remove unnecessary suppression  App1    C:\Git\App1\C.cs    17  Active

[jnm2]

I watched it start doing this after working properly. I think I might have built between when it worked and when it broke, but I'm not sure.

[mavasani]

@jnm2 Your pragma suppression above mentioned CA103 instead of CA1031.

[jnm2]

Hahaha, sorry about that!

[mavasani]

@JohanLarsson I was able to repro your behavior and this is a very interesting case, so also paging in @CyrusNajmabadi and @sharwell for their views.

1. Code provided by @JohanLarsson generates an IDE0066 (when pragma suppressions are removed)
2. Adding the pramga suppression to disable IDE0066 around switch statement leads to the analyzer itself bailing out because it has the below check: https://github.com/dotnet/roslyn/blob/cf55f3a58e47298426fa971d3bd9d8857c746c65/src/Analyzers/CSharp/Analyzers/ConvertSwitchStatementToExpression/ConvertSwitchStatementToExpressionDiagnosticAnalyzer.cs#L38-L41
3. IDE0079 analyzer to detect unnecessary pragma marks this pragma suppression as unnecessary because it is not this pragma which is causing the analyzer diagnostic to be suppressed, but the analyzer itself not reporting the diagnostic due to directives check.

I also verified that if the IDE0066 pragma suppressions is move few lines out so ContainsDirectives checks fails, then things work as expected:

#pragma warning disable IDE0066 // Convert switch statement to expression
        private static int M(StringComparison stringComparison)
        {
            switch (stringComparison)
            {
                case StringComparison.CurrentCulture:
                case StringComparison.CurrentCultureIgnoreCase:
                case StringComparison.InvariantCulture:
                    return 1;
                case StringComparison.InvariantCultureIgnoreCase:
                case StringComparison.Ordinal:
                case StringComparison.OrdinalIgnoreCase:
                    return 2;
                default:
                    throw new ArgumentOutOfRangeException(nameof(stringComparison), stringComparison, null);
            }
        }
#pragma warning restore IDE0066 // Convert switch statement to expression

Basically, IDE0079 unnecessary pragma suppressions analyzer will always generate false positives for any analyzer that has such bail out checks based on presence of directives. Not sure what is the best way to approach fixing this issue.

[jnm2]

To fix just the IDE0066 case, is it easy to make an exception when the pragma is only disabling or restoring IDE0079 in particular? Then it's a matter of making analyzer writers aware of IDE0079.

[mavasani]

@jnm2 It seems like any pramga suppression, or in fact any kind of preprocessor directive anywhere inside the switch statement will end up turning off IDE0066 analyzer for this case. I have a feeling that instead we should fix IDE0066 analyzer to remove the blanket ContainsDirective bailout and require that analyzers to do more precise checks on what directives it wants to bail out on.

[CyrusNajmabadi]

to remove the blanket ContainsDirective bailout and require that analyzers to do more precise checks on what directives it wants to bail out on.

The issue here is that many rewriting analyzers effectively go: ok... trying to handle directives within me is actually too complex for me to figure out how to do sensibly. So i will just not even try as it's more likely that i will 'f' things up.

It's not that they care what is in the directive, it's that they truly just don't feel tehy can effectively rewrite in the presence of directives (of any sort).

[mavasani]

It's not that they care what is in the directive, it's that they truly just don't feel tehy can effectively rewrite in the presence of directives (of any sort).

IMO, the analyzer should not be doing such generic bailouts. It seems weird for the analyzer to stop flagging valid cases just if user adds say #pragma warning disable InvalidId. A better approach is for the analyzer to still continue flagging, and the code fixer should bailout under the assumption that fixing code with directives is tricky. This would be very similar to the way remove unnecessary usings fades in presence of usings within #if but the code fix does not actually remove these usings, user has to manually do the removal.

[sharwell]

I agree with https://github.com/dotnet/roslyn/issues/49660#issuecomment-739075105. This is a bug in the IDE0066 analyzer. Directives should not impact reported diagnostics, but the code fix should not be offered if the diagnostic occurred in the presence of a directive (manual fix required for that case).

[CyrusNajmabadi]

Directives should not impact reported diagnostics, but the code fix should not be offered if the diagnostic occurred in the presence of a directive (manual fix required for that case).

We shoudl take this to a meeting. I could be convinced of this. But it goes against all our precedents and designs so far that we only report the problem if we actually have a way to fix it. If we don't have a way, we've decided to not offer in the first place.

[CyrusNajmabadi]

A better approach is for the analyzer to still continue flagging, and the code fixer should bailout under the assumption that fixing code with directives is tricky.

As above, i'm hesitant about this (but not fully opposed to it). Up till now, we've had the position that someone can enable these diagnostics, and set whatever severity level they want, knowing that we'll then fix it. This would not introduce a new concept: issues that we report, and cause errors, but which users have to then go manually address. I don't love this idea, esp. as it might be a lot of manual work to make the change.