Skip to content

Commit

Permalink
fixes calls to 'Object.GetType()' on value types generating incorrect…
Browse files Browse the repository at this point in the history 
… code (#298)
  • Loading branch information
@adrianoc
adrianoc committed Oct 5, 2024
1 parent dbd20e4 commit 2220a5e
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 28 deletions.
58 changes: 57 additions & 1 deletion Cecilifier.Core.Tests/Tests/Unit/InvocationOnValueTypeTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ namespace Cecilifier.Core.Tests.Tests.Unit;


/// <summary>
/// These tests covers scenarios in which we end up with a call on a value type.
/// These tests cover scenarios in which we end up with a call on a value type.
/// </summary>
[TestFixture]
public class InvocationOnValueTypeTests : CecilifierUnitTestBase
Expand Down Expand Up @@ -109,4 +109,60 @@ public void TestCallVersusCallVirt(string snippet, string expectedCall)
var found = Regex.Matches(cecilifiedCode, expectedCall);
Assert.That(found.Count, Is.EqualTo(1), $"Expected call instruction not found ({expectedCall})\n\n{cecilifiedCode}");
}

[TestCase(
"System.Console.WriteLine(42.GetType().Name);",
"assembly.MainModule.TypeSystem.Int32",
"Ldloc",
TestName = "Literal")]
[TestCase(
"bool b = true; System.Console.WriteLine(b.GetType().Name);",
"assembly.MainModule.TypeSystem.Boolean",
"Ldloc",
TestName = "Local Variable")]
[TestCase(
"int Get() => 42; string M() => Get().GetType().Name;",
"assembly.MainModule.TypeSystem.Int32",
"Ldloc",
TestName = "Return")]
[TestCase(
"string M(long l) => l.GetType().Name;",
"assembly.MainModule.TypeSystem.Int64",
"Ldarg",
TestName = "Parameter")]
[TestCase(
"class Foo { double d; string M() => d.GetType().Name; }",
"assembly.MainModule.TypeSystem.Double",
"Ldfld",
TestName = "Field")]
[TestCase(
"int[] ints = {1}; System.Console.WriteLine(ints[0].GetType().Name);",
"assembly.MainModule.TypeSystem.Int32",
"Ldelem",
TestName = "Array Element")]
[TestCase(
"int i =42; ref int rf = ref i; System.Console.WriteLine(rf.GetType().Name);",
"assembly.MainModule.TypeSystem.Int32",
"Ldind",
TestName = "Local Ref",
IgnoreReason = "Corner case, does not worth the extra complexity to handle.")]
[TestCase(
"string M(ref int ri) => ri.GetType().Name;",
"assembly.MainModule.TypeSystem.Int32",
"Ldind",
TestName = "Parameter Ref",
IgnoreReason = "Corner case, does not worth the extra complexity to handle.")]
public void CallGetTypeOnValueType(string snippet, string boxTargetType, string expectedLoadBeforeBox)
{
var result = RunCecilifier(snippet);
var cecilifiedCode = result.GeneratedCode.ReadToEnd();

var expectedCall = $"""
(\s+.+\.Emit\(OpCodes.){expectedLoadBeforeBox}.+\);
\1Box, {boxTargetType}\);
\1Callvirt,.+ImportReference\(TypeHelpers.ResolveMethod\(typeof\(System.Object\), "GetType".+(System\.Reflection\.BindingFlags\.)Default\|\2Instance\|\2Public\)\)\);
""";
var found = Regex.Matches(cecilifiedCode, expectedCall);
Assert.That(found.Count, Is.EqualTo(1), $"Expected call instruction not found:\n{expectedCall}\n\n{cecilifiedCode}");
}
}
16 changes: 11 additions & 5 deletions Cecilifier.Core/AST/ExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ public override void VisitLiteralExpression(LiteralExpressionSyntax node)
LoadLiteralValue(ilVar,
literalType,
value,
literalParent.Accept(UsageVisitor.GetInstance(Context)) == UsageKind.CallTarget,
literalParent.Accept(UsageVisitor.GetInstance(Context)),
literalParent);

skipLeftSideVisitingInAssignment = (literalType.IsValueType || literalType.TypeKind == TypeKind.TypeParameter) && !literalType.IsPrimitiveType();
Expand Down Expand Up @@ -816,8 +816,8 @@ public override void VisitDefaultExpression(DefaultExpressionSyntax node)
var type = Context.GetTypeInfo(node.Type).Type.EnsureNotNull();

var defaultParent = node.Parent.EnsureNotNull<SyntaxNode, CSharpSyntaxNode>();
var isTargetOfCall = defaultParent.Accept(UsageVisitor.GetInstance(Context)) == UsageKind.CallTarget;
LoadLiteralValue(ilVar, type, type.ValueForDefaultLiteral(), isTargetOfCall, defaultParent);
var usageResult = defaultParent.Accept(UsageVisitor.GetInstance(Context));
LoadLiteralValue(ilVar, type, type.ValueForDefaultLiteral(), usageResult, defaultParent);

skipLeftSideVisitingInAssignment = (type.IsValueType || type.TypeKind == TypeKind.TypeParameter) && !type.IsPrimitiveType();
}
Expand Down Expand Up @@ -1214,7 +1214,13 @@ bool IsObjectCreationExpressionUsedAsInParameter(ExpressionSyntax toBeChecked)
private void StoreTopOfStackInLocalVariableAndLoad(ExpressionSyntax expressionSyntax, ITypeSymbol type)
{
var tempLocalName = StoreTopOfStackInLocalVariable(Context, ilVar, "tmp", type).VariableName;
Context.EmitCilInstruction(ilVar, RequiresAddressOfValue() ? OpCodes.Ldloca_S : OpCodes.Ldloc, tempLocalName);
if (!HandleLoadAddress(ilVar, type, expressionSyntax, OpCodes.Ldloca_S, tempLocalName))
{
// HandleLoadAddress() does not handle scenarios in which a value type instantiation is passed as an
// 'in parameter' to a method (that method is already complex so I don't want to make it even more
// complex)
Context.EmitCilInstruction(ilVar, RequiresAddressOfValue() ? OpCodes.Ldloca_S : OpCodes.Ldloc, tempLocalName);
}

var parentMae = expressionSyntax.FirstAncestorOrSelf<MemberAccessExpressionSyntax>(ancestor => ancestor.Kind() == SyntaxKind.SimpleMemberAccessExpression);
if (parentMae != null && SymbolEqualityComparer.Default.Equals(Context.SemanticModel.GetSymbolInfo(parentMae).Symbol.ContainingType, Context.RoslynTypeSystem.SystemValueType))
Expand Down Expand Up @@ -1437,7 +1443,7 @@ private void ProcessArgumentsTakingDefaultParametersIntoAccount(ISymbol? method,

foreach (var arg in methodSymbol.Parameters.Skip(args.Arguments.Count))
{
LoadLiteralValue(ilVar, arg.Type, ArgumentValueToUseForDefaultParameter(arg, methodSymbol.Parameters, args.Arguments), false, args);
LoadLiteralValue(ilVar, arg.Type, ArgumentValueToUseForDefaultParameter(arg, methodSymbol.Parameters, args.Arguments), UsageResult.None, args);
}
}

Expand Down
74 changes: 52 additions & 22 deletions Cecilifier.Core/AST/SyntaxWalkerBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ protected string CreateCilInstruction(string ilVar, string instVar, OpCode opCod
return instVar;
}

protected void LoadLiteralValue(string ilVar, ITypeSymbol type, string value, bool isTargetOfCall, SyntaxNode parent)
protected void LoadLiteralValue(string ilVar, ITypeSymbol type, string value, UsageResult usageResult, SyntaxNode parent)
{
if (LoadDefaultValueForTypeParameter(ilVar, type, parent))
return;
Expand Down Expand Up @@ -114,7 +114,7 @@ protected void LoadLiteralValue(string ilVar, ITypeSymbol type, string value, bo
break;

case SpecialType.System_Char:
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, (int) value[0], isTargetOfCall);
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, (int) value[0], usageResult);
break;

case SpecialType.System_Byte:
Expand All @@ -126,15 +126,15 @@ protected void LoadLiteralValue(string ilVar, ITypeSymbol type, string value, bo
case SpecialType.System_UInt64:
case SpecialType.System_Double:
case SpecialType.System_Single:
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, value, isTargetOfCall);
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, value, usageResult);
break;

case SpecialType.System_Boolean:
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, Boolean.Parse(value) ? 1 : 0, isTargetOfCall);
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, Boolean.Parse(value) ? 1 : 0, usageResult);
break;

case SpecialType.System_IntPtr:
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, value, isTargetOfCall);
LoadLiteralToStackHandlingCallOnValueTypeLiterals(ilVar, type, value, usageResult);
Context.EmitCilInstruction(ilVar, OpCodes.Conv_I);
break;

Expand Down Expand Up @@ -191,12 +191,21 @@ private bool LoadDefaultValueForTypeParameter(string ilVar, ITypeSymbol type, Sy
return true;
}

private void LoadLiteralToStackHandlingCallOnValueTypeLiterals(string ilVar, ITypeSymbol literalType, object literalValue, bool isTargetOfCall)
private void LoadLiteralToStackHandlingCallOnValueTypeLiterals(string ilVar, ITypeSymbol literalType, object literalValue, UsageResult usageResult)
{
var opCode = literalType.LoadOpCodeFor();
Context.EmitCilInstruction(ilVar, opCode, literalValue);
if (isTargetOfCall)
StoreTopOfStackInLocalVariableAndLoadItsAddress(ilVar, literalType);
if (usageResult.Kind == UsageKind.CallTarget)
{
var tempLocalName = StoreTopOfStackInLocalVariable(Context, ilVar, "tmp", literalType).VariableName;
if (!usageResult.Target.IsVirtual && SymbolEqualityComparer.Default.Equals(usageResult.Target.ContainingType, Context.RoslynTypeSystem.SystemObject))
{
Context.EmitCilInstruction(ilVar, OpCodes.Ldloc, tempLocalName);
Context.EmitCilInstruction(ilVar, OpCodes.Box, Context.TypeResolver.Resolve(literalType));
}
else
Context.EmitCilInstruction(ilVar, OpCodes.Ldloca_S, tempLocalName);
}
}

private void StoreTopOfStackInLocalVariableAndLoadItsAddress(string ilVar, ITypeSymbol type, string variableName = "tmp")
Expand Down Expand Up @@ -429,7 +438,7 @@ protected void ProcessField(string ilVar, SimpleNameSyntax node, IFieldSymbol fi
SpecialType.System_Boolean => (bool) fieldSymbol.ConstantValue ? 1 : 0,
_ => fieldSymbol.ConstantValue
},
nodeParent.Accept(UsageVisitor.GetInstance(Context)) == UsageKind.CallTarget);
nodeParent.Accept(UsageVisitor.GetInstance(Context)));
return;
}

Expand Down Expand Up @@ -485,30 +494,51 @@ private bool HandleLoadAddressOnStorage(string ilVar, ITypeSymbol symbol, CSharp
return HandleLoadAddress(ilVar, symbol, node, opCode, operand);
}

protected bool HandleLoadAddress(string ilVar, ITypeSymbol symbol, CSharpSyntaxNode node, OpCode opCode, string operand)
protected bool HandleLoadAddress(string ilVar, ITypeSymbol loadedType, CSharpSyntaxNode node, OpCode loadOpCode, string operand)
{
var parentNode = (CSharpSyntaxNode)node.Parent;
return HandleCallOnTypeParameter() || HandleCallOnValueType() || HandleRefAssignment() || HandleParameter() || HandleInlineArrayElementAccess();

bool HandleCallOnValueType()
{
if (!symbol.IsValueType)
if (!loadedType.IsValueType)
return false;

// in this case we need to call System.Index.GetOffset(int32) on a value type (System.Index)
// which requires the address of the value type.
var isSystemIndexUsedAsIndex = IsSystemIndexUsedAsIndex(symbol, parentNode);
var usageResult = parentNode.Accept(UsageVisitor.GetInstance(Context).WithTargetNode(node));
var isSystemIndexUsedAsIndex = IsSystemIndexUsedAsIndex(loadedType, parentNode);
var usageResult = parentNode!.Accept(UsageVisitor.GetInstance(Context).WithTargetNode(node));
if (isSystemIndexUsedAsIndex || parentNode.IsKind(SyntaxKind.AddressOfExpression) || IsPseudoAssignmentToValueType() || node.IsMemberAccessOnElementAccess() || usageResult.Kind == UsageKind.CallTarget)
{
Context.EmitCilInstruction(ilVar, opCode, operand);
if (usageResult.Kind == UsageKind.CallTarget && SymbolEqualityComparer.Default.Equals(usageResult.Target.ContainingType, Context.RoslynTypeSystem.SystemObject) && !usageResult.Target.IsVirtual)
{
var ordinaryLoad = loadOpCode.Code switch
{
Code.Ldarga => OpCodes.Ldarg,
Code.Ldarga_S => OpCodes.Ldarg_S,

Code.Ldloca => OpCodes.Ldloc,
Code.Ldloca_S => OpCodes.Ldloc_S,

Code.Ldflda => OpCodes.Ldfld,
Code.Ldsflda => OpCodes.Ldsfld,

Code.Ldelema => (Dummy:operand=null, Ret:loadedType.LdelemOpCode()).Ret,
_ => throw new InvalidOperationException($"Cannot find ordinary load op code for {loadOpCode}")
};
Context.EmitCilInstruction(ilVar, ordinaryLoad, operand);
Context.EmitCilInstruction(ilVar, OpCodes.Box, Context.TypeResolver.Resolve(loadedType));
}
else
Context.EmitCilInstruction(ilVar, loadOpCode, operand);

if (!Context.HasFlag(Constants.ContextFlags.Fixed) && parentNode.IsKind(SyntaxKind.AddressOfExpression))
Context.EmitCilInstruction(ilVar, OpCodes.Conv_U);

// calls to virtual methods on custom value types needs to be constrained (don't know why, but the generated IL for such scenarios does `constrains`).
// the only methods that falls into this category are virtual methods on Object (ToString()/Equals()/GetHashCode())
if (usageResult.Target is { IsOverride: true } && usageResult.Target.ContainingType.IsNonPrimitiveValueType(Context))
Context.SetFlag(Constants.ContextFlags.MemberReferenceRequiresConstraint, Context.TypeResolver.Resolve(symbol));
Context.SetFlag(Constants.ContextFlags.MemberReferenceRequiresConstraint, Context.TypeResolver.Resolve(loadedType));
return true;
}

Expand All @@ -517,7 +547,7 @@ bool HandleCallOnValueType()

bool HandleCallOnTypeParameter()
{
if (symbol is not ITypeParameterSymbol typeParameter)
if (loadedType is not ITypeParameterSymbol typeParameter)
return false;

if (typeParameter.HasReferenceTypeConstraint || typeParameter.IsReferenceType)
Expand All @@ -526,8 +556,8 @@ bool HandleCallOnTypeParameter()
if (parentNode.Accept(UsageVisitor.GetInstance(Context)) != UsageKind.CallTarget)
return false;

Context.EmitCilInstruction(ilVar, opCode, operand);
Context.SetFlag(Constants.ContextFlags.MemberReferenceRequiresConstraint, Context.TypeResolver.Resolve(symbol));
Context.EmitCilInstruction(ilVar, loadOpCode, operand);
Context.SetFlag(Constants.ContextFlags.MemberReferenceRequiresConstraint, Context.TypeResolver.Resolve(loadedType));
return true;
}

Expand All @@ -540,7 +570,7 @@ bool HandleRefAssignment()
if (assignedValueSymbol.Symbol.IsByRef())
return false;

Context.EmitCilInstruction(ilVar, opCode, operand);
Context.EmitCilInstruction(ilVar, loadOpCode, operand);
return true;
}

Expand All @@ -551,18 +581,18 @@ bool HandleParameter()

if (Context.SemanticModel.GetSymbolInfo(argument.Expression).Symbol?.IsByRef() == false)
{
Context.EmitCilInstruction(ilVar, opCode, operand);
Context.EmitCilInstruction(ilVar, loadOpCode, operand);
return true;
}
return false;
}

bool HandleInlineArrayElementAccess()
{
if (!node.Parent.IsKind(SyntaxKind.ElementAccessExpression) || !symbol.TryGetAttribute<InlineArrayAttribute>(out var _))
if (!node.Parent.IsKind(SyntaxKind.ElementAccessExpression) || !loadedType.TryGetAttribute<InlineArrayAttribute>(out var _))
return false;

Context.EmitCilInstruction(ilVar, opCode, operand);
Context.EmitCilInstruction(ilVar, loadOpCode, operand);
return true;
}

Expand Down
1 change: 1 addition & 0 deletions Cecilifier.Core/AST/UsageResult.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ struct UsageResult
{
public UsageKind Kind { get; }
public ISymbol Target { get; }
public static UsageResult None = new(UsageKind.None, null);

public static implicit operator UsageKind(UsageResult result) => result.Kind;

Expand Down

0 comments on commit 2220a5e

Please sign in to comment.