Skip to content

Widen RefKindVector to hold RefReadonlyParameter#69034

Merged
jjonescz merged 4 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-13-RefKindVector
Jul 19, 2023
Merged

Widen RefKindVector to hold RefReadonlyParameter#69034
jjonescz merged 4 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-13-RefKindVector

Conversation

@jjonescz
Copy link
Copy Markdown
Member

@jjonescz jjonescz commented Jul 14, 2023

Follow up on #68179 (comment).
Test plan: #68056

@jjonescz jjonescz added the Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 14, 2023
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 14, 2023
@jjonescz jjonescz force-pushed the RefReadonly-13-RefKindVector branch from 1d5ccce to a257170 Compare July 14, 2023 13:35
@jjonescz jjonescz force-pushed the RefReadonly-13-RefKindVector branch from a257170 to 79e0e84 Compare July 14, 2023 14:29
@jjonescz jjonescz marked this pull request as ready for review July 14, 2023 17:40
@jjonescz jjonescz requested a review from a team as a code owner July 14, 2023 17:40
@jjonescz jjonescz requested review from AlekseyTs and jcouv and removed request for a team July 14, 2023 17:40
AlekseyTs
AlekseyTs reviewed Jul 17, 2023
View reviewed changes
Comment thread src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LoweredDynamicOperationFactory.cs
}
}

// PROTOTYPE: RefKindVector does not support RefReadOnlyParameter.
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

// PROTOTYPE: RefKindVector does not support RefReadOnlyParameter.

Are we testing this code path when ref readonly parameter is involved? #Closed

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 17, 2023 •
edited
Loading 

    public static bool TryParse(string refKindString, int capacity, out RefKindVector result)

Do we need to worry about change in interpretation across compiler versions? Is there a scenario when an old compiler has to parse a name generated by new compiler and vise versa?

In reply to: 1638050803

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/RefKindVector.cs:109 in 79e0e84. [](commit_id = 79e0e84, deletion_comment = False)

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 17, 2023 •
edited
Loading 

        static RefKind getRefKind(RefKind refKind) => refKind == RefKind.None ? RefKind.None : RefKind.Ref;

It looks like we should never get here with refKind == RefKind.RefReadOnlyParameter. Consider adding an assert.

In reply to: 1638065309

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LoweredDynamicOperationFactory.cs:806 in 79e0e84. [](commit_id = 79e0e84, deletion_comment = False)

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 17, 2023

Done with review pass (commit 1)

@jjonescz
Copy link
Copy Markdown
Member Author

jjonescz commented Jul 17, 2023 

    public static bool TryParse(string refKindString, int capacity, out RefKindVector result)

I don't think so. The only usage I found is in anonymous type manager which parses names from the current module being built.

In reply to: 1638050803

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/RefKindVector.cs:109 in 79e0e84. [](commit_id = 79e0e84, deletion_comment = False)

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 17, 2023 

    public static bool TryParse(string refKindString, int capacity, out RefKindVector result)

which parses names from the current module being built.

I am not sure if this is the case. It enumerates GetPreviousAnonymousDelegates

In reply to: 1638088633

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/RefKindVector.cs:109 in 79e0e84. [](commit_id = 79e0e84, deletion_comment = False)

@jjonescz
Copy link
Copy Markdown
Member Author

jjonescz commented Jul 18, 2023 

    public static bool TryParse(string refKindString, int capacity, out RefKindVector result)

It enumerates GetPreviousAnonymousDelegates

Yes, but that's used during Edit & Continue, which I hope cannot span across different compiler versions.

But if we don't want to change RefKindVector encoding, I'm fine with that, I can close this PR and just remove the prototype comments - everything works fine - just delegates with ref readonly parameters are encoded as <>f__AnonymousDelegates not <>A{0000000} delegates.

In reply to: 1638269099

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/RefKindVector.cs:109 in 79e0e84. [](commit_id = 79e0e84, deletion_comment = False)

@jjonescz jjonescz requested a review from AlekseyTs July 18, 2023 08:29
AlekseyTs
AlekseyTs approved these changes Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
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 3)

jcouv
jcouv reviewed Jul 18, 2023
View reviewed changes
Comment thread src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs
}
catch (Microsoft.CSharp.RuntimeBinder.RuntimeBinderException)
{
System.Console.Write("exception");
Copy link
Copy Markdown
Member

@jcouv jcouv Jul 18, 2023

Choose a reason for hiding this comment

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

nit: in the future, consider adding spaces at the end of such strings (that are meant to be concatenated into the output), I was looking for "exception2" in the source and couldn't find it, haha

@jcouv jcouv self-assigned this Jul 18, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 18, 2023
@jcouv jcouv added this to the C# 12.0 milestone Jul 18, 2023
jcouv
jcouv approved these changes Jul 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 3)

@jaredpar jaredpar modified the milestones: C# 12.0, 17.8 Jul 18, 2023
@jjonescz jjonescz enabled auto-merge (squash) July 19, 2023 08:17
@jjonescz jjonescz merged commit 60de847 into dotnet:features/RefReadonly Jul 19, 2023
@jjonescz jjonescz deleted the RefReadonly-13-RefKindVector branch July 19, 2023 09:38
@allisonchou allisonchou mentioned this pull request Aug 2, 2023
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

@jcouv jcouv jcouv approved these changes

Assignees

@jcouv jcouv

Labels

Area-Compilers Feature - Ref Readonly Parameters `ref readonly` parameters

Projects

None yet

Milestone

17.8

Development

Successfully merging this pull request may close these issues.

4 participants

@jjonescz @AlekseyTs @jcouv @jaredpar