CallBuilder produces strange cast on call target, in case two very similar overloads are present

[CreateAndInject]

StrangeCast.zip

See XpoTspecInsulInfo2021:

public int XpoTspecInsulInfo2021Id
{
	get
	{
		return _fXpoTspecInsulInfo2021Id;
	}
	set
	{
		((PersistentBase)this).SetPropertyValue<int>("XpoTspecInsulInfo2021Id", ref _fXpoTspecInsulInfo2021Id, value);
	}
}

There's ((PersistentBase)this), but there's no such issue in the other properties defined in the same type

[siegfriedpammer]

The problem here is that there are two very similar methods defined in the base class PersistentBase.

protected bool SetPropertyValue<T>(string propertyName, ref T propertyValueHolder, T newValue);
protected bool SetPropertyValue(string propertyName, ref int propertyValueHolder, int newValue);

The IL tells us that the generic overload should be used:

IL_000d: call instance bool PersistentBase::SetPropertyValue<int32>(string, !!0&, !!0) /* 2B000081 */

And while it would be sufficient to specify generic type arguments to disambiguate in this case, our algorithm is not clever enough to try that combination:

ILSpy/ICSharpCode.Decompiler/CSharp/CallBuilder.cs

Lines 991 to 1031 in 06039f2

	// TODO : implement some more intelligent algorithm that decides which of these fixes (cast args, add target, cast target, add type args)
	// is best in this case. Additionally we should not cast all arguments at once, but step-by-step try to add only a minimal number of casts.
	if (argumentList.FirstOptionalArgumentIndex >= 0)
	{
	argumentList.FirstOptionalArgumentIndex = -1;
	}
	else if (!argumentsCasted)
	{
	// If we added type arguments beforehand, but that didn't make the code any better,
	// undo that decision and add casts first.
	if (appliedRequireTypeArgumentsShortcut)
	{
	requireTypeArguments = false;
	typeArguments = Empty<IType>.Array;
	appliedRequireTypeArgumentsShortcut = false;
	}
	argumentsCasted = true;
	argumentList.UseImplicitlyTypedOut = false;
	CastArguments(argumentList.Arguments, argumentList.ExpectedParameters);
	}
	else if ((allowedTransforms & CallTransformation.RequireTarget) != 0 && !requireTarget)
	{
	requireTarget = true;
	targetResolveResult = target.ResolveResult;
	}
	else if ((allowedTransforms & CallTransformation.RequireTarget) != 0 && !targetCasted)
	{
	targetCasted = true;
	target = target.ConvertTo(method.DeclaringType, expressionBuilder);
	targetResolveResult = target.ResolveResult;
	}
	else if ((allowedTransforms & CallTransformation.RequireTypeArguments) != 0 && !requireTypeArguments)
	{
	requireTypeArguments = true;
	typeArguments = method.TypeArguments.ToArray();
	}
	else
	{
	break;
	}
	continue;

We try the following things (in that order):

1. cast all arguments to the expected types
2. add call target, if it was optional
3. cast the target to the exact type
4. add type arguments explicitly

If steps 2/3 and 4 are swapped, then the produced code should just be:

SetPropertyValue<int>("XpoTspecInsulInfo2021Id", ref _fXpoTspecInsulInfo2021Id, value);

However, I think there are many cases where this change would make the code look a lot worse... especially if the method has more than one type argument, or the type argument is a generic type with multiple levels of nesting.
Also note that there is already a TODO note on that issue on line 991.

[CreateAndInject]

@siegfriedpammer Seems no need to swap steps, since SetPropertyValue is protected, so ((PersistentBase)this).SetPropertyValue is invalid

[siegfriedpammer]

Accessing protected members from derived classes is perfectly fine. What do you mean?

[CreateAndInject]

@siegfriedpammer

[siegfriedpammer]

Ah, yes, sorry, you are right. That is definitely something that we should fix. Thanks for reporting.