Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Microsoft.CodeAnalysis.CSharp.UseAutoProperty;
Expand Down Expand Up @@ -2999,4 +3000,62 @@ class Class
readonly ref int P => ref i;
}
""");

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77011")]
public Task TestRemoveThisIfPreferredCodeStyle()
=> TestInRegularAndScriptAsync(
"""
class C
{
[|private readonly string a;|]

public C(string a)
{
this.a = a;
}

public string A => a;
}
""",
"""
class C
{
public C(string a)
{
A = a;
}

public string A { get; }
}
""",
options: Option(CodeStyleOptions2.QualifyPropertyAccess, false, NotificationOption2.Error));

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/77011")]
public Task TestKeepThisIfPreferredCodeStyle()
=> TestInRegularAndScriptAsync(
"""
class C
{
[|private readonly string a;|]

public C(string a)
{
this.a = a;
}

public string A => a;
}
""",
"""
class C
{
public C(string a)
{
this.A = a;
}

public string A { get; }
}
""",
options: Option(CodeStyleOptions2.QualifyPropertyAccess, true, NotificationOption2.Error));
}
114 changes: 114 additions & 0 deletions src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_RefSafety.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using Microsoft.CodeAnalysis.CSharp.Symbols;

namespace Microsoft.CodeAnalysis.CSharp.CodeGen;

internal partial class CodeGenerator
{
/// <inheritdoc cref="MightEscapeTemporaryRefs(bool, TypeSymbol, RefKind, ParameterSymbol?, ImmutableArray{ParameterSymbol})"/>
private static bool MightEscapeTemporaryRefs(BoundCall node, bool used)
{
return MightEscapeTemporaryRefs(
used: used,
returnType: node.Type,
returnRefKind: node.Method.RefKind,
thisParameterSymbol: node.Method.TryGetThisParameter(out var thisParameter) ? thisParameter : null,
parameters: node.Method.Parameters);
}

/// <inheritdoc cref="MightEscapeTemporaryRefs(bool, TypeSymbol, RefKind, ParameterSymbol?, ImmutableArray{ParameterSymbol})"/>
private static bool MightEscapeTemporaryRefs(BoundObjectCreationExpression node, bool used)
{
return MightEscapeTemporaryRefs(
used: used,
returnType: node.Type,
returnRefKind: RefKind.None,
thisParameterSymbol: null,
parameters: node.Constructor.Parameters);
}

/// <inheritdoc cref="MightEscapeTemporaryRefs(bool, TypeSymbol, RefKind, ParameterSymbol?, ImmutableArray{ParameterSymbol})"/>
private static bool MightEscapeTemporaryRefs(BoundFunctionPointerInvocation node, bool used)
{
FunctionPointerMethodSymbol method = node.FunctionPointer.Signature;
return MightEscapeTemporaryRefs(
used: used,
returnType: node.Type,
returnRefKind: method.RefKind,
thisParameterSymbol: null,
parameters: method.Parameters);
}

/// <summary>
/// Determines whether a 'ref' can be captured by the call (considering only its signature).
/// </summary>
/// <remarks>
/// <para>
/// The emit layer consults this to avoid reusing temporaries that are passed by ref to such methods.
/// </para>
/// <para>
/// This is a heuristic which might have false positives, i.e.,
/// it might not recognize that a call cannot capture a 'ref'
/// even if the binding-time ref safety analysis would recognize that.
/// That is fine, the IL will be correct, albeit less optimal (it might use more temps).
/// But we still want some heuristic to avoid regressing IL of common safe cases.
/// </para>
/// </remarks>
private static bool MightEscapeTemporaryRefs(
bool used,
TypeSymbol returnType,
RefKind returnRefKind,
ParameterSymbol? thisParameterSymbol,
ImmutableArray<ParameterSymbol> parameters)
{
// whether we have any outputs that can capture `ref`s
bool anyRefTargets = false;
// whether we have any inputs that can contain `ref`s
bool anyRefSources = false;

if (used && (returnRefKind != RefKind.None || returnType.IsRefLikeOrAllowsRefLikeType()))
{
// If returning by ref or returning a ref struct, the result might capture `ref`s.
anyRefTargets = true;
}

if (thisParameterSymbol is not null && processParameter(thisParameterSymbol, anyRefSources: ref anyRefSources, anyRefTargets: ref anyRefTargets))
{
return true;
}

foreach (var parameter in parameters)
{
if (processParameter(parameter, anyRefSources: ref anyRefSources, anyRefTargets: ref anyRefTargets))
{
return true;
}
}

return false;

// Returns true if we can return 'true' early.
static bool processParameter(ParameterSymbol parameter, ref bool anyRefSources, ref bool anyRefTargets)
{
if (parameter.Type.IsRefLikeOrAllowsRefLikeType() && parameter.EffectiveScope != ScopedKind.ScopedValue)
{
anyRefSources = true;
if (parameter.RefKind.IsWritableReference())
{
anyRefTargets = true;
}
}
else if (parameter.RefKind != RefKind.None && parameter.EffectiveScope == ScopedKind.None)
{
anyRefSources = true;
}

// If there is at least one output and at least one input, a `ref` can be captured.
return anyRefTargets && anyRefSources;
}
}
}
39 changes: 37 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1660,7 +1660,13 @@ private void EmitStaticCallExpression(BoundCall call, UseKind useKind)

Debug.Assert(method.IsStatic);

var countBefore = _builder.LocalSlotManager.StartScopeOfTrackingAddressedLocals();

EmitArguments(arguments, method.Parameters, call.ArgumentRefKindsOpt);

_builder.LocalSlotManager.EndScopeOfTrackingAddressedLocals(countBefore,
MightEscapeTemporaryRefs(call, used: useKind != UseKind.Unused));

int stackBehavior = GetCallStackBehavior(method, arguments);

if (method.IsAbstract || method.IsVirtual)
Expand Down Expand Up @@ -1690,6 +1696,8 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
bool box;
LocalDefinition tempOpt;

var countBefore = _builder.LocalSlotManager.StartScopeOfTrackingAddressedLocals();

if (receiverIsInstanceCall(call, out BoundCall nested))
{
var calls = ArrayBuilder<BoundCall>.GetInstance();
Expand Down Expand Up @@ -1755,6 +1763,11 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
}

emitArgumentsAndCallEpilogue(call, callKind, receiverUseKind);

_builder.LocalSlotManager.EndScopeOfTrackingAddressedLocals(countBefore, MightEscapeTemporaryRefs(call, used: true));

countBefore = _builder.LocalSlotManager.StartScopeOfTrackingAddressedLocals();

FreeOptTemp(tempOpt);
tempOpt = null;

Expand Down Expand Up @@ -1815,6 +1828,9 @@ private void EmitInstanceCallExpression(BoundCall call, UseKind useKind)
}

emitArgumentsAndCallEpilogue(call, callKind, useKind);

_builder.LocalSlotManager.EndScopeOfTrackingAddressedLocals(countBefore, MightEscapeTemporaryRefs(call, used: useKind != UseKind.Unused));

FreeOptTemp(tempOpt);

return;
Expand Down Expand Up @@ -2446,8 +2462,14 @@ private void EmitObjectCreationExpression(BoundObjectCreationExpression expressi
}

// none of the above cases, so just create an instance

var countBefore = _builder.LocalSlotManager.StartScopeOfTrackingAddressedLocals();

EmitArguments(expression.Arguments, constructor.Parameters, expression.ArgumentRefKindsOpt);

_builder.LocalSlotManager.EndScopeOfTrackingAddressedLocals(countBefore,
MightEscapeTemporaryRefs(expression, used));

var stackAdjustment = GetObjCreationStackBehavior(expression);
_builder.EmitOpCode(ILOpCode.Newobj, stackAdjustment);

Expand Down Expand Up @@ -2572,8 +2594,7 @@ private void EmitAssignmentExpression(BoundAssignmentOperator assignmentOperator
private bool TryEmitAssignmentInPlace(BoundAssignmentOperator assignmentOperator, bool used)
{
// If the left hand is itself a ref, then we can't use in-place assignment
// because we need to spill the creation. This code can't be written in C#, but
// can be produced by lowering.
// because we need to spill the creation.
if (assignmentOperator.IsRef)
{
return false;
Expand Down Expand Up @@ -2704,7 +2725,14 @@ private bool TryInPlaceCtorCall(BoundExpression target, BoundObjectCreationExpre
Debug.Assert(temp == null, "in-place ctor target should not create temps");

var constructor = objCreation.Constructor;

var countBefore = _builder.LocalSlotManager.StartScopeOfTrackingAddressedLocals();

EmitArguments(objCreation.Arguments, constructor.Parameters, objCreation.ArgumentRefKindsOpt);

_builder.LocalSlotManager.EndScopeOfTrackingAddressedLocals(countBefore,
MightEscapeTemporaryRefs(objCreation, used));

// -2 to adjust for consumed target address and not produced value.
var stackAdjustment = GetObjCreationStackBehavior(objCreation) - 2;
_builder.EmitOpCode(ILOpCode.Call, stackAdjustment);
Expand Down Expand Up @@ -4026,7 +4054,14 @@ private void EmitCalli(BoundFunctionPointerInvocation ptrInvocation, UseKind use
}

FunctionPointerMethodSymbol method = ptrInvocation.FunctionPointer.Signature;

var countBefore = _builder.LocalSlotManager.StartScopeOfTrackingAddressedLocals();

EmitArguments(ptrInvocation.Arguments, method.Parameters, ptrInvocation.ArgumentRefKindsOpt);

_builder.LocalSlotManager.EndScopeOfTrackingAddressedLocals(countBefore,
MightEscapeTemporaryRefs(ptrInvocation, used: useKind != UseKind.Unused));

var stackBehavior = GetCallStackBehavior(ptrInvocation.FunctionPointer.Signature, ptrInvocation.Arguments);

if (temp is object)
Expand Down
12 changes: 11 additions & 1 deletion src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -991,13 +991,23 @@ public override BoundNode VisitAssignmentOperator(BoundAssignmentOperator node)
// Special Case: If the RHS is a pointer conversion, then the assignment functions as
// a conversion (because the RHS will actually be typed as a native u/int in IL), so
// we should not optimize away the local (i.e. schedule it on the stack).
if (CanScheduleToStack(localSymbol) &&
else if (CanScheduleToStack(localSymbol) &&
assignmentLocal.Type.IsPointerOrFunctionPointer() && right.Kind == BoundKind.Conversion &&
((BoundConversion)right).ConversionKind.IsPointerConversion())
{
ShouldNotSchedule(localSymbol);
}

// If this is a pointer-to-ref assignment, keep the local so GC knows to re-track it.
// We don't need to do this for implicitly synthesized locals because working with pointers in an unsafe context does not guarantee any GC tracking,
// but when a pointer is converted to a user-defined ref local, it becomes a use of a "safe" feature where we should guarantee the ref is tracked by GC.
else if (localSymbol.RefKind != RefKind.None &&
localSymbol.SynthesizedKind == SynthesizedLocalKind.UserDefined &&
right.Kind == BoundKind.PointerIndirectionOperator)
{
ShouldNotSchedule(localSymbol);
}

RecordVarWrite(localSymbol);
assignmentLocal = null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,20 @@ public override ImmutableArray<ParameterSymbol> Parameters
}
}

internal override bool TryGetThisParameter(out ParameterSymbol? thisParameter)
{
if (UnderlyingMethod.TryGetThisParameter(out ParameterSymbol? underlyingThisParameter))
{
thisParameter = underlyingThisParameter != null
? new ThisParameterSymbol(this, _container)
: null;
return true;
}

thisParameter = null;
return false;
}

public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations => ImmutableArray<MethodSymbol>.Empty;

public override ImmutableArray<CustomModifier> RefCustomModifiers => UnderlyingMethod.RefCustomModifiers;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,8 @@ static ref readonly int M(in int x)
// Code size 72 (0x48)
.maxstack 3
.locals init (int V_0,
int V_1)
int V_1,
int V_2)
IL_0000: ldc.i4.s 42
IL_0002: stloc.0
IL_0003: ldloca.s V_0
Expand All @@ -870,22 +871,22 @@ .locals init (int V_0,
IL_000b: call ""void System.Console.WriteLine(int)""
IL_0010: newobj ""Program..ctor()""
IL_0015: ldc.i4.5
IL_0016: stloc.0
IL_0017: ldloca.s V_0
IL_0016: stloc.1
IL_0017: ldloca.s V_1
IL_0019: ldc.i4.6
IL_001a: stloc.1
IL_001b: ldloca.s V_1
IL_001a: stloc.2
IL_001b: ldloca.s V_2
IL_001d: call ""int Program.this[in int, in int].get""
IL_0022: call ""void System.Console.WriteLine(int)""
IL_0027: ldc.i4.s 42
IL_0029: stloc.0
IL_002a: ldloca.s V_0
IL_0029: stloc.1
IL_002a: ldloca.s V_1
IL_002c: call ""ref readonly int Program.M(in int)""
IL_0031: ldind.i4
IL_0032: call ""void System.Console.WriteLine(int)""
IL_0037: ldc.i4.s 42
IL_0039: stloc.0
IL_003a: ldloca.s V_0
IL_0039: stloc.2
IL_003a: ldloca.s V_2
IL_003c: call ""ref readonly int Program.M(in int)""
IL_0041: ldind.i4
IL_0042: call ""void System.Console.WriteLine(int)""
Expand Down
Loading
Loading