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

Add and emit ref kind for ref readonly parameters #68179

Merged
merged 26 commits into from
Jun 21, 2023
Merged

Add and emit ref kind for ref readonly parameters #68179

merged 26 commits into from
Jun 21, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented May 12, 2023 •
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 May 12, 2023
@build-analysis build-analysis bot mentioned this pull request May 12, 2023
Open
5 tasks
@jjonescz jjonescz force-pushed the RefReadonly-02-RefKindEmit branch 3 times, most recently from dad2351 to 030d450 Compare May 17, 2023 13:57
@jjonescz jjonescz added the Feature - Ref Readonly Parameters `ref readonly` parameters label May 17, 2023
@jjonescz jjonescz added this to the C# 12.0 milestone May 17, 2023
@jjonescz jjonescz force-pushed the RefReadonly-02-RefKindEmit branch from 030d450 to 7ad29e0 Compare June 8, 2023 14:30
@jjonescz jjonescz marked this pull request as ready for review June 8, 2023 15:44
@jjonescz jjonescz requested review from a team as code owners June 8, 2023 15:44
jcouv
jcouv reviewed Jun 8, 2023
View reviewed changes
Copy link
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.

Done with review pass (iteration 1). Minor comments only

@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jun 8, 2023
@jcouv jcouv self-assigned this Jun 8, 2023
@AlekseyTs
Copy link
Contributor 

            isBad = true;

Pretty much everything about state of a symbol is accessible through APIs. No need to call a method to check result of importing it.

In reply to: 1598695510

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs:401 in 982dad6. [](commit_id = 982dad6, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 20, 2023 •
edited
Loading 

            isBad |= (parameter.RefKind == RefKind.RefReadOnly) != hasInAttributeModifier;

Consider adding an assert that RefKind is not RefKind.RefReadOnlyParameter

In reply to: 1598763691

Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs:391 in ccc3b80. [](commit_id = ccc3b80, deletion_comment = False)

AlekseyTs
AlekseyTs reviewed Jun 20, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenRefReadonlyParameterTests.cs
{
var p = m.GlobalNamespace.GetMember<MethodSymbol>("<>f__AnonymousDelegate0.Invoke").Parameters.Single();
VerifyRefReadonlyParameter(p,
// PROTOTYPE: Invoke method is virtual but no modreq is emitted. This happens for `in` parameters, as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Invoke method is virtual but no modreq is emitted. This happens for in parameters, as well.

Sounds like a pre-existing bug.

AlekseyTs
AlekseyTs approved these changes Jun 20, 2023
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 25)

jcouv
jcouv reviewed Jun 20, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenRefReadonlyParameterTests.cs Outdated
{
if (refKind)
{
Assert.Equal(RefKind.RefReadOnlyParameter, parameter.RefKind);
Copy link
Member

Choose a reason for hiding this comment

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

When refKind is false, we do no check. In that case, could we tighten to check that the RefKind is not RefReadOnlyParameter?
Assert.Equal(refKind, RefKind.RefReadOnlyParameter == parameter.RefKind);

jcouv
jcouv reviewed Jun 20, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenRefReadonlyParameterTests.cs Outdated

if (metadataIn)
{
Assert.True(parameter.IsMetadataIn);
Copy link
Member

Choose a reason for hiding this comment

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

Same question here. Consider Assert.Equal(metadataIn, parameter.IsMetadataIn);

jcouv
jcouv approved these changes Jun 20, 2023
View reviewed changes
Copy link
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 25) with two small test suggestions to consider

@jjonescz jjonescz enabled auto-merge (squash) June 21, 2023 08:05
@jjonescz jjonescz merged commit eb63b9f into dotnet:features/RefReadonly Jun 21, 2023
@jjonescz jjonescz deleted the RefReadonly-02-RefKindEmit branch June 21, 2023 09:16
This was referenced Jul 14, 2023
Merged
Merged
@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

@tannergooding tannergooding tannergooding left review comments

@tmat tmat tmat left review comments

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Feature - Ref Readonly Parameters `ref readonly` parameters
Projects
None yet
Milestone
C# 12.0
Development

Successfully merging this pull request may close these issues.
6 participants
@jjonescz @jaredpar @jcouv @AlekseyTs @tmat @tannergooding