Skip to content

Commit

Permalink
fix extra ldarg_0 instruction emmited on member access through condit…
Browse files Browse the repository at this point in the history
…ional expressions (#208)

Member access through conditional expressions were being considered to be equivalent to an
unqualified access which requires 'this' to be implicitly loaded leading to extra 'ldlarg_0'
to be emmited.
  • Loading branch information
adrianoc committed Aug 18, 2023
1 parent f5dfb34 commit 0f1701a
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 26 deletions.
19 changes: 19 additions & 0 deletions Cecilifier.Core.Tests/Tests/Unit/OperatorsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,25 @@ public void TestNullConditionalOperatorOnStorageTargets(string target, Code expe
@"il_M_3.Append\(lbl_conditionEnd_7\);"));
}

[TestCase("f", "Ldfld, fld_f_6", TestName = "Field")]
[TestCase("P", "Call, m_get_2", TestName = "Property")]
[TestCase("M()", "Call, m_M_4", TestName = "Method")]
public void TestNullConditionalOperator_OnQualifiedMemberAccess_DoesNoLoadThis(string member, string expected)
{
var result = RunCecilifier($$"""class Foo { private int P => 0; private int M() => 0; private int f; object Bar(Foo p) => p?.{{member}}; }""");
Assert.That(
result.GeneratedCode.ReadToEnd(),
Does.Match(
$"""
(il_bar_\d+\.Emit\(OpCodes\.)Ldarg_1\);
(\s+\1){expected}\);
\2Newobj, .+ImportReference\(.+ResolveMethod\(typeof\(System\.Nullable<System.Int32>\), ".ctor",.+, "System.Int32"\)\)\);
\s+il_bar_\d+.Append\(lbl_conditionEnd_\d+\);
\2Box,.+ImportReference\(typeof\(System.Nullable<>\)\)\.MakeGenericInstanceType\(assembly.MainModule.TypeSystem.Int32\)\);
\2Ret\);
"""));
}

[TestCase(
"int i = 42; i -= 10;",
@"(il_topLevelMain_\d\.Emit\(OpCodes\.)Ldc_I4, 42\);\s+\1Stloc, (l_i_\d+)\);\s+\1Ldloc, \2\);\s+\1Ldc_I4, 10\);\s+\1Sub\);\s+\1Stloc, \2\);\s+m_topLevelStatements_1\.Body\.Instructions\.Add\(.+OpCodes\.Ret\)\);",
Expand Down
25 changes: 1 addition & 24 deletions Cecilifier.Core/AST/ExpressionVisitor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1175,7 +1175,7 @@ private void ProcessProperty(SimpleNameSyntax node, IPropertySymbol propertySymb
propertySymbol.EnsurePropertyExists(Context, node);

var isAccessOnThisOrObjectCreation = node.Parent.IsAccessOnThisOrObjectCreation();
if (IsUnqualifiedAccess(node, propertySymbol))
if (!propertySymbol.IsStatic && !node.IsQualifiedAccessToTypeOrMember())
{
// if this is an *unqualified* access we need to load *this*
Context.EmitCilInstruction(ilVar, OpCodes.Ldarg_0);
Expand Down Expand Up @@ -1212,29 +1212,6 @@ private void HandlePropertyGetAccess(SimpleNameSyntax node, IPropertySymbol prop
}
}

/// <summary>
/// Checks whether the given <paramref name="node"/> represents an unqualified reference (i.e, only a type name) or
/// a qualified one. For instance in `Foo f = new NS.Foo();`,
/// 1. Foo => Unqualified
/// 2. NS.Foo => Qualified
///
/// </summary>
/// <param name="node">Node with the name of the property being accessed.</param>
/// <param name="propertySymbol">Property symbol representing the property being accessed.</param>
/// <returns>true if <paramref name="node"/> represents an unqualified access to <paramref name="propertySymbol"/>, false otherwise</returns>
/// <remarks>
/// A NameColon syntax represents the `Length: 42` in an expression like `o as string { Length: 42 }`
/// A MemberBindExpression represents the null coalescing operator
/// </remarks>
private static bool IsUnqualifiedAccess(SimpleNameSyntax node, IPropertySymbol propertySymbol)
{
var parentMae = node.Parent as MemberAccessExpressionSyntax;
return (parentMae == null || parentMae.Expression == node)
&& !node.Parent.IsKind(SyntaxKind.NameColon)
&& !node.Parent.IsKind(SyntaxKind.MemberBindingExpression)
&& !propertySymbol.IsStatic;
}

private void ProcessMethodReference(SimpleNameSyntax node, IMethodSymbol method)
{
var invocationParent = node.Ancestors().OfType<InvocationExpressionSyntax>()
Expand Down
3 changes: 1 addition & 2 deletions Cecilifier.Core/AST/SyntaxWalkerBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -499,8 +499,7 @@ protected void ProcessField(string ilVar, SimpleNameSyntax node, IFieldSymbol fi

var fieldDeclarationVariable = fieldSymbol.EnsureFieldExists(Context, node);

var isTargetOfQualifiedAccess = (node.Parent is MemberAccessExpressionSyntax mae) && mae.Name == node;
if (!fieldSymbol.IsStatic && !isTargetOfQualifiedAccess)
if (!fieldSymbol.IsStatic && !node.IsQualifiedAccessToTypeOrMember())
Context.EmitCilInstruction(ilVar, OpCodes.Ldarg_0);

if (HandleLoadAddressOnStorage(ilVar, fieldSymbol.Type, node, fieldSymbol.IsStatic ? OpCodes.Ldsflda : OpCodes.Ldflda, fieldSymbol.Name, VariableMemberKind.Field, fieldSymbol.ContainingType.ToDisplayString()))
Expand Down
18 changes: 18 additions & 0 deletions Cecilifier.Core/Extensions/SyntaxNodeExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,24 @@ namespace Cecilifier.Core.Extensions
{
public static class SyntaxNodeExtensions
{
/// <summary>
/// Checks whether the given <paramref name="node"/> is the name in a qualified reference.
/// </summary>
/// <param name="node">Node with the name of the type/member to be tested.</param>
/// <returns>true if <paramref name="node"/> is the name in a qualified access, false otherwise</returns>
/// <remarks>
/// Examples of qualified / unqualified access:
/// 1. in, `Foo f = new NS.Foo();`, `Foo` : Foo f => Unqualified, NS.Foo => Qualified
/// 2. `o.field ?? otherField`, otherField => Unqualified, `field` in `o.field` => Qualified
/// </remarks>
public static bool IsQualifiedAccessToTypeOrMember(this SimpleNameSyntax node) => node.Parent switch
{
MemberAccessExpressionSyntax mae => mae.Name == node,
MemberBindingExpressionSyntax mbe => mbe.Name == node, // A MemberBindExpression represents `?.` in the null conditional operator, for instance, `o?.member`
NameColonSyntax => true, // A NameColon syntax represents the `Length: 42` in an expression like `o as string { Length: 42 }`. In this case, `Length` is equivalent to `o.Length`
_ => false
};

/// <summary>
/// Returns a human readable summary of the <paramref name="node"/> containing nodes/tokens until (including) first one with a new line trivia.
/// </summary>
Expand Down

0 comments on commit 0f1701a

Please sign in to comment.