Skip to content

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

Merged
jjonescz merged 26 commits intodotnet:features/RefInAsyncfrom
jjonescz:RefInAsync-01-RefLikeLocals
Apr 16, 2024
Merged

Allow ref-like locals in iterators and async methods#72664
jjonescz merged 26 commits intodotnet:features/RefInAsyncfrom
jjonescz:RefInAsync-01-RefLikeLocals

Conversation

@jjonescz
Copy link
Member

@jjonescz jjonescz commented Mar 22, 2024 •
edited
Loading

@ghost ghost 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
@jjonescz jjonescz requested review from AlekseyTs and cston March 26, 2024 11:29
cston
cston reviewed Mar 26, 2024
View reviewed changes
cston
cston reviewed Mar 29, 2024
View reviewed changes
cston
cston reviewed Mar 29, 2024
View reviewed changes
cston
cston reviewed Mar 29, 2024
View reviewed changes
@cston
Copy link
Contributor

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
AlekseyTs reviewed Apr 2, 2024
View reviewed changes
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 4, 2024 

    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

AlekseyTs commented Apr 4, 2024 

    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

AlekseyTs commented Apr 4, 2024

Done with review pass (commit 18)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 5, 2024

Done with review pass (commit 21)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 8, 2024

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
Contributor

@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
Contributor

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
Contributor

@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
@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

@AlekseyTs AlekseyTs AlekseyTs approved these changes

+1 more reviewer

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Compilers 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