Skip to content

Allow parsing ref readonly parameters in cref#69104

Merged
jjonescz merged 9 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-17-CrefParsing
Jul 26, 2023
Merged

Allow parsing ref readonly parameters in cref#69104
jjonescz merged 9 commits intodotnet:features/RefReadonlyfrom
jjonescz:RefReadonly-17-CrefParsing

Conversation

@jjonescz
Copy link
Copy Markdown
Member

@jjonescz jjonescz commented Jul 19, 2023

Test plan: #68056

@ghost ghost 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 force-pushed the RefReadonly-17-CrefParsing branch from cd16b91 to d2c7205 Compare July 19, 2023 13:42
@jjonescz jjonescz added the 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
}

[Fact]
public void CRef_RefReadonlyParameter()
Copy link
Copy Markdown
Member

@jcouv jcouv Jul 19, 2023

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

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)

@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
@jjonescz jjonescz force-pushed the RefReadonly-17-CrefParsing branch from da231e3 to 4ec46b2 Compare July 20, 2023 09:21
internal sealed partial class CrefParameterSyntax : CSharpSyntaxNode
{
internal readonly SyntaxToken? refKindKeyword;
internal readonly SyntaxToken? readOnlyKeyword;
Copy link
Copy Markdown
Contributor

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

Done with review pass (commit 2)

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

Done with review pass (commit 7)

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

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

// (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
Copy Markdown
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
Copy Markdown
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.

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

@jjonescz jjonescz merged commit 6ee0240 into dotnet:features/RefReadonly Jul 26, 2023
@jjonescz jjonescz deleted the RefReadonly-17-CrefParsing branch July 26, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants