Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implicit conversions in a type not working #248

Open
mocakturk opened this issue Aug 18, 2022 · 4 comments
Open

Implicit conversions in a type not working #248

mocakturk opened this issue Aug 18, 2022 · 4 comments

Comments

@mocakturk
Copy link

Hello,

First of all, DynamicExpresso is great library. I hate using CodeDom, because it is slow. I hate using Roslyn, because it depends on dozens of other libraries...

I noticed that, DynamicExpresso does not use implicit conversions defined in a custom type.
For example, I have a type like following:

class MyCustomType
{
	public static implicit operator long(MyCustomType d) { ... }
	public static implicit operator ulong(MyCustomType d) { ... }
	public static implicit operator float(MyCustomType d) { ... }
	public static implicit operator double(MyCustomType d) { ... }
	public static implicit operator string(MyCustomType d) { ... }
}

I'm expecting to to be able to execute following expression:
"GetMyCustomType() + 5"
However, expression is not compiled.

I need to add overload every operator (+, -, *, / |, <<, >>, ...) for each basic types into the MyCustomType.
Is it possible to support using the implicit operators in a type?

@holdenmai
Copy link
Contributor

holdenmai commented Aug 21, 2022

In a way, this is a simple ask as the implicit conversion is taken care of by Expression.Convert as shown below

private class Convertible<T>
{
    private readonly T m_sourceValue;

    public Convertible(T sourceValue)
    {
        m_sourceValue = sourceValue;
    }

    public T SourceValue { get { return m_sourceValue; } }

    public static implicit operator T(Convertible<T> source)
    {
        return source.SourceValue;
    }

    public static implicit operator Convertible<T>(T source)
    {
        return new Convertible<T>(source);
    }
}

[Test]
public void ExampleExpressions()
{
    var initialValue = Expression.Constant(1);
    var assignedValue = Expression.Variable(typeof(Convertible<int>));
    var reassignedValue = Expression.Variable(typeof(int));

    Expression.Lambda(Expression.Block(new ParameterExpression[] { assignedValue, reassignedValue },
        Expression.Assign(assignedValue, Expression.Convert(initialValue, assignedValue.Type)),
        Expression.Assign(reassignedValue, Expression.Convert(assignedValue, reassignedValue.Type))
    )).Compile().DynamicInvoke();
}

Expression.Convert calls the corresponding conversion operator, regardless of the operator being implicit or explicit.
It also determines if the convert call is even necessary upon compilation.
Changing the test method to below shows a Convert call in the debug view for both assignments, however the compiled expression calls no operator.

[Test]
public void ExampleExpressions()
{
	var initialValue = Expression.Constant(1);
	var assignedValue = Expression.Variable(typeof(Convertible<int>));
	var reassignedValue = Expression.Variable(typeof(Convertible<int>));

	var lambda = Expression.Lambda(Expression.Block(new ParameterExpression[] { assignedValue, reassignedValue },
		Expression.Assign(assignedValue, Expression.Convert(initialValue, assignedValue.Type)),
		Expression.Assign(reassignedValue, Expression.Convert(assignedValue, reassignedValue.Type))
		));
	var result = lambda.Compile().DynamicInvoke();
}

As such, for assignment calls this is trivial as we could always call Convert on the value being assigned.

Method lookup is more difficult as shown by the following code

[Test]
public void TestMethodLookup()
{
	var c = new Convertible<int>(5);
	MethodWithBaseType(c);
	var reflectionMethod = GetType().GetMethod(nameof(MethodWithBaseType), new Type[] { c.GetType() });
	Assert.IsNull(reflectionMethod);
}

public void MethodWithBaseType(int i)
{
}

Note how the c# compiler correctly finds the method, but the reflection does not.

From some quick investigation of code base, it seems like all that is needed is to update IsCompatibleWith method to be able to look up conversion operators since the Expression.Convert takes care of the actual conversion call.
The biggest downside to that is determining if we want to require the (TargetType) convention if the resulting conversion is explicit.

However, the determination of when to perform the conversion can be substantially more difficult once it comes to operators as it is necessary to determine which argument to convert based on which operators are present, or if conversion is not necessary due to operators already existing on the class.

In the case of MyCustomType as defined above, anytime an instance of that uses one of the operators defined on the conversion type, it would always be converted.

As shown below things can get pretty interesting once you start adding your own operators.

public static Convertible<int> operator +(Convertible<T> left, int right)
{
	if (left is Convertible<int> ci)
	{
		return ci.m_sourceValue + (right * 2);
	}
	throw new NotImplementedException();
}

public static Convertible<int> operator +(int left, Convertible<T> right)
{
	if (right is Convertible<int> ci)
	{
		return ci.m_sourceValue + (left * 3);
	}
	throw new NotImplementedException();
}


public static Convertible<decimal> operator +(Convertible<T> left, Convertible<T> right)
{
	if (right is Convertible<decimal> ci && left is Convertible<decimal> cir)
	{
		return ci.m_sourceValue + cir.m_sourceValue;
	}
	throw new NotImplementedException();
}


[Test]
public void OperatorExpressions()
{
	var arg1 = new Convertible<int>(5);
	var arg2 = 3;

	var testConvLeft = arg1 + arg2;
	//To prove the compiler automatically lifts it to the result type of the operator
	//and we aren't getting any changed results from conversion
	Assert.IsInstanceOf(typeof(Convertible<int>), testConvLeft);
	//Also to prove we are calling ours.
	Assert.AreEqual(11, testConvLeft.SourceValue);
	var testConvRight = arg2 + arg1;
	Assert.AreEqual(14, testConvRight.SourceValue);

	//c# compiler automatically converts down to the implicit conversion type since we don't actually have one defined.
	var usingIntOperators = arg1 - arg2;
	Assert.AreEqual(2, usingIntOperators);
	usingIntOperators = arg2 - arg1;
	Assert.AreEqual(usingIntOperators, -2);

	var decimalL = new Convertible<decimal>(10m);
	var decimalR = new Convertible<decimal>(5m);
	Assert.AreEqual(15m, (decimalL + decimalR).SourceValue);
}

However, all that noise boils down to the following logic

  1. If there is an operator defined that uses both types explicitly, use that one
  2. Is there an operator on one of the types where the parameter can be converted directly?
  3. Is that operator present on one of the result types of the conversions?

Point 3 is where things can cause problems/ambiguous operator detection.
In the case of MyCustomType the following would not be allowed

new MyCustomType((int)5) + new MyCustomType((int)5)

This results in a compiler error of "Operator "+" is ambiguous on operands of type 'MyCustomType' and 'MyCustomType'"

I think this covers all of the typical use cases where implicit conversions are respected.

@metoule
Copy link
Contributor

metoule commented Aug 21, 2022

Hi @holdenmai, thanks for the investigation, that's really interesting!
FYI, if you want to go deeper, you can read this section of the C# language specifications.

I'm not sure we should treat user defined operators any differently than the way we treat other methods. I suspect DynamicExpresso doesn't handle implicit conversion for method call parameters either. I think a good place to start would be CheckIfMethodIsApplicableAndPrepareIt: that's where we check if a list of methods can be called with the provided arguments. If none match, we could check if an implicit conversion exist, or simply use Expression.Convert like you suggest.

WDYT?

@holdenmai
Copy link
Contributor

Decided to dig into how the .net standard performs the operator check to see if there was something publicly available. Unfortunately not, however the logic they use to check if there is a conversion is present here:
.NET TypeUtils

So potentially we could use this logic as well.

@cpiber
Copy link
Contributor

cpiber commented Sep 2, 2024

A nice step into this direction would also be to make MemberFinder configurable enough to at least allow custom promotions, which would help our case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants