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

Fix UnscopedRef handling in indexers #74343

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Conversation

jaredpar
Copy link
Member

The object initializer logic wasn't completely handling the lifetime of
the in part of parameters. It was considering it escaping even if it
wasn't marked as [UnscopedRef]

closes #66056

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 11, 2024
@jaredpar
Copy link
Member Author

Waiting for #74341 to merge before publishing

@jaredpar jaredpar force-pushed the ref-more2-fix branch 4 times, most recently from e9c8a8b to 08617ab Compare July 15, 2024 13:52
The object initializer logic wasn't completely handling the lifetime of
the `in` part of parameters. It was considering it escaping even if it
wasn't marked as `[UnscopedRef]`

closes dotnet#66056
@jaredpar jaredpar marked this pull request as ready for review July 15, 2024 13:54
@jaredpar jaredpar requested a review from a team as a code owner July 15, 2024 13:54
@jaredpar
Copy link
Member Author

@cston, @jjonescz, @RikkiGibson PTAL

@RikkiGibson RikkiGibson self-assigned this Jul 15, 2024
local = new() { [in x] = "" }; // 1
local = new() { [in y] = "" }; // 2
local = new() { [0] = "" }; // 3
}
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] = "";
}

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]] = "" }; // 2
Copy link
Member

Choose a reason for hiding this comment

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

// 2

Remove?

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs Outdated Show resolved Hide resolved
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.

jaredpar and others added 2 commits July 22, 2024 06:03
Co-authored-by: Rikki Gibson <rikkigibson@gmail.com>
@jaredpar
Copy link
Member Author

@RikkiGibson feedback addressed

@jaredpar jaredpar merged commit 84fb0c3 into dotnet:main Jul 23, 2024
24 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the Next milestone Jul 23, 2024
@jaredpar jaredpar deleted the ref-more2-fix branch July 23, 2024 22:26
@RikkiGibson RikkiGibson modified the milestones: Next, 17.12 P1 Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing ref safety error for [UnscopedRef] indexer argument in object initializer
3 participants