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

Fix UnscopedRef handling in indexers #74343

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Fix UnscopedRef handling in indexers #74343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1b5ae65
Fix UnscopedRef handling in indexers
jaredpar Jul 11, 2024
2dee10c
Apply suggestions from code review
jaredpar Jul 22, 2024
e66b86b
PR feedback
jaredpar Jul 22, 2024
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
7 changes: 7 additions & 0 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 @@ -4624,6 +4624,13 @@ uint getIndexerEscape(
uint receiverEscapeScope = CallingMethodScope;
foreach (var escapeValue in escapeValues)
{
// This is a call to an indexer so the ref escape scope can only impact the escape value if it
// can be assigned to `this`. Return Only can't do this.
if (escapeValue.IsRefEscape && escapeValue.EscapeLevel != EscapeLevel.CallingMethod)
{
continue;
}

uint escapeScope = escapeValue.IsRefEscape
? GetRefEscape(escapeValue.Argument, scopeOfTheContainingExpression)
: GetValEscape(escapeValue.Argument, scopeOfTheContainingExpression);
Expand Down
180 changes: 180 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 @@ -3369,6 +3369,89 @@ static void Test()
);
}

[Fact]
public void RefLikeEscapeMixingObjectInitializer4()
{
var tree = SyntaxFactory.ParseSyntaxTree("""
using System;

public ref struct S1
{
public S1(ref Span<int> span) { }
int this[in Span<int> span] { get => 0; set {} }

static void Test()
{
Span<int> stackSpan = stackalloc int[10];
Span<int> heapSpan = default;

var local1 = new S1(ref heapSpan) { [heapSpan] = 0 };
var local2 = new S1(ref heapSpan) { [stackSpan] = 0 }; // 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Is behavior of this test affected by the impl change in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I was stacking PRs waiting for my original indexer PR to get final sign off. This test was meant to be a part of that PR but ended up in the sub-branch for this change.

var local3 = new S1(ref stackSpan) { [heapSpan] = 0 };
var local4 = new S1(ref stackSpan) { [stackSpan] = 0 };
}
}

public ref struct S2
{
public S2(scoped ref Span<int> span) { }
int this[Span<int> span] { get => 0; set {} }

static void Test()
{
Span<int> stackSpan = stackalloc int[10];
Span<int> heapSpan = default;

var local1 = new S2(ref heapSpan) { [heapSpan] = 0 };
var local2 = new S2(ref heapSpan) { [stackSpan] = 0 };
var local3 = new S2(ref stackSpan) { [heapSpan] = 0 };
var local4 = new S2(ref stackSpan) { [stackSpan] = 0 };
}
}

public ref struct S3
{
public S3(ref Span<int> span) { }
int this[scoped Span<int> span] { get => 0; set {} }

static void Test()
{
Span<int> stackSpan = stackalloc int[10];
Span<int> heapSpan = default;

var local1 = new S3(ref heapSpan) { [heapSpan] = 0 };
var local2 = new S3(ref heapSpan) { [stackSpan] = 0 };
var local3 = new S3(ref stackSpan) { [heapSpan] = 0 };
var local4 = new S3(ref stackSpan) { [stackSpan] = 0 };
}
}

public struct S5
{
public S5(Span<int> span) { }
Span<int> this[int index] { get => default; set { } }

static void Test()
{
Span<int> stackSpan = stackalloc int[] { 13 };
Span<int> heapSpan = default;

S5 local;
local = new S5(stackSpan) { [0] = stackSpan };
local = new S5(stackSpan) { [0] = heapSpan };
local = new S5(heapSpan) { [0] = stackSpan };
local = new S5(heapSpan) { [0] = heapSpan };
}
}
""", TestOptions.Regular.WithLanguageVersion(LanguageVersion.CSharp11));
var comp = CreateCompilationWithSpan(tree, TestOptions.UnsafeDebugDll);
comp.VerifyEmitDiagnostics(
// (14,33): error CS8350: This combination of arguments to 'S1.S1(ref Span<int>)' is disallowed because it may expose variables referenced by parameter 'span' outside of their declaration scope
// var local2 = new S1(ref heapSpan) { [stackSpan] = 0 }; // 1
Diagnostic(ErrorCode.ERR_CallArgMixing, "heapSpan").WithArguments("S1.S1(ref System.Span<int>)", "span").WithLocation(14, 33)
);
}

[Theory]
[InlineData(LanguageVersion.CSharp10)]
[InlineData(LanguageVersion.CSharp11)]
Expand Down Expand Up @@ -3946,6 +4029,103 @@ static void Test()
);
}

[Fact]
public void ObjectInitializer_Indexer_In_Escape()
{
var code = """
using System;
using System.Diagnostics.CodeAnalysis;

public ref struct S1
{
public S1() { }
string this[in int x] { get => default; set { } }

static void Test(int[] array, int x)
{
int y = 0;
S1 local;

local = new() { [in array[0]] = "" };
local = new() { [in x] = "" };
local = new() { [in y] = "" };
local = new() { [0] = "" };
}
}

public ref struct S2
{
public S2() { }
string this[[UnscopedRef] in int x] { get => default; set { } }

static void Test(int x, in int y, [UnscopedRef] in int z)
{
S2 local = default;
local = new() { [x] = "" }; // 1
local = new() { [y] = "" }; // 2
local = new() { [z] = "" };
local[x] = ""; // 3
local[y] = ""; // 4
local[z] = "";
}
Copy link
Member

Choose a reason for hiding this comment

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

}

Consider testing:

static void Test(int x, in int y, [UnscopedRef] in int z)
{
    S2 local = default;
    local = new() { [x] = "" }; // 1
    local = new() { [y] = "" }; // 2
    local = new() { [z] = "" };
    local[x] = ""; // 3
    local[y] = ""; // 4
    local[z] = "";
}

}
""";

var comp = CreateCompilationWithSpan([code, UnscopedRefAttributeDefinition], TestOptions.UnsafeDebugDll, TestOptions.Regular11);
comp.VerifyEmitDiagnostics(
// (29,23): error CS8352: Cannot use variable '[x] = ""' in this context because it may expose referenced variables outside of their declaration scope
// local = new() { [x] = "" }; // 1
Diagnostic(ErrorCode.ERR_EscapeVariable, @"{ [x] = """" }").WithArguments(@"[x] = """"").WithLocation(29, 23),
// (30,23): error CS8352: Cannot use variable '[y] = ""' in this context because it may expose referenced variables outside of their declaration scope
// local = new() { [y] = "" }; // 2
Diagnostic(ErrorCode.ERR_EscapeVariable, @"{ [y] = """" }").WithArguments(@"[y] = """"").WithLocation(30, 23),
// (32,9): error CS8350: This combination of arguments to 'S2.this[in int]' is disallowed because it may expose variables referenced by parameter 'x' outside of their declaration scope
// local[x] = ""; // 3
Diagnostic(ErrorCode.ERR_CallArgMixing, "local[x]").WithArguments("S2.this[in int]", "x").WithLocation(32, 9),
// (32,15): error CS8166: Cannot return a parameter by reference 'x' because it is not a ref parameter
// local[x] = ""; // 3
Diagnostic(ErrorCode.ERR_RefReturnParameter, "x").WithArguments("x").WithLocation(32, 15),
// (33,9): error CS8350: This combination of arguments to 'S2.this[in int]' is disallowed because it may expose variables referenced by parameter 'x' outside of their declaration scope
// local[y] = ""; // 4
Diagnostic(ErrorCode.ERR_CallArgMixing, "local[y]").WithArguments("S2.this[in int]", "x").WithLocation(33, 9),
// (33,15): error CS9077: Cannot return a parameter by reference 'y' through a ref parameter; it can only be returned in a return statement
// local[y] = ""; // 4
Diagnostic(ErrorCode.ERR_RefReturnOnlyParameter, "y").WithArguments("y").WithLocation(33, 15)
);
}

[Fact]
[WorkItem("https://github.com/dotnet/roslyn/issues/66056")]
public void ObjectInitializer_Indexer_Unscoped()
{
var code = """
using System;
using System.Diagnostics.CodeAnalysis;

public ref struct S1
{
public S1() { }
string this[[UnscopedRef] in int x] { get => default; set { } }

static S1 Test1(int x) => new S1() { [in x] = "" }; // 1
static S1 Test2(in int x) => new S1() { [in x] = "" };
static S1 Test3(scoped in int x) => new S1() { [in x] = "" }; // 2
static S1 Test3(int[] x) => new S1() { [in x[0]] = "" };
}

""";

var comp = CreateCompilationWithSpan([code, UnscopedRefAttributeDefinition], TestOptions.UnsafeDebugDll, TestOptions.Regular11);
comp.VerifyEmitDiagnostics(
// (9,40): error CS8352: Cannot use variable '[in x] = ""' in this context because it may expose referenced variables outside of their declaration scope
// static S1 Test1(int x) => new S1() { [in x] = "" }; // 1
Diagnostic(ErrorCode.ERR_EscapeVariable, @"{ [in x] = """" }").WithArguments(@"[in x] = """"").WithLocation(9, 40),
// (11,50): error CS8352: Cannot use variable '[in x] = ""' in this context because it may expose referenced variables outside of their declaration scope
// static S1 Test3(scoped in int x) => new S1() { [in x] = "" }; // 2
Diagnostic(ErrorCode.ERR_EscapeVariable, @"{ [in x] = """" }").WithArguments(@"[in x] = """"").WithLocation(11, 50)
);
}

[Theory]
[InlineData(LanguageVersion.CSharp10)]
[InlineData(LanguageVersion.CSharp11)]
Expand Down