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

Suppress ref readonly warning for extension method receiver #69083

Merged
merged 4 commits into from
Jul 21, 2023
Merged

Suppress ref readonly warning for extension method receiver #69083

merged 4 commits into from
Jul 21, 2023

Conversation

jjonescz
Copy link
Member

@jjonescz jjonescz commented Jul 18, 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 Jul 18, 2023
@jjonescz jjonescz added the Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 18, 2023
@jjonescz jjonescz requested review from AlekseyTs and jcouv July 18, 2023 13:26
@jjonescz jjonescz marked this pull request as ready for review July 18, 2023 13:26
@jjonescz jjonescz requested a review from a team as a code owner July 18, 2023 13:26
AlekseyTs
AlekseyTs reviewed Jul 18, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Outdated
ErrorCode.WRN_ArgExpectedRefOrIn,
argument.Syntax,
arg + 1);
if (!invokedAsExtensionMethod)
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

if (!invokedAsExtensionMethod)

Suppressing for all arguments? Looks suspicious. #Closed

AlekseyTs
AlekseyTs reviewed Jul 18, 2023
View reviewed changes
src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs
var source = $$"""
static class C
{
static void M(this {{modifier}} int x) => System.Console.Write(x);
Copy link
Contributor

@AlekseyTs AlekseyTs Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

this {{modifier}} int x)

Are we testing mismatch on other parameters? #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 18, 2023 •
edited by jjonescz
Loading 

                                ErrorCode.WRN_RefReadonlyNotVariable,

Are we testing this code path for this parameter of an extension method? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:3298 in a941da0. [](commit_id = a941da0, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 18, 2023 •
edited by jjonescz
Loading 

                                ErrorCode.WRN_ArgExpectedIn,

Are we testing this code path for this parameter of an extension method? #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:3317 in a941da0. [](commit_id = a941da0, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 1)

@jcouv jcouv self-assigned this Jul 18, 2023
@jcouv jcouv added this to the C# 12.0 milestone Jul 18, 2023
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 18, 2023
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jul 19, 2023 •
edited
Loading 

                                arg + 1);

Here and below we probably shouldn't do + 1 when invokedAsExtensionMethod is true. Otherwise we are getting confusing messages like:

            // (9,14): warning CS9503: Argument 2 should be passed with 'ref' or 'in' keyword
            //         x.M1(x);

But there is no second argument.

In reply to: 1642126147

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs:3288 in 596600a. [](commit_id = 596600a, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 3)

AlekseyTs
AlekseyTs approved these changes Jul 19, 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 4)

@jaredpar jaredpar modified the milestones: C# 12.0, 17.9, 17.8 Jul 19, 2023
@jjonescz
Copy link
Member Author

@jcouv for a second review, thanks

jcouv
jcouv approved these changes Jul 20, 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 4)

@jjonescz jjonescz merged commit d5dd6ac into dotnet:features/RefReadonly Jul 21, 2023
@jjonescz jjonescz deleted the RefReadonly-15-ExtensionMethods branch July 21, 2023 08:47
@jjonescz jjonescz mentioned this pull request Aug 1, 2023
Merged
@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

@jcouv jcouv jcouv approved these changes

@AlekseyTs AlekseyTs AlekseyTs approved these changes

Assignees

@jcouv jcouv

Labels
Area-Compilers Feature - Ref Readonly Parameters `ref readonly` parameters
Projects
None yet
Milestone
17.8
Development

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