This project is read-only.
3

Closed

GetConversion incorrectly returns "outer" conversion on user-defined implicit conversion

description

I have a terrible feeling that this one is my bad.

As you know, a user-defined conversion is permitted to have a standard conversion on "either end". Suppose we have a user-defined implicit conversion that takes a C and returns a D. Suppose that B is convertible to C by a standard implicit conversion, and similarly D is convertible to E. This means that a user-defined conversion is permitted to go from B to C, to D, to E.

Suppose that we have a class type Test and a user-defined conversion which takes a Test and returns an int. Int to long is a standard implicit conversion, so it should be legal to convert from Test to long via a user-defined conversion.

And it is legal; that's not the problem. The problem is that GetConversion returns the conversion classification for the "back end" conversion from int to long. But a conversion from Test to long is not classified as an implicit numeric conversion; it's a user-defined implicit conversion with an implicit numeric conversion on the back end.

Here's an example.
using System;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using System.Collections.Immutable;
using Microsoft.CodeAnalysis.CSharp.Syntax;

namespace EricLippertScratch
{
    class Program
    {
        static void Main()
        {
            const string sourceText = @"
public class Test
{
    public static implicit operator int(Test t) { return 0; }

    public void MyTest(Test myTest)
    {
        int myInt;
        long myLong;
        
        myInt = myTest;
        myLong = myTest;
    }
}
";
            var references =
                ImmutableArray.Create(
                    typeof(object),
                    typeof(System.Linq.Enumerable))
                .Select(type => new MetadataFileReference(
                    fullPath: type.Assembly.Location,
                    kind: MetadataImageKind.Assembly,
                    alias: null,
                    embedInteropTypes: false))
                .ToImmutableArray();

            var tree = SyntaxFactory.ParseSyntaxTree(sourceText, "fileName", CSharpParseOptions.Default);

            var compilation = CSharpCompilation.Create(
                assemblyName: "outputFile",
                options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary),
                syntaxTrees: ImmutableArray.Create( tree ),
                references: references);

            var model = compilation.GetSemanticModel(tree);

            var diagnostics = compilation.GetDiagnostics();

            var convs = tree
                .GetRoot()
                .DescendantNodes()
                .OfType<BinaryExpressionSyntax>()
                .Select(x => model.GetConversion(x.Right))
                .ToImmutableArray();

            foreach (var conv in convs)
            {
                Console.WriteLine(conv + " " + conv.MethodSymbol);

            }
        }
    } 
}
Expected output:

ImplicitUserDefined Test.implicit operator int(Test)
ImplicitUserDefined Test.implicit operator int(Test)

Observed output:

ImplicitUserDefined Test.implicit operator int(Test)
ImplicitNumeric

I have not yet debugged through the code to figure out where it goes wrong.
Closed Dec 2, 2014 at 1:48 AM by nmgafter
Fixed. Not a huge design error, Eric. It just requires a tiny bit of code in SemanticModel.

comments

EricLippert wrote Sep 13, 2014 at 12:03 AM

Interestingly, replacing the projection with

.Select(x => model.ClassifyConversion(x.Right, model.GetTypeInfo(x.Right).ConvertedType))

does produce the correct results. This should be an effective workaround.

EricLippert wrote Sep 15, 2014 at 7:40 PM

I now understand the difference between the case that fails and the workaround case. What we have here is yet another premature lowering, and yes, my bad.

The workaround calls ClassifyConversion directly, which comes back with "this is a user-defined conversion with an identity conversion on the front end and a numeric conversion on the back end", and so that is what is reported.

The original, failing code calls GetConversion on the right hand side of an assignment. The semantic model begins by stepping out to the assignment expression and binding the whole thing. So it binds the left and right sides of the assignment, and then must determine if the expression on the right is assignable to the type of the left.

It calls ClassifyConversion to do so, which of course comes back with "user-defined conversion, identity and numeric". The binder then lowers that into the tree:
         assignment
    /                       \
local              numeric conversion
                                     \ 
                                 user-defined conversion
                                       \
                                      formal                              
And now GetConversion interrogates the right hand side of the bound assignment and discovers hey, there's a numeric conversion on the top of that thing, so it reports that.

A better strategy would have been to NOT prematurely lower the user-defined conversion into a numeric conversion on top of a user-defined conversion, but rather to have a bound-user-defined conversion node that keeps track of the extra conversions on the top and bottom. The lowering into a conversion on top of a method call to the conversion method could happen during the lowering pass.