Skip to content

Disallow applying RequiresLocation and Out attributes to ref readonly parameters in source#68871

Merged
jjonescz merged 2 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-09-AttributeInSource
Jul 7, 2023
Merged

Disallow applying RequiresLocation and Out attributes to ref readonly parameters in source#68871
jjonescz merged 2 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-09-AttributeInSource

Conversation

@jjonescz
Copy link
Copy Markdown
Member

@jjonescz jjonescz commented Jul 4, 2023

@jjonescz jjonescz added the Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 4, 2023
@jjonescz jjonescz added this to the C# 12.0 milestone Jul 4, 2023
@ghost ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 4, 2023
@jjonescz jjonescz force-pushed the RefReadonly-09-AttributeInSource branch from 66dee83 to 356051d Compare July 4, 2023 11:34
@jjonescz jjonescz marked this pull request as ready for review July 4, 2023 12:12
@jjonescz jjonescz requested a review from a team as a code owner July 4, 2023 12:12
@jjonescz jjonescz requested review from AlekseyTs and jcouv July 4, 2023 12:13
AlekseyTs
AlekseyTs reviewed Jul 5, 2023
View reviewed changes
src/Compilers/CSharp/Portable/CSharpResources.resx
<value>An in parameter cannot have the Out attribute.</value>
</data>
<data name="ERR_OutAttrOnRefReadonlyParam" xml:space="preserve">
<value>A ref readonly parameter cannot have the Out attribute.</value>
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

A ref readonly parameter cannot have the Out attribute.

This error doesn't quite fit the title of the PR. Consider adjusting it #Closed

AlekseyTs
AlekseyTs reviewed Jul 5, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/SourceComplexParameterSymbol.cs
ReservedAttributes.DynamicAttribute |
ReservedAttributes.IsReadOnlyAttribute |
// PROTOTYPE: RequiresLocationAttribute
ReservedAttributes.RequiresLocationAttribute |
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

RequiresLocationAttribute

I see ReservedAttributes.IsReadOnlyAttribute specified as reserved for other symbols too. Consider following the pattern

Copy link
Copy Markdown
Member Author

@jjonescz jjonescz Jul 5, 2023

Choose a reason for hiding this comment

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

I think IsReadOnly has wider semantics - for example, returns can be readonly, but they probably cannot be RequiresLocation.

Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 6, 2023

Choose a reason for hiding this comment

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

I think IsReadOnly has wider semantics - for example, returns can be readonly, but they probably cannot be RequiresLocation.

Looking at IsByRefLikeAttribute, compiler synthesizes it only for named types, but ReservedAttributes.IsByRefLikeAttribute is checked for a wide set of symbols. I don't think this is a blocking issue for this PR, but let's confirm with LDM whether we want proactively prohibit application of the attribute in other contexts in order to preserve future design space.

Copy link
Copy Markdown
Member Author

@jjonescz jjonescz Jul 7, 2023

Choose a reason for hiding this comment

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

Added to section "alternatives pending LDM review": dotnet/csharplang#7328

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 5, 2023 •
edited
Loading 

            Assert.Same(attribute, p.GetAttributes().Single().AttributeClass);

Isn't this going to conflict with your other PR? #Closed

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs:138 in 356051d. [](commit_id = 356051d, deletion_comment = False)

AlekseyTs
AlekseyTs reviewed Jul 5, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs
void M2([RequiresLocation] in int p) { }
void M3([RequiresLocation] ref int p) { }
void M4([RequiresLocation] int p) { }
[return: RequiresLocation] int M5() => 5;
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

RequiresLocation

Do we allow IsReadOnlyAttribute on return value? #Closed

Copy link
Copy Markdown
Member Author

@jjonescz jjonescz Jul 5, 2023

Choose a reason for hiding this comment

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

No, but that's because IsReadOnly can be emitted there by the compiler. RequiresLocation is different.

AlekseyTs
AlekseyTs reviewed Jul 5, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs
""";

CreateCompilation(source).VerifyDiagnostics(
// (1,1): hidden CS8019: Unnecessary using directive.
Copy link
Copy Markdown
Contributor

@AlekseyTs AlekseyTs Jul 5, 2023 •
edited by jjonescz
Loading

Choose a reason for hiding this comment

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

// (1,1): hidden CS8019: Unnecessary using directive.

Consider removing using in order to reduce the noise. Could use fully qualified name instead. #Pending

@AlekseyTs
Copy link
Copy Markdown
Contributor

AlekseyTs commented Jul 5, 2023

Done with review pass (commit 1)

@jjonescz
Copy link
Copy Markdown
Member Author

jjonescz commented Jul 5, 2023 

            Assert.Same(attribute, p.GetAttributes().Single().AttributeClass);

Yes, but note that this isn't changed in this PR, it's only CodeFlow's diff displaying it as an addition.

In reply to: 1621881653

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs:138 in 356051d. [](commit_id = 356051d, deletion_comment = False)

@jjonescz jjonescz changed the title Disallow applying RequiresLocationAttribute to parameters in source Disallow applying RequiresLocation and Out attributes to ref readonly parameters in source Jul 5, 2023
AlekseyTs
AlekseyTs approved these changes Jul 6, 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 1)

@jcouv jcouv self-assigned this Jul 7, 2023
jcouv
jcouv approved these changes Jul 7, 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 1)

@jjonescz jjonescz enabled auto-merge (squash) July 7, 2023 08:55
@jjonescz jjonescz merged commit 644742d into dotnet:features/RefReadonly Jul 7, 2023
@jjonescz jjonescz deleted the RefReadonly-09-AttributeInSource branch July 7, 2023 10:01
@allisonchou allisonchou mentioned this pull request Aug 2, 2023
Closed
@jjonescz jjonescz mentioned this pull request Sep 19, 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 untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

C# 12.0

Development

Successfully merging this pull request may close these issues.

3 participants

@jjonescz @AlekseyTs @jcouv