Skip to content

Commit

Permalink
fixes Nullable<T> initialization with null literal (#323)
Browse files Browse the repository at this point in the history
such intialization (for instance 'int? i=null;'  was generating invalid code
  • Loading branch information
adrianoc committed Jan 16, 2025
1 parent 749380e commit 475fc16
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 5 deletions.
55 changes: 55 additions & 0 deletions Cecilifier.Core.Tests/Tests/Unit/NullableTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -116,4 +116,59 @@ public void ConstructorIsInvokedAfterCast()
\k<emit>Ret\);
"""));
}

[TestCase(/*language=C#*/
//"class C { void M(System.DateTime p) => p = default; }",
"class C { void M(int? p) => p = null; }",
"""
//p = null
\s+il_M_2.Emit\(OpCodes.Ldarga, p_p_\d+\);
\s+il_M_2.Emit\(OpCodes.Initobj, .+ImportReference\(typeof\(System.Nullable<>\)\).MakeGenericInstanceType\(.+Int32\)\);
""",
TestName = "Parameter")]
[TestCase(/*language=C#*/
"class C { void M() { int? i; i = null; } }",
"""
//i = null;
\s+il_M_2.Emit\(OpCodes.Ldloca, l_i_\d+\);
\s+il_M_2.Emit\(OpCodes.Initobj, .+ImportReference\(typeof\(System.Nullable<>\)\).MakeGenericInstanceType\(.+Int32\)\);
""",
TestName = "Local variable assignment")]
[TestCase(/*language=C#*/
"class C { void M() { int? i = null; } }",
"""
//int\? i = null;
\s+var (?<var>l_i_\d+) = new VariableDefinition\((?<nullable>.+ImportReference\(typeof\(System.Nullable<>\)\)\.MakeGenericInstanceType\(.+Int32\))\);
\s+m_M_1.Body.Variables.Add\(\k<var>\);
(?<emit>\s+il_M_\d+\.Emit\(OpCodes\.)Ldloca, \k<var>\);
\k<emit>Initobj, \k<nullable>\);
""",
TestName = "Local variable initializer")]
[TestCase(/*language=C#*/
"class C { void M(int? p) => M(null); }",
"""
//M\(null\)
(\s+il_M_\d+\.Emit\(OpCodes\.)Ldarg_0\);
\s+var (?<var>l_tmpNull_\d+) = new VariableDefinition\(.+ImportReference\(.+System.Nullable<>\)\).MakeGenericInstanceType\(.+Int32\)\);
\s+m_M_\d+.Body.Variables.Add\(\k<var>\);
\1Ldloca_S, \k<var>\);
\1Initobj, .+ImportReference\(typeof\(System.Nullable<>\)\).MakeGenericInstanceType\(.+Int32\)\);
(\s+il_M_\d+\.Emit\(OpCodes\.)Ldloc_S, \k<var>\);
\1Call, m_M_\d+\);
""",
TestName = "Argument")]
[TestCase(/*language=C#*/
"class C { void M(int? p) => f = null; int ?f; }",
"""
//f = null
(\s+il_M_\d+\.Emit\(OpCodes\.)Ldarg_0\);
\1Ldflda, fld_f_\d+\);
\1Initobj, .+ImportReference\(typeof\(System.Nullable<>\)\).MakeGenericInstanceType\(.+Int32\)\);
""",
TestName = "Field")]
public void InitializingWithNull_EmitsCorrectCode(string code, string expectedSnippet)
{
var result = RunCecilifier(code);
Assert.That(result.GeneratedCode.ReadToEnd(), Does.Match(expectedSnippet));
}
}
16 changes: 12 additions & 4 deletions Cecilifier.Core/AST/ExpressionVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
using Cecilifier.Core.Extensions;
using Cecilifier.Core.Misc;
using Cecilifier.Core.Mappings;
using Cecilifier.Core.Naming;
using Cecilifier.Core.Variables;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
Expand Down Expand Up @@ -360,7 +359,6 @@ public override void VisitAssignmentExpression(AssignmentExpressionSyntax node)
if (HandlePseudoAssignment(node))
return;

// check if the left hand side of the assignment is a property (but not indexers) and handle that as a method (set) call.
var visitor = new AssignmentVisitor(Context, ilVar, node);

visitor.InstructionPrecedingValueToLoad = Context.CurrentLine;
Expand Down Expand Up @@ -584,14 +582,14 @@ public override void VisitArgument(ArgumentSyntax node)
// compute it's length)
var last = Context.CurrentLine;

using var nah = new NullLiteralArgumentDecorator(Context, node, ilVar);
base.VisitArgument(node);

node.Expression.InjectRequiredConversions(Context, ilVar, () =>
{
AddCecilExpression(last.Value.Replace("\t", string.Empty).Replace("\n", String.Empty));
});


StackallocAsArgumentFixer.Current?.StoreTopOfStackToLocalVariable(Context.SemanticModel.GetTypeInfo(node.Expression).Type);
}

Expand Down Expand Up @@ -1070,16 +1068,26 @@ private void ProcessPrefixPostfixOperators(ExpressionSyntax operand, OpCode opCo
operand.Accept(assignmentVisitor);
}

/*
* Normal assignments usually requires visiting the *right* node first (i.e, the value to be assigned) and
* then the *left* one (i.e, the target of the assignment)
*
* Some syntaxes (mostly involving value types) requires the order of visiting to be swapped, i.e, target of assignment
* first and then the value. One example of such cases is when assigning `default` to a value type (DateTime d = default;)
*/
private bool HandlePseudoAssignment(AssignmentExpressionSyntax node)
{
var lhsType = Context.SemanticModel.GetTypeInfo(node.Left).Type.EnsureNotNull();
var isSimpleIndexAccess = SymbolEqualityComparer.Default.Equals(lhsType.OriginalDefinition, Context.RoslynTypeSystem.SystemIndex)
&& node.Right.IsKind(SyntaxKind.IndexExpression)
&& !node.Left.IsKind(SyntaxKind.SimpleMemberAccessExpression);

var isNullAssignmentToNullableValueType = node.Right is LiteralExpressionSyntax { RawKind: (int) SyntaxKind.NullLiteralExpression }
&& SymbolEqualityComparer.Default.Equals(lhsType.OriginalDefinition, Context.RoslynTypeSystem.SystemNullableOfT);

var isRhsStructDefaultLiteralExpression = node.Right.IsKind(SyntaxKind.DefaultLiteralExpression) && lhsType.IsValueType;

if (!isSimpleIndexAccess && !isRhsStructDefaultLiteralExpression)
if (!isSimpleIndexAccess && !isRhsStructDefaultLiteralExpression && !isNullAssignmentToNullableValueType)
return false;

if (Context.SemanticModel.GetSymbolInfo(node.Left).Symbol is IParameterSymbol { RefKind: RefKind.Out or RefKind.Ref or RefKind.RefReadOnly })
Expand Down
56 changes: 56 additions & 0 deletions Cecilifier.Core/AST/NullLiteralArgumentDecorator.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
using System.Threading;
using Cecilifier.Core.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Mono.Cecil.Cil;

namespace Cecilifier.Core.AST;

/// <summary>
/// A `Disposable` type used to emit code to handle passing null as arguments to parameters typed as Nullable{T}
/// </summary>
/// <remarks>
/// Since Nullable{T} is a struct passing `null` as an argument the code needs to:
/// 1. Add a synthetic local variable
/// 2. Loads it address to stack
/// 3. Execute InitObj instruction
/// 4. Load the local variable to stack (which will be used as the argument value)
///
/// This type handles 1, 2 & 4.
/// </remarks>
internal ref struct NullLiteralArgumentDecorator
{
private string? _localVariableName;

Check warning on line 24 in Cecilifier.Core/AST/NullLiteralArgumentDecorator.cs

View workflow job for this annotation

GitHub Actions / RunTests

The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
private readonly IVisitorContext? _context;

Check warning on line 25 in Cecilifier.Core/AST/NullLiteralArgumentDecorator.cs

View workflow job for this annotation

GitHub Actions / RunTests

The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
private readonly string? _ilVar;

Check warning on line 26 in Cecilifier.Core/AST/NullLiteralArgumentDecorator.cs

View workflow job for this annotation

GitHub Actions / RunTests

The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.

public NullLiteralArgumentDecorator(IVisitorContext context, ArgumentSyntax node, string ilVar)
{
if (node.Expression is not LiteralExpressionSyntax { RawKind: (int) SyntaxKind.NullLiteralExpression } nullLiteralExpression)
return;

var argType = context.SemanticModel.GetTypeInfo(node.Expression).ConvertedType.EnsureNotNull();
if (!SymbolEqualityComparer.Default.Equals(argType.OriginalDefinition, context.RoslynTypeSystem.SystemNullableOfT))
return;

// we have a `null` being passed to a Nullable<T> parameter so we need to emit code
// for steps 1 & 2 as outlined in the remarks section above.
var local = context.AddLocalVariableToCurrentMethod("tmpNull", context.TypeResolver.Resolve(argType));
context.EmitCilInstruction(ilVar, OpCodes.Ldloca_S, local.VariableName);

_localVariableName = local.VariableName;
_context = context;
_ilVar = ilVar;
}

public void Dispose()
{
// Step 4 (see remarks in the method description)
string localVariable = Interlocked.Exchange(ref _localVariableName, null);
if ( localVariable != null)
{
_context!.EmitCilInstruction(_ilVar, OpCodes.Ldloc_S, localVariable);
}
}
}
5 changes: 4 additions & 1 deletion Cecilifier.Core/AST/StatementVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ private void ProcessVariableInitialization(VariableDeclaratorSyntax localVar, IT

// the same applies if...
var isDefaultLiteralExpressionOnNonPrimitiveValueType = localVar.Initializer.Value.IsKind(SyntaxKind.DefaultLiteralExpression) && variableType.IsValueType && !variableType.IsPrimitiveType();
if (isIndexExpression || isDefaultLiteralExpressionOnNonPrimitiveValueType)
var isNullAssignmentToNullableValueType = localVar.Initializer is EqualsValueClauseSyntax { Value: LiteralExpressionSyntax { RawKind: (int) SyntaxKind.NullLiteralExpression } }
&& SymbolEqualityComparer.Default.Equals(variableType.OriginalDefinition, Context.RoslynTypeSystem.SystemNullableOfT);

if (isIndexExpression || isDefaultLiteralExpressionOnNonPrimitiveValueType || isNullAssignmentToNullableValueType)
{
Context.EmitCilInstruction(_ilVar, OpCodes.Ldloca, localVarDef.VariableName);
}
Expand Down

0 comments on commit 475fc16

Please sign in to comment.