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.

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

Don't check safe-to-escape of receiver arguments to readonly members #35597

Merged
merged 5 commits into from
May 21, 2019
Merged

Don't check safe-to-escape of receiver arguments to readonly members #35597

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4d1bbd0
Allow readonly methods to accept span parameters from stackalloc
RikkiGibson May 9, 2019
13d102c
Cleanup
RikkiGibson May 9, 2019
c129754
Add relevant spec language to CheckInvocationArgMixing
RikkiGibson May 14, 2019
8bf11e2
isReadOnlyInvocation -> isReceiverRefReadOnly
RikkiGibson May 14, 2019
31a03bf
Move isReceiverRefReadOnly to local function
RikkiGibson May 17, 2019
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
19 changes: 18 additions & 1 deletion 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 @@ -1322,6 +1322,11 @@ private static bool CheckInvocationArgMixing(
uint scopeOfTheContainingExpression,
DiagnosticBag diagnostics)
{
// SPEC:
// In a method invocation, the following constraints apply:
// - If there is a ref or out argument of a ref struct type (including the receiver), with safe-to-escape E1, then
// - no argument (including the receiver) may have a narrower safe-to-escape than E1.

if (symbol.IsStatic)
{
// ignore receiver when symbol is static
Expand All @@ -1331,9 +1336,21 @@ private static bool CheckInvocationArgMixing(
// widest possible escape via writeable ref-like receiver or ref/out argument.
uint escapeTo = scopeOfTheContainingExpression;

var isReceiverRefReadOnly = symbol switch
{
MethodSymbol m => m.IsEffectivelyReadOnly,
// TODO: val escape checks should be skipped for property accesses when
Copy link
Member

Choose a reason for hiding this comment

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

This is the case you were telling me about, where we can't easily determine which accessor we are using?

So an indexer that takes a Span<T> (for multi-dimensional indexing) won't work with this change and will need to be handled in the future, correct (just talking about the getter, since the setter needs to be mutable in this case)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah if it looks like this[Span<T> span] { readonly get; set; } we will be pessimistic and say the span could escape illegally even though we should be able to tell that only the getter is called.

// we can determine the only accessors being called are readonly.
// For now we are pessimistic and check escape if any accessor is non-readonly.
// Tracking in https://github.com/dotnet/roslyn/issues/35606
PropertySymbol p => p.GetMethod?.IsEffectivelyReadOnly != false && p.SetMethod?.IsEffectivelyReadOnly != false,
_ => throw ExceptionUtilities.UnexpectedValue(symbol)
};


// collect all writeable ref-like arguments, including receiver
var receiverType = receiverOpt?.Type;
if (receiverType?.IsRefLikeType == true && receiverType?.IsReadOnly == false)
if (receiverType?.IsRefLikeType == true && !isReceiverRefReadOnly)
{
escapeTo = GetValEscape(receiverOpt, scopeOfTheContainingExpression);
}
Expand Down
175 changes: 175 additions & 0 deletions src/Compilers/CSharp/Test/Semantic/Semantics/RefEscapingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2466,6 +2466,181 @@ public void CopyTo(Span<T> other)
);
}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyRefStruct_Method_RefLikeStructParameter()
{
var csharp = @"
using System;

public readonly ref struct S<T>
{
public void M(Span<T> x) { }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
b.M(x);
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics();
}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyMethod_RefLikeStructParameter()
{
var csharp = @"
using System;

public ref struct S<T>
{
public readonly void M(Span<T> x) { }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
b.M(x);
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics();
}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyRefStruct_RefLikeProperty()
{
var csharp = @"
using System;

public readonly ref struct S<T>
{
public Span<T> P { get => default; set {} }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
b.P = x;
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics(
// (11,15): error CS8352: Cannot use local 'x' in this context because it may expose referenced variables outside of their declaration scope
// b.P = x;
Diagnostic(ErrorCode.ERR_EscapeLocal, "x").WithArguments("x").WithLocation(11, 15));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this test is more to explicitly illustrate the existing behavior. Strictly speaking we might be able to exclude such properties from escape analysis but it doesn't seem to present much value.

}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyRefLikeProperty_01()
{
var csharp = @"
using System;

public ref struct S<T>
{
public readonly Span<T> P { get => default; set {} }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
b.P = x;
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics(
// (11,15): error CS8352: Cannot use local 'x' in this context because it may expose referenced variables outside of their declaration scope
// b.P = x;
Diagnostic(ErrorCode.ERR_EscapeLocal, "x").WithArguments("x").WithLocation(11, 15));
}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyRefLikeProperty_02()
{
var csharp = @"
using System;

public ref struct S<T>
{
public Span<T> P { get => default; readonly set {} }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
b.P = x;
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics(
// (11,15): error CS8352: Cannot use local 'x' in this context because it may expose referenced variables outside of their declaration scope
// b.P = x;
Diagnostic(ErrorCode.ERR_EscapeLocal, "x").WithArguments("x").WithLocation(11, 15));
}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyIndexer_RefLikeStructParameter_01()
{
var csharp = @"
using System;

public ref struct S<T>
{
public readonly Span<T> this[Span<T> span] { get => default; set {} }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
_ = b[x];
b[x] = x;
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics(
// (11,13): error CS8347: Cannot use a result of 'S<byte>.this[Span<byte>]' in this context because it may expose variables referenced by parameter 'span' outside of their declaration scope
// _ = b[x];
Diagnostic(ErrorCode.ERR_EscapeCall, "b[x]").WithArguments("S<byte>.this[System.Span<byte>]", "span").WithLocation(11, 13),
// (11,15): error CS8352: Cannot use local 'x' in this context because it may expose referenced variables outside of their declaration scope
// _ = b[x];
Diagnostic(ErrorCode.ERR_EscapeLocal, "x").WithArguments("x").WithLocation(11, 15));
}

[Fact, WorkItem(35146, "https://github.com/dotnet/roslyn/issues/35146")]
public void ReadOnlyIndexer_RefLikeStructParameter_02()
{
var csharp = @"
using System;
public ref struct S<T>
{
public Span<T> this[Span<T> span] { get => default; readonly set {} }

public unsafe static void N(S<byte> b)
{
Span<byte> x = stackalloc byte[5];
_ = b[x];
b[x] = x;
}
}
";
var comp = CreateCompilationWithMscorlibAndSpan(csharp, TestOptions.UnsafeDebugDll);
comp.VerifyDiagnostics(
// (10,13): error CS8350: This combination of arguments to 'S<byte>.this[Span<byte>]' is disallowed because it may expose variables referenced by parameter 'span' outside of their declaration scope
// _ = b[x];
Diagnostic(ErrorCode.ERR_CallArgMixing, "b[x]").WithArguments("S<byte>.this[System.Span<byte>]", "span").WithLocation(10, 13),
// (10,15): error CS8352: Cannot use local 'x' in this context because it may expose referenced variables outside of their declaration scope
// _ = b[x];
Diagnostic(ErrorCode.ERR_EscapeLocal, "x").WithArguments("x").WithLocation(10, 15),
// (11,9): error CS8350: This combination of arguments to 'S<byte>.this[Span<byte>]' is disallowed because it may expose variables referenced by parameter 'span' outside of their declaration scope
// b[x] = x;
Diagnostic(ErrorCode.ERR_CallArgMixing, "b[x]").WithArguments("S<byte>.this[System.Span<byte>]", "span").WithLocation(11, 9),
// (11,11): error CS8352: Cannot use local 'x' in this context because it may expose referenced variables outside of their declaration scope
// b[x] = x;
Diagnostic(ErrorCode.ERR_EscapeLocal, "x").WithArguments("x").WithLocation(11, 11));
}

[WorkItem(22197, "https://github.com/dotnet/roslyn/issues/22197")]
[Fact()]
public void RefTernaryMustMatchValEscapes()
Expand Down