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

Allow parsing ref readonly modifier #68164

Merged
merged 12 commits into from
Jun 8, 2023
Merged

Allow parsing ref readonly modifier #68164

merged 12 commits into from
Jun 8, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented May 11, 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 11, 2023
@jjonescz jjonescz changed the base branch from main to features/RefReadonly May 11, 2023 10:35
@jjonescz jjonescz force-pushed the RefReadonly-01-AllowModifier branch from 8e975a7 to b10e188 Compare May 11, 2023 12:02
@jjonescz jjonescz added the New Feature - Ref Readonly Parameters `ref readonly` parameters label May 11, 2023
@jjonescz jjonescz requested review from AlekseyTs and jcouv May 11, 2023 13:26
@jjonescz jjonescz marked this pull request as ready for review May 11, 2023 14:14
@jjonescz jjonescz requested a review from a team as a code owner May 11, 2023 14:14
@jcouv jcouv self-assigned this May 11, 2023
jcouv
jcouv reviewed May 11, 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.

It looks like an alternative syntax is still being discussed (using an attribute), so I'll hold off review until we close on that

jcouv
jcouv previously approved these changes May 11, 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) with some small test suggestions

@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label May 11, 2023
@jcouv jcouv added this to the C# 12.0 milestone May 11, 2023
@jjonescz jjonescz dismissed jcouv’s stale review May 12, 2023 09:43

changed parsing

@jjonescz jjonescz mentioned this pull request May 12, 2023
Closed
67 tasks
@jjonescz
Copy link
Member Author

@jcouv, @AlekseyTs PTAL

@jjonescz jjonescz changed the title Allow ref readonly modifier Allow parsing ref readonly modifier May 16, 2023
@jjonescz
Copy link
Member Author

@jcouv @AlekseyTs @dotnet/roslyn-compiler please review

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 7)

AlekseyTs
AlekseyTs approved these changes May 18, 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 11), assuming CI is passing

jcouv
jcouv reviewed May 19, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs Outdated
using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Xunit;

namespace Microsoft.CodeAnalysis.CSharp.UnitTests.Semantics
Copy link
Member

Choose a reason for hiding this comment

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

Consider using file-scoped namespace (we tend to do that for new test files)

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using file-scoped namespace (we tend to do that for new test files)

I do not think there is a requirement like this. Also, I personally prefer the block-like namespaces.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, there's no requirement. The main reason I advocate this is because test files tend to have long lines (such as diagnostics).

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason I advocate this is because test files tend to have long lines (such as diagnostics).

I dislike this form of namespaces no matter what.

jcouv
jcouv reviewed May 19, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
{
diagnostics.Add(ErrorCode.ERR_RefReadOnlyWrongOrdering, modifier);
}
else if (seenRef)
Copy link
Member

@jcouv jcouv May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need this condition? #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this condition?

It makes an observable difference and, in my opinion, improves quality of diagnostics.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like one test would be affected: params ref readonly. I think you're correct, this check reduces cascaded diagnostics in such scenario.
Then my problem is with how the variable is named, it would be better as "seenValidRef". That said, previous variables seem to suffer the same problem, so it's fine to leave.

jcouv
jcouv reviewed May 19, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs
else if (seenRef)
{
Binder.CheckFeatureAvailability(modifier, MessageID.IDS_FeatureRefReadonlyParameters, diagnostics);
seenReadonly = true;
Copy link
Member

@jcouv jcouv May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

nit: Should we unconditionally set seenReadonly? #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we unconditionally set seenReadonly?

If there is a good enough reason to do that. However, the tests do not show that. If you have a specific scenario in mind, it would be worth looking at.

Copy link
Member

Choose a reason for hiding this comment

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

What is the reason to set seenReadonly conditionally?

Copy link
Contributor

@AlekseyTs AlekseyTs May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What is the reason to set seenReadonly conditionally?

It does not look useful to set it in any other condition. Other cases do not look at the value. An error that readonly it is not preceded by ref is much better than an error about a duplicate, in my opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW, this is the difference: 8cdd541

Copy link
Contributor

@AlekseyTs AlekseyTs May 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Basically readonly means nothing unless it is part of ref readonly and the ref is actually not ignored.

jcouv
jcouv approved these changes May 19, 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 11)

@jjonescz jjonescz enabled auto-merge (squash) June 7, 2023 07:27
@jjonescz jjonescz merged commit d573f23 into dotnet:features/RefReadonly Jun 8, 2023
@jjonescz jjonescz deleted the RefReadonly-01-AllowModifier branch June 8, 2023 07:15
@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

@Sergio0694 Sergio0694 Sergio0694 left review comments

@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
Projects
None yet
Milestone
C# 12.0
Development

Successfully merging this pull request may close these issues.
4 participants
@jjonescz @AlekseyTs @Sergio0694 @jcouv