Skip to content

Add and emit ref kind for ref readonly parameters#68179

Merged
jjonescz merged 26 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-02-RefKindEmit
Jun 21, 2023
Merged

Add and emit ref kind for ref readonly parameters#68179
jjonescz merged 26 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-02-RefKindEmit

Conversation

@jjonescz
Copy link
Copy Markdown
Member

@jjonescz jjonescz commented May 12, 2023 •
edited
Loading

@ghost ghost 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
jcouv
jcouv reviewed Jun 8, 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.

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
tmat
tmat reviewed Jun 9, 2023
View reviewed changes
@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jun 20, 2023 

            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
Copy Markdown
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
AlekseyTs
AlekseyTs reviewed Jun 20, 2023
View reviewed changes
AlekseyTs
AlekseyTs reviewed Jun 20, 2023
View reviewed changes
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
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jun 20, 2023

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
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 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
Copy Markdown
Member

@jcouv jcouv Jun 20, 2023

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
Copy Markdown
Member

@jcouv jcouv Jun 20, 2023

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

@tmat tmat tmat left review comments

@tannergooding tannergooding tannergooding left review comments

@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

C# 12.0

Development

Successfully merging this pull request may close these issues.

6 participants

@jjonescz @jaredpar @jcouv @AlekseyTs @tmat @tannergooding