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

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

Merged
merged 2 commits into from
Jul 7, 2023
Merged

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

merged 2 commits into from
Jul 7, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jul 4, 2023

@jjonescz jjonescz added the New Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 4, 2023
@jjonescz jjonescz added this to the C# 12.0 milestone Jul 4, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot 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
@@ -5878,6 +5878,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_OutAttrOnInParam" xml:space="preserve">
<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
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
@@ -802,7 +802,7 @@ protected override void DecodeWellKnownAttributeImpl(ref DecodeWellKnownAttribut
else if (ReportExplicitUseOfReservedAttributes(in arguments,
ReservedAttributes.DynamicAttribute |
ReservedAttributes.IsReadOnlyAttribute |
// PROTOTYPE: RequiresLocationAttribute
ReservedAttributes.RequiresLocationAttribute |
Copy link
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
Member Author

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
Contributor

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

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

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

Done with review pass (commit 1)

@jjonescz
Copy link
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
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
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
25 checks passed
@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 New 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