Skip to content

Out as return only #64318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jaredpar merged 28 commits into from
Sep 29, 2022
Merged

Out as return only #64318

Changes from 1 commit
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
7522f0d
Out change
jaredpar Sep 27, 2022
39ac635
More
jaredpar Sep 27, 2022
7e64550
more
jaredpar Sep 27, 2022
e2ef7a3
Fixed out
jaredpar Sep 27, 2022
1312bbb
finishing up
jaredpar Sep 27, 2022
2e18e89
feedback
jaredpar Sep 27, 2022
fadfefd
more
jaredpar Sep 27, 2022
d90757e
Clean up
jaredpar Sep 27, 2022
eee84e7
More
jaredpar Sep 27, 2022
4cee632
more
jaredpar Sep 27, 2022
ff1f999
PR feedback
jaredpar Sep 27, 2022
9f2605b
PR feedback
jaredpar Sep 27, 2022
d9608af
PR feedback
jaredpar Sep 27, 2022
1503cfc
PR feedback
jaredpar Sep 28, 2022
f4b2ce5
Refactor the EscapeLevel anaylsis
jaredpar Sep 28, 2022
6c70c9b
PR feedback
jaredpar Sep 28, 2022
a2007a4
PR feedback
jaredpar Sep 28, 2022
e11141d
PR feedback
jaredpar Sep 28, 2022
f68618c
In person feedback
jaredpar Sep 28, 2022
7d2605b
More
jaredpar Sep 28, 2022
28dd184
RSTE of all ref is return-only
jaredpar Sep 28, 2022
dd92bf2
Apply suggestions from code review
jaredpar Sep 28, 2022
99cc78a
more tests
jaredpar Sep 28, 2022
b663cb5
PR feedback
jaredpar Sep 28, 2022
3a4577d
PR feedback
jaredpar Sep 28, 2022
6e231a4
More
jaredpar Sep 29, 2022
5536fd1
Tests
jaredpar Sep 29, 2022
e20541e
formatting
jaredpar Sep 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
PR feedback
  • Loading branch information
@jaredpar
jaredpar committed Sep 27, 2022
commit 9f2605b91a04c837047c66c9025eda35b03d38ce
115 changes: 75 additions & 40 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Dynamic;
using Microsoft.CodeAnalysis.CSharp.CodeGen;
using Microsoft.CodeAnalysis.CSharp.Symbols;
Expand All @@ -21,12 +22,19 @@ namespace Microsoft.CodeAnalysis.CSharp
internal partial class Binder
{
#nullable enable

internal enum EscapeLevel : uint
{
CallingMethod = Binder.CallingMethodScope,
ReturnOnly = Binder.ReturnOnlyScope,
}

/// <summary>
/// This is an argument that needs to be considered for MAMM rules. It's at least an
/// output argument of the function and very often also an input. Essentially this
/// represents all the writable ref and out arguments.
/// </summary>
internal readonly struct MixableArgument
internal readonly struct MixableDestination
{
internal BoundExpression Argument { get; }

Expand All @@ -37,14 +45,35 @@ internal readonly struct MixableArgument
/// </summary>
internal ParameterSymbol? Parameter { get; }

internal bool IsOut => Parameter is { RefKind: RefKind.Out };
/// <summary>
/// This destination can only be written to by arguments that have at least
/// this escape level. An destination that is <see cref="EscapeLevel.CallingMethod"/>
/// can never be written to by an argument that has a level of <see cref="EscapeLevel.ReturnOnly"/>
/// </summary>
internal EscapeLevel EscapeLevel { get; }

internal MixableArgument(ParameterSymbol? parameter, BoundExpression argument)
internal MixableDestination(ParameterSymbol parameter, BoundExpression argument)
{
Debug.Assert(parameter.RefKind.IsWritableReference());
Argument = argument;
Parameter = parameter;
EscapeLevel = parameter.RefKind == RefKind.Out ? EscapeLevel.ReturnOnly : EscapeLevel.CallingMethod;
}

internal MixableDestination(BoundExpression argument, EscapeLevel escapeLevel)
{
Argument = argument;
Parameter = null;
EscapeLevel = escapeLevel;
}

internal bool IsApplicableTo(EscapeLevel level) => EscapeLevel switch
{
EscapeLevel.CallingMethod => level == EscapeLevel.CallingMethod,
EscapeLevel.ReturnOnly => true,
_ => throw ExceptionUtilities.UnexpectedValue(EscapeLevel)
};

public override string? ToString() => Parameter is { } p
? p.ToString()
: Argument.ToString();
Expand Down Expand Up @@ -86,13 +115,6 @@ public void Deconstruct(out ParameterSymbol? parameter, out BoundExpression argu
: Argument.ToString();
}

internal enum EscapeKind
{
Value,
RefCallingMethod,
RefReturnOnly,
}

/// <summary>
/// Represents a value being analyzed for escape analysis purposes. This represents the value
/// as it contributes to escape analysis which means arguments can show up multiple times. For
Expand All @@ -107,22 +129,36 @@ internal readonly struct EscapeValue

internal BoundExpression Argument { get; }

internal EscapeKind EscapeKind { get; }
/// <summary>
/// This differentiates whethere the value can only escape to <see cref="EscapeLevel.CallingMethod"/>
/// locations or to any return spot.
/// </summary>
internal EscapeLevel EscapeLevel { get; }

internal bool IsRefEscape => EscapeKind is EscapeKind.RefReturnOnly or EscapeKind.RefCallingMethod;
internal bool IsRefEscape { get; }

internal EscapeValue(ParameterSymbol? parameter, BoundExpression argument, EscapeKind escapeKind)
internal EscapeValue(ParameterSymbol parameter, BoundExpression argument, EscapeLevel escapeLevel, bool isRefEscape)
{
Argument = argument;
Parameter = parameter;
EscapeKind = escapeKind;
EscapeLevel = escapeLevel;
IsRefEscape = isRefEscape;
}

internal EscapeValue(BoundExpression argument, EscapeLevel escapeLevel, bool isRefEscape)
{
Argument = argument;
Parameter = null;
EscapeLevel = escapeLevel;
IsRefEscape = false;
}

public void Deconstruct(out ParameterSymbol? parameter, out BoundExpression argument, out EscapeKind escapeKind)
public void Deconstruct(out ParameterSymbol? parameter, out BoundExpression argument, out EscapeLevel escapeLevel, out bool isRefEscape)
{
parameter = Parameter;
argument = Argument;
escapeKind = EscapeKind;
escapeLevel = EscapeLevel;
isRefEscape = IsRefEscape;
}

public override string? ToString() => Parameter is { } p
Expand Down Expand Up @@ -1877,7 +1913,7 @@ private static void GetInvocationArgumentsForEscape(
ImmutableArray<RefKind> argRefKindsOpt,
ImmutableArray<int> argsToParamsOpt,
bool ignoreArglistRefKinds,
ArrayBuilder<MixableArgument>? mixableArguments,
ArrayBuilder<MixableDestination>? mixableArguments,
ArrayBuilder<EscapeArgument> escapeArguments)
{
if (receiver is { })
Expand All @@ -1887,7 +1923,7 @@ private static void GetInvocationArgumentsForEscape(

if (mixableArguments is not null && isMixableParameter(tuple.Parameter))
{
mixableArguments.Add(new MixableArgument(tuple.Parameter, receiver));
mixableArguments.Add(new MixableDestination(tuple.Parameter, receiver));
}
}

Expand Down Expand Up @@ -1915,7 +1951,7 @@ private static void GetInvocationArgumentsForEscape(

if (mixableArguments is not null && isMixableParameter(parameter))
{
mixableArguments.Add(new MixableArgument(parameter, argument));
mixableArguments.Add(new MixableDestination(parameter, argument));
}

var refKind = parameter?.RefKind ?? RefKind.None;
Expand All @@ -1933,7 +1969,7 @@ private static void GetInvocationArgumentsForEscape(
}
}

static bool isMixableParameter(ParameterSymbol? parameter) =>
static bool isMixableParameter([NotNullWhen(true)] ParameterSymbol? parameter) =>
parameter is not null &&
parameter.Type.IsRefLikeType &&
parameter.RefKind.IsWritableReference();
Expand Down Expand Up @@ -1966,7 +2002,7 @@ static EscapeArgument getReceiver(Symbol symbol, BoundExpression receiver)
static void getArgList(
ImmutableArray<BoundExpression> argsOpt,
ImmutableArray<RefKind> argRefKindsOpt,
ArrayBuilder<MixableArgument>? mixableArguments,
ArrayBuilder<MixableDestination>? mixableArguments,
ArrayBuilder<EscapeArgument> escapeArguments)
{
for (int argIndex = 0; argIndex < argsOpt.Length; argIndex++)
Expand All @@ -1977,7 +2013,7 @@ static void getArgList(

if (refKind == RefKind.Ref && mixableArguments is not null)
Copy link
Contributor

@cston cston Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refKind

Do we need to check if argument is a ref struct? #Closed

Copy link
Member Author

@jaredpar jaredpar Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For better or worse this layer returns all arguments no matter if they are ref struct or not. Higher layers filter out the values that aren't going to contribute to escape analysis. I was loath to make a change this deep just now given our time constraints.

{
mixableArguments.Add(new MixableArgument(parameter: null, argument));
mixableArguments.Add(new MixableDestination(argument, EscapeLevel.CallingMethod));
}
}
}
Expand Down Expand Up @@ -2070,7 +2106,7 @@ private void GetFilteredArguments(
ImmutableArray<RefKind> argRefKindsOpt,
ImmutableArray<int> argsToParamsOpt,
bool ignoreArglistRefKinds,
ArrayBuilder<MixableArgument>? mixableArguments,
ArrayBuilder<MixableDestination>? mixableArguments,
ArrayBuilder<EscapeValue> escapeValues)
{
if (!symbol.RequiresInstanceReceiver())
Expand All @@ -2093,17 +2129,17 @@ private void GetFilteredArguments(

foreach (var (parameter, argument, refKind) in escapeArguments)
{
// This means it's part of an __arglist or function pointer receiver.
Copy link
Member

@RikkiGibson RikkiGibson Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused as to why function pointer receiver is a scenario here. Isn't the receiver just the function pointer itself? It seems like nothing can escape into it or out of it.

Copy link
Member Author

@jaredpar jaredpar Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was attempting to maintain the original code here. I'm also unsure why a function pointer could come into play here. Not sure you can really do those on a ref struct. I will try removing and see what happens.

if (parameter is null)
{
// This means it's part of an __arglist or function pointer receiver.
if (refKind != RefKind.None)
{
escapeValues.Add(new EscapeValue(parameter: null, argument, EscapeKind.RefReturnOnly));
escapeValues.Add(new EscapeValue(argument, EscapeLevel.ReturnOnly, isRefEscape: true));
}

if (argument.Type?.IsRefLikeType == true)
{
escapeValues.Add(new EscapeValue(parameter: null, argument, EscapeKind.Value));
escapeValues.Add(new EscapeValue(argument, EscapeLevel.CallingMethod, isRefEscape: false));
}

continue;
Expand All @@ -2113,18 +2149,18 @@ private void GetFilteredArguments(
{
if (parameter.EffectiveScope == DeclarationScope.Unscoped)
{
var kind = parameter.Type.IsRefLikeType ? EscapeKind.RefReturnOnly : EscapeKind.RefCallingMethod;
escapeValues.Add(new EscapeValue(parameter, argument, kind));
var kind = parameter.Type.IsRefLikeType ? EscapeLevel.ReturnOnly : EscapeLevel.CallingMethod;
escapeValues.Add(new EscapeValue(parameter, argument, kind, isRefEscape: true));
}

if (parameter.RefKind != RefKind.Out && parameter.Type.IsRefLikeType)
{
escapeValues.Add(new EscapeValue(parameter, argument, EscapeKind.Value));
escapeValues.Add(new EscapeValue(parameter, argument, EscapeLevel.CallingMethod, isRefEscape: false));
}
}
else if (parameter.Type.IsRefLikeType && parameter.EffectiveScope == DeclarationScope.Unscoped)
{
escapeValues.Add(new EscapeValue(parameter, argument, EscapeKind.Value));
escapeValues.Add(new EscapeValue(parameter, argument, EscapeLevel.CallingMethod, isRefEscape: false));
}
}

Expand Down Expand Up @@ -2275,7 +2311,7 @@ private bool CheckInvocationArgMixingWithUpdatedRules(
uint scopeOfTheContainingExpression,
BindingDiagnosticBag diagnostics)
{
var mixableArguments = ArrayBuilder<MixableArgument>.GetInstance();
var mixableArguments = ArrayBuilder<MixableDestination>.GetInstance();
var escapeValues = ArrayBuilder<EscapeValue>.GetInstance();
GetFilteredArguments(
symbol,
Expand All @@ -2292,22 +2328,21 @@ private bool CheckInvocationArgMixingWithUpdatedRules(
foreach (var mixableArg in mixableArguments)
{
var toArgEscape = GetValEscape(mixableArg.Argument, scopeOfTheContainingExpression);
foreach (var (fromParameter, fromArg, escapeKind) in escapeValues)
foreach (var (fromParameter, fromArg, escapeKind, isRefEscape) in escapeValues)
{
if (mixableArg.Parameter is not null && object.ReferenceEquals(mixableArg.Parameter, fromParameter))
{
continue;
}

valid = escapeKind switch
if (!mixableArg.IsApplicableTo(escapeKind))
{
EscapeKind.RefReturnOnly => mixableArg.IsOut
? CheckRefEscape(fromArg.Syntax, fromArg, scopeOfTheContainingExpression, toArgEscape, checkingReceiver: false, diagnostics)
: true,
EscapeKind.RefCallingMethod => CheckRefEscape(fromArg.Syntax, fromArg, scopeOfTheContainingExpression, toArgEscape, checkingReceiver: false, diagnostics),
EscapeKind.Value => CheckValEscape(fromArg.Syntax, fromArg, scopeOfTheContainingExpression, toArgEscape, checkingReceiver: false, diagnostics),
_ => throw ExceptionUtilities.UnexpectedValue(escapeKind),
};
continue;
}

valid = isRefEscape
? CheckRefEscape(fromArg.Syntax, fromArg, scopeOfTheContainingExpression, toArgEscape, checkingReceiver: false, diagnostics)
: CheckValEscape(fromArg.Syntax, fromArg, scopeOfTheContainingExpression, toArgEscape, checkingReceiver: false, diagnostics);

if (!valid)
{
Expand Down