Skip to content
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.

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

Out arguments do not contribute val escape #64784

Merged
merged 4 commits into from
Nov 28, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
11 changes: 8 additions & 3 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2167,7 +2167,7 @@ private void GetEscapeValuesForUpdatedRules(
continue;
}

if (parameter.Type.IsRefLikeType && GetParameterValEscapeLevel(parameter) is { } valEscapeLevel)
if (parameter.Type.IsRefLikeType && parameter.RefKind != RefKind.Out && GetParameterValEscapeLevel(parameter) is { } valEscapeLevel)
{
escapeValues.Add(new EscapeValue(parameter, argument, valEscapeLevel, isRefEscape: false));
}
Expand Down Expand Up @@ -2417,6 +2417,11 @@ private bool CheckInvocationArgMixingWithUpdatedRules(

void inferDeclarationExpressionValEscape()
{
if (argsOpt.IsDefault)
{
return;
}

// find the widest scope that arguments could safely escape to.
// use this scope as the inferred STE of declaration expressions.
var inferredDestinationValEscape = CallingMethodScope;
Expand All @@ -2427,9 +2432,9 @@ void inferDeclarationExpressionValEscape()
: GetValEscape(fromArg, scopeOfTheContainingExpression));
}

foreach (var (_, fromArg, _, _) in escapeValues)
foreach (var argument in argsOpt)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since out arguments are usually not included in escapeValues, we needed to change to check argsOpt instead. Note that escapeValues was probably never the right collection to enumerate, since the same argument can be present multiple times, it just happened to work coincidentally until now.

Copy link
Contributor Author

@RikkiGibson RikkiGibson Nov 22, 2022

Choose a reason for hiding this comment

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

Also, although it doesn't look like argsOpt is default-checked on this code path, I actually think that the argument passed for argsOpt is never default. I'll send a separate PR after this one to add an assertion and rename the parameter.

{
if (ShouldInferDeclarationExpressionValEscape(fromArg, out var localSymbol))
if (ShouldInferDeclarationExpressionValEscape(argument, out var localSymbol))
{
localSymbol.SetValEscape(inferredDestinationValEscape);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6416,5 +6416,77 @@ static RS M22(ref RS rs5)
Diagnostic(ErrorCode.ERR_EscapeVariable, "rs6").WithArguments("rs6").WithLocation(45, 16)
);
}

[Fact, WorkItem(64783, "https://github.com/dotnet/roslyn/issues/64783")]
public void OutArgumentsDoNotContributeValEscape_01()
{
var source = """
using System;

class Program
{
static Span<byte> M1()
{
Span<byte> a = stackalloc byte[42];
var ret = OneOutSpanReturnsSpan(out a);
return ret;
}

static Span<byte> M2()
{
Span<byte> a = stackalloc byte[42];
TwoOutSpans(out a, out Span<byte> b);
return b;
}

static Span<byte> OneOutSpanReturnsSpan(out Span<byte> a)
{
// 'return a' is illegal until it is overwritten
a = default;
return default;
}

static void TwoOutSpans(out Span<byte> a, out Span<byte> b)
{
// 'a = b' and 'b = a' are illegal until one has already been written
a = b = default;
}
}

""";

var comp = CreateCompilationWithSpan(source);
comp.VerifyDiagnostics();
}

[Fact, WorkItem(56587, "https://github.com/dotnet/roslyn/issues/56587")]
public void OutArgumentsDoNotContributeValEscape_02()
{
// Test that out discard arguments are not treated as inputs.
// This means we don't need to take special care to zero-out the variable used for a discard argument between uses.
var source = """
using System;

class Program
{
static Span<byte> M1()
{
Span<byte> a = stackalloc byte[42];
TwoOutSpans(out a, out _);
TwoOutSpans(out _, out Span<byte> c);
return c;
}

static void TwoOutSpans(out Span<byte> a, out Span<byte> b)
{
// 'a = b' and 'b = a' are illegal until one has already been written
a = b = default;
}
}
""";

var comp = CreateCompilationWithSpan(source);
comp.VerifyDiagnostics();
}
}
}