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

Jump to bottom

Allow ref-like locals in iterators and async methods #72664

Merged
merged 26 commits into from
Apr 16, 2024
Merged

Allow ref-like locals in iterators and async methods #72664

merged 26 commits into from
Apr 16, 2024

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Mar 22, 2024 •
edited
Loading

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Mar 22, 2024
@jjonescz jjonescz marked this pull request as ready for review March 22, 2024 12:03
@jjonescz jjonescz requested a review from a team as a code owner March 22, 2024 12:03
@cston
Copy link
Member

cston commented Mar 29, 2024 •
edited by jjonescz
Loading 

    // this is an error

Consider updating comment. #Resolved

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/SpanStackSafetyTests.cs:854 in 8e0373f. [](commit_id = 8e0373f, deletion_comment = False)

@jaredpar jaredpar added this to the 17.11 milestone Mar 29, 2024
@AlekseyTs
Copy link
Contributor 

    foreach (var y in x.F)

I actually meant to make y a ref local.

In reply to: 2035256041

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs:20437 in d6dbc5f. [](commit_id = d6dbc5f, deletion_comment = False)

@AlekseyTs
Copy link
Contributor 

    foreach (var y in x.F)

I actually meant to make y a ref local.

In reply to: 2035260444

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs:21284 in d6dbc5f. [](commit_id = d6dbc5f, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 18)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 21)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 22). The only remaining issue I am tracking is #72664 (comment).

AlekseyTs
AlekseyTs approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 23), assuming CI is passing

@jjonescz jjonescz requested a review from cston April 8, 2024 14:12
@jjonescz
Copy link
Member Author

jjonescz commented Apr 12, 2024 •
edited
Loading

@cston for another look, thanks

cston
cston reviewed Apr 15, 2024
View reviewed changes
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncSpillTests.cs
// {
Diagnostic(ErrorCode.ERR_ByRefTypeAndAwait, @"{
await Async1(F1(), G(F2(), stackalloc int[] { 1, 2, 3 }, await F3()));
}").WithArguments("System.Span<int>").WithLocation(8, 5)
Copy link
Member

@cston cston Apr 15, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

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

Minor: Should the error location be stackalloc int[] { 1, 2, 3 } rather than the containing block? #WontFix

Copy link
Member Author

@jjonescz jjonescz Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That does not look easily doable. The problematic local used across await is a synthesized local whose syntax is the await F3() expression and the node where the usage after await occurs has the whole block as syntax (that's what's used to report the diagnostic, it seems more useful than the former in most scenarios). In any case, we do not seem to have the location of the stackalloc expression at hand.

@cston
Copy link
Member

cston commented Apr 15, 2024 •
edited by jjonescz
Loading

public ref struct S

It looks like this test wasn't part of this PR, but ...
Is the ref struct used, and does this test actually test ref struct? Perhaps Enumerator.Current was intended to be declared public S Current => new S(i); instead.

Actually, let's not make any changes since this is an existing test. #Resolved

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAwaitForeachTests.cs:2049 in b4cc4b5. [](commit_id = b4cc4b5, deletion_comment = False)

cston
cston reviewed Apr 15, 2024
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/InlineArrayTests.cs Outdated
}

[ConditionalFact(typeof(CoreClrOnly))]
public void UserDefinedIndexer_Warning_01()
public void UserDefinedIndexer_Warning_17()
Copy link
Member

@cston cston Apr 15, 2024 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

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

17

01? #Resolved

@tmat tmat mentioned this pull request Apr 15, 2024
Open
@jjonescz
Copy link
Member Author

jjonescz commented Apr 16, 2024 •
edited
Loading

public ref struct S

Not sure if the scenario with ref struct Current is tested elsewhere, so I will add a new test for that. Thanks.

In reply to: 2057185471

Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAwaitForeachTests.cs:2049 in b4cc4b5. [](commit_id = b4cc4b5, deletion_comment = False)

@jjonescz jjonescz merged commit 8ff156f into dotnet:features/RefInAsync Apr 16, 2024
30 checks passed
@jjonescz jjonescz deleted the RefInAsync-01-RefLikeLocals branch April 16, 2024 10:38
This was referenced May 18, 2024
Closed
Closed
@dotnet-bot dotnet-bot mentioned this pull request Jun 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cston cston cston approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees
No one assigned
Labels
Area-Compilers New Feature - Ref/Unsafe in Iterators/Async untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.11
Development

Successfully merging this pull request may close these issues.
4 participants
@jjonescz @cston @AlekseyTs @jaredpar