Allow default values for `ref readonly` parameters by jjonescz · Pull Request #69032 · dotnet/roslyn

jjonescz · 2023-07-14T11:54:49Z

Speclet: https://github.com/dotnet/csharplang/blob/05ed9dcfba84a038e02301a9b267516edfe14159/proposals/ref-readonly-parameters.md#default-parameter-values
Test plan: #68056

AlekseyTs · 2023-07-17T13:11:37Z

src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs

+            // (4,67): warning CS9521: A default value is specified for 'ref readonly' parameter 'i', but 'ref readonly' should be used only for references. Consider declaring the parameter as 'in'.
+            //     static void M([DefaultParameterValue(1)] ref readonly int i = 1) => throw null;
+            Diagnostic(ErrorCode.WRN_RefReadonlyParameterDefaultValue, "1").WithArguments("i").WithLocation(4, 67),
+            // (4,67): error CS8017: The parameter has multiple distinct default values.


// (4,67): error CS8017: The parameter has multiple distinct default values.

This is probably unrelated to the changes, but why is this error reported? #Closed

Yes, that's weird (and you're right it's unrelated as it happens for in parameters today). It's reported because DecodeDefaultParameterValueAttribute returns a bad constant value if there's an equals value and hence it isn't equal to the one from the equals value checked by VerifyParamDefaultValueMatchesAttributeIfAny so it reports they are different (I guess the latter should not report if any of the values is bad).

I guess the latter should not report if any of the values is bad

Probably. Could you open an issue for this?

AlekseyTs · 2023-07-17T13:24:07Z

src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs

+            // (8,9): error CS7036: There is no argument given that corresponds to the required parameter 'd' of 'C.M(ref readonly DateTime)'
+            //         M();
+            Diagnostic(ErrorCode.ERR_NoCorrespondingArgument, "M").WithArguments("d", "C.M(ref readonly System.DateTime)").WithLocation(8, 9));
+    }


}

It looks like we are not testing consumption of default values from metadata. I think we should cover at least all valid forms of encoding: attributes only, constant value, combination of both. #Closed

AlekseyTs · 2023-07-17T13:24:38Z

Done with review pass (commit 1)

AlekseyTs · 2023-07-17T15:33:15Z

src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs

+            ? CreateCompilation(source2, new[] { CreateCompilation(source1).ToMetadataReference() }, options: TestOptions.ReleaseExe)
+            : CreateCompilation(new[] { source1, source2 }, options: TestOptions.ReleaseExe);
+        var verifier = CompileAndVerify(comp, expectedOutput: "1222").VerifyDiagnostics(
+            // (3,47): warning CS9521: A default value is specified for 'ref readonly' parameter 'i', but 'ref readonly' should be used only for references. Consider declaring the parameter as 'in'.


// (3,47): warning CS9521: A default value is specified for 'ref readonly' parameter 'i', but 'ref readonly' should be used only for references. Consider declaring the parameter as 'in'.

Is this warning expected when fromMetadata is true? And if its, then why? #Closed

Yes, because ToMetadataReference() just wraps the compilation, so the warning is propagated from it (I think). I will change it to EmitToImageReference() and split the diagnostics, perhaps that's better.

AlekseyTs · 2023-07-18T14:47:58Z

src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs

+            //         C.M(x);
+            Diagnostic(ErrorCode.WRN_ArgExpectedRefOrIn, "x").WithArguments("1").WithLocation(7, 13);
+        var comp = fromMetadata
+            ? CreateCompilation(source2, new[] { CreateCompilation(source1).VerifyDiagnostics(warning1).EmitToImageReference() }, options: TestOptions.ReleaseExe).VerifyDiagnostics(warning2)


VerifyDiagnostics(warning1)

I think verifying this diagnostics isn't that interesting. #Closed

AlekseyTs · 2023-07-18T14:50:38Z

src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs

+            Diagnostic(ErrorCode.WRN_ArgExpectedRefOrIn, "x").WithArguments("1").WithLocation(7, 13);
+        var comp = fromMetadata
+            ? CreateCompilation(source2, new[] { CreateCompilation(source1).VerifyDiagnostics(warning1).EmitToImageReference() }, options: TestOptions.ReleaseExe).VerifyDiagnostics(warning2)
+            : CreateCompilation(new[] { source1, source2 }, options: TestOptions.ReleaseExe).VerifyDiagnostics(warning1, warning2);


VerifyDiagnostics(warning1, warning2)

I think that we should verify diagnostics on CompileAndVerify instead. The set of diagnostics will be conditional on fromMetadata, but we want to verify complete set of diagnostics, including from emit phase for the compilation. #Closed

AlekseyTs · 2023-07-18T16:20:18Z

src/Compilers/CSharp/Test/Emit2/Semantics/RefReadonlyParameterTests.cs

 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.

+using System;


System

Is this using necessary?

Yes, for Array.Empty for conditional diagnostics in one test

AlekseyTs

LGTM (commit 4), assuming CI is passing

jcouv

LGTM Thanks (iteration 4)

…ters

jjonescz added the Feature - Ref Readonly Parameters `ref readonly` parameters label Jul 14, 2023

ghost added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jul 14, 2023

jjonescz force-pushed the RefReadonly-11-DefaultParameters branch 3 times, most recently from 0c6a54b to c66cf64 Compare July 14, 2023 14:04

Allow default values for ref readonly parameters

acb88a9

jjonescz force-pushed the RefReadonly-11-DefaultParameters branch from c66cf64 to acb88a9 Compare July 14, 2023 14:06

jjonescz marked this pull request as ready for review July 14, 2023 17:39

jjonescz requested a review from a team as a code owner July 14, 2023 17:39

jjonescz requested review from AlekseyTs and jcouv and removed request for a team July 14, 2023 17:40

AlekseyTs reviewed Jul 17, 2023

View reviewed changes

Verify consuming default parameter values from metadata

d57df4d

AlekseyTs reviewed Jul 17, 2023

View reviewed changes

jjonescz mentioned this pull request Jul 18, 2023

Multiple distinct default values error incorrectly reported #69078

Open

Use EmitToImageReference for metadata default params

a942f40

jjonescz requested a review from AlekseyTs July 18, 2023 10:45

jjonescz mentioned this pull request Jul 18, 2023

Diagnostics are propagated from referenced compilation in tests #69084

Open

AlekseyTs reviewed Jul 18, 2023

View reviewed changes

Verify default parameter diagnostics after emit

e77ec26

jjonescz requested a review from AlekseyTs July 18, 2023 15:14

AlekseyTs reviewed Jul 18, 2023

View reviewed changes

AlekseyTs approved these changes Jul 18, 2023

View reviewed changes

jcouv approved these changes Jul 18, 2023

View reviewed changes

jcouv added this to the C# 12.0 milestone Jul 18, 2023

jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Jul 18, 2023

jcouv self-assigned this Jul 18, 2023

jaredpar modified the milestones: C# 12.0, 17.8 Jul 18, 2023

Merge branch 'features/RefReadonly' into RefReadonly-11-DefaultParame…

304286d

…ters

jjonescz enabled auto-merge (squash) July 19, 2023 06:31

jjonescz merged commit 0bed80f into dotnet:features/RefReadonly Jul 19, 2023

jjonescz deleted the RefReadonly-11-DefaultParameters branch July 19, 2023 07:51

allisonchou mentioned this pull request Aug 2, 2023

[Automated] PRs inserted in VS build main-34002.289 #69347

Closed

Conversation

jjonescz commented Jul 14, 2023

Uh oh!

AlekseyTs Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 17, 2023

Uh oh!

AlekseyTs Jul 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

jjonescz Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AlekseyTs Jul 17, 2023 •

edited

Loading

AlekseyTs Jul 17, 2023 •

edited

Loading

AlekseyTs Jul 17, 2023 •

edited

Loading

jjonescz Jul 18, 2023 •

edited

Loading

AlekseyTs Jul 18, 2023 •

edited

Loading

AlekseyTs Jul 18, 2023 •

edited

Loading