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 1 commit
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
13 changes: 12 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 @@ -1331,9 +1331,20 @@ private static bool CheckInvocationArgMixing(
// widest possible escape via writeable ref-like receiver or ref/out argument.
uint escapeTo = scopeOfTheContainingExpression;

var isReadOnlyInvocation = 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.
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 && !isReadOnlyInvocation)
{
escapeTo = GetValEscape(receiverOpt, scopeOfTheContainingExpression);
}
Expand Down
154 changes: 154 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,160 @@ public void CopyTo(Span<T> other)
);
}

[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 ReadOnlyStruct_Property_RefLikeStruct_01()
{
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));
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 ReadOnlyStruct_Property_RefLikeStruct_02()
{
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 ReadOnlyProperty_RefLikeStructParameter()
{
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 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));
}

[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 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));
}

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