IDE0019 suggests pattern matching but the generated pattern matching changes the meaning of the code

[jhudsoncedaron]

Version Used:

C# 8.0

Steps to Reproduce:

Apologies if this doesn't quite reproduce here. The origin file is some 3000 lines and the project is probably in the millions.

void ProcessRow(IDataReader reader, ISet<int> questionIds, IDictionary<int, string> visibleExpressions, IDictionary<int, string> enabledExpressions, IDictionary<int, string> completeExpressions)
{
        int i = 0;
        var questionId = reader.GetInt32(i++);
        var dataLevelId = reader[i++] as byte?;
        var visibleExpression = reader[i++] as string;
        var enabledExpression = reader[i++] as string;
        var completeExpression = reader[i++] as string;

        if (questionIds.Contains(questionId))
        {
            if (visibleExpression != null)
               visibleExpressions[questionId] = visibleExpression;
            if (enabledExpression != null)
               enabledExpressions[questionId] = enabledExpression;
            if (completeExpression != null)
               completeExpressions[questionId] = completeExpression;
        }
}

For some reason it wants to use pattern matching but can't understand that i has a different value by that time and the code replacement is non-working code.

We use this indexing idiom here because this loop runs thousands of times per web request and it's very nearly as resilient to schema changes as string indexers and just as easy to read as string indexers and as fast as numeric literal indexers into the data reader. The real code has 18 columns here.

Expected Behavior:

No ... under var as the fix can't be applied here.

Actual Behavior:

Attached screen grab.

[dpoeschl]

Should we only allow the refactoring if the as & null check are adjacent? Or allow this but show a warning in the preview? Or ?

[jhudsoncedaron]

I guess my view is fundamentally simpler than yours. It reads "Don't suggest fixes you don't know don't change the meaning of the code."

[CyrusNajmabadi]

It reads "Don't suggest fixes you don't know don't change the meaning of the code."

These are effectively all fixes. We abandoned that approach because it meant we could almost never actually supply options, even when it was safe 99%+ of the time.

as a simple example:

var v = a.b == null ? null : a.b.c;

In nearly every codebase, this can safely move to:

var v = a.b?.c;

But that's not guaranteed, and there's a chance semantics may break. We opt to helping out the majority of users for the most common code patterns, and we'd like warnings for users that use patterns where the fix may be possible, but may statically look like it carries more risk.

[CyrusNajmabadi]

Should we only allow the refactoring if the as & null check are adjacent?

I wouldn't do that. Consider:

        var dataLevelId = x as byte?;
        var visibleExpression = y as string;
        var enabledExpression = z as string;
        var completeExpression = w as string;

        if (questionIds.Contains(questionId))
        {
            if (visibleExpression != null)
               visibleExpressions[questionId] = visibleExpression;
            if (enabledExpression != null)
               enabledExpressions[questionId] = enabledExpression;
            if (completeExpression != null)
               completeExpressions[questionId] = completeExpression;
        }
}

There would still be merit here in updating the code.

I would, however, look for obvious things that signal mutation (++) being one of those things, and then move to warning (optionally with a different diagnostic or property). The different diagnostic/property would exclude these cases from participating in fix-all due to the higher risk associated with them.

[jhudsoncedaron]

var v = a.b == null ? null : a.b.c;

In nearly every codebase, this can safely move to:

var v = a.b?.c;

I actually had a sample where this didn't work. Somebody actually fixed it in a recent release to stop offering it on just that case. (b had an overload of == that did something funny with null).

[CyrusNajmabadi]

I actually had a sample where this didn't work.

Yes, as i mentioned: But that's not guaranteed, and there's a chance semantics may break. We opt to helping out the majority of users for the most common code patterns, and we'd like warnings for users that use patterns where the fix may be possible, but may statically look like it carries more risk.

The goal is to not let the rare cases prevent providing value for the more common and normal types of code cases. Where we can detect something potentially odd, we can try to add appropriate smarts. However, there are whole hosts of cases where there is no way to know statically and we can choose to basically cut the feature, or assume common coding patterns and attempt to offer the user something helpful for almost all cases.

[CyrusNajmabadi]

Note: happy to discuss this more over at gitter.im/dotnet/roslyn or aka.ms/csharp-discord. Thanks!