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 parameters in cref #69104

Merged
merged 9 commits into from
Jul 26, 2023
Merged

Allow parsing ref readonly parameters in cref #69104

merged 9 commits into from
Jul 26, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jul 19, 2023 •
edited
Loading

Test plan: #68056

@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 19, 2023
@jjonescz jjonescz force-pushed the RefReadonly-17-CrefParsing branch 2 times, most recently from 308e1b2 to cd16b91 Compare July 19, 2023 13:27
@jjonescz jjonescz added the New Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 19, 2023
@jjonescz jjonescz requested review from AlekseyTs and jcouv July 19, 2023 14:29
@jjonescz jjonescz marked this pull request as ready for review July 19, 2023 14:29
@jjonescz jjonescz requested review from a team as code owners July 19, 2023 14:29
jcouv
jcouv reviewed Jul 19, 2023
View reviewed changes
src/Compilers/CSharp/Test/Symbol/DocumentationComments/CrefTests.cs

var parameterSymbol = ((IMethodSymbol)model.GetSymbolInfo(cref).Symbol).Parameters.Single();
Assert.Equal(RefKind.In, parameterSymbol.RefKind);
}

[Fact]
public void CRef_RefReadonlyParameter()
Copy link
Member

@jcouv jcouv Jul 19, 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.

Consider also testing readonly ref (error) #Resolved

jcouv
jcouv approved these changes Jul 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 1)

@jcouv jcouv self-assigned this Jul 19, 2023
@jcouv jcouv added this to the C# 12.0 milestone Jul 19, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 19, 2023
CyrusNajmabadi
CyrusNajmabadi reviewed Jul 20, 2023
View reviewed changes
...CSharpSyntaxGenerator/CSharpSyntaxGenerator.SourceGenerator/Syntax.xml.Internal.Generated.cs
/// there is no name and there are no attributes or other modifiers.
/// </summary>
internal sealed partial class CrefParameterSyntax : CSharpSyntaxNode
{
internal readonly SyntaxToken? refKindKeyword;
internal readonly SyntaxToken? readOnlyKeyword;
Copy link
Member

Choose a reason for hiding this comment

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

another, possibly cleaner, way to do this is to just changet this to a TokenList of Modifiers.

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 2)

@jjonescz jjonescz requested review from AlekseyTs and jcouv July 21, 2023 09:42
@AlekseyTs
Copy link
Contributor

Done with review pass (commit 7)

@jjonescz jjonescz requested a review from AlekseyTs July 21, 2023 13:50
AlekseyTs
AlekseyTs approved these changes Jul 21, 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 8)

AlekseyTs
AlekseyTs approved these changes Jul 21, 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 9)

jcouv
jcouv reviewed Jul 25, 2023
View reviewed changes
src/Compilers/CSharp/Test/Symbol/DocumentationComments/CrefTests.cs
// (3,16): error CS8652: The feature 'ref readonly parameters' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.
// void M(ref readonly int x)
Diagnostic(ErrorCode.ERR_FeatureInPreview, "readonly").WithArguments("ref readonly parameters").WithLocation(3, 16),
// (8,26): warning CS1658: The feature 'ref readonly parameters' is currently in Preview and *unsupported*. To use Preview features, use the 'preview' language version.. See also error CS8652.
Copy link
Member

Choose a reason for hiding this comment

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

not related to this PR: I'm curious whether this warning override will trigger the IDE fixer to turn on the preview LangVer. This would be a pre-existing issue. May need to file an issue

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't. Filed #69228.

jcouv
jcouv approved these changes Jul 25, 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 9)

@jjonescz jjonescz merged commit 6ee0240 into dotnet:features/RefReadonly Jul 26, 2023
25 checks passed
@jjonescz jjonescz deleted the RefReadonly-17-CrefParsing branch July 26, 2023 07:40
@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

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi 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 @CyrusNajmabadi @jcouv