UseType analyzers/fixers should handle ref types

[jcouv]

Customer scenario

Use a ref var in a local declaration or foreach loop.
In 15.7, you would get a gold bar for an analyzer crash.
With this fix, the UseExplicitType and UseImplicitType analyzers and fixers can handle ref types (ie. only fix the type portion, leaving the ref alone).

Bugs this fixes

Fixes #27221

Risk

Performance impact

Low. The core of this change is to strip the ref in a few places in the analysis logic.

Is this a regression from a previous update?

No.

Root cause analysis

The bug exists since C# 7.2 (VS 2017 version 15.5) which introduced ref locals. It was compounded by foreach ref loops in C# 7.3 (15.7).

How was the bug found?

Reported by customer.

Tagging @CyrusNajmabadi @dotnet/roslyn-ide for review. Thanks

[CyrusNajmabadi]

ideally would go in a file/type called TypeSyntaxExtensions. But that's very minor and having this here is fine.

[CyrusNajmabadi]

So, the StripRef stuff looks fine to me. But what isn't clear is where the original crash was, and why this is fixing it. Could you clarify where that is in teh PR? Thanks!

[mavasani]

Nit: extra new line.

[jcouv]

The crash came from this method (see annotations).
The GetTypeInfo method returns null on ref var (it works on var, not on ref var).

      protected override bool AssignmentSupportsStylePreference(
            SyntaxToken identifier,
            TypeSyntax typeName,
            ExpressionSyntax initializer,
            SemanticModel semanticModel,
            OptionSet optionSet,
            CancellationToken cancellationToken)
        {
           ...
            var declaredType = semanticModel.GetTypeInfo(typeName, cancellationToken).Type; // returns null on `ref var`
            if (declaredType != null && declaredType.TypeKind == TypeKind.Dynamic)
            {
                return false;
            }

            ...
            var conversion = semanticModel.GetConversion(expression, cancellationToken);
            if (conversion.IsIdentity)
            {
                var initializerType = semanticModel.GetTypeInfo(expression, cancellationToken).Type;
                return declaredType.Equals(initializerType); // throws here
            }

            return false;
        }

[jcouv]

@jinujoseph for ask-mode approval for 15.8. Thanks

[Neme12]

@jcouv If declaredType shouldn't be null in that method, isn't the null check in

if (declaredType != null && declaredType.TypeKind == TypeKind.Dynamic)

unnecessary?

I would expect that the method would either be completely resilient to declaredType being null (in which case there shouldn't have been a NRE) or assume that it is never null, in which case the exception might as well have happened earlier at declaredType.TypeKind == TypeKind.Dynamic.

[jcouv]

@Neme12 There are no tests that currently result in a null declaredType. But it's not my purpose to remove a layer of defensiveness (I'd have to prove it can never be null). In other words, it's a riskier change.

What I do know, however, is that when the conversion is classified as identity there must have been a types to convert from and to convert to.
So I think the code is fine as-is.

[CyrusNajmabadi]

I'm ok with this as a spot fix. However, it def concerns me that we may have lurking issues where code assumed that "GetTypeInfo" on a TypeSyntax would always be non-null. But, for now, i think this change is fine :)

[jcouv]

@CyrusNajmabadi I'm happy to add a null check for safety. I'm just not comfortable removing one at this moment.

[jcouv]

@jinujoseph for ask-mode approval for 15.8. Thanks

[jinujoseph]

Approved to merge for 15.8.Preview3
Thanks for the fix @jcouv