Disallow `ref` to `in`/`ref readonly` method parameter conversion #71531

jjonescz · 2024-01-08T14:28:26Z

Fixes #71383.
Corresponding speclet update: dotnet/csharplang#7820
Test plan: #68056

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

jaredpar · 2024-01-10T19:40:21Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+        // In method group conversions,
+        // - 'in' is allowed to be converted to 'ref',
+        // - 'ref readonly' is allowed to be converted to 'ref' or 'in',
+        // - 'in' is allowed to be converted to 'ref readonly'.


Reading through this chart, occurs to me that out should be allowed to convert to ref. I do not want to change the scope of the PR to include this. Just a thought that occured.

Seems safe, but that probably needs a separate proposal, right?

Agree. Not sure it's worth it, just occurred to me when reading this particular part of the change.

AlekseyTs · 2024-01-11T17:17:33Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1937,7 +1937,7 @@ internal bool MethodIsCompatibleWithDelegateOrFunctionPointer(BoundExpression? r
                //  * Every ref or similar parameter has an identity conversion to the corresponding target parameter
                // Note the addition of the reference requirement: it means that for delegate type void D(int i), void M(long l) is
                // _applicable_, but not _compatible_.
-                if (!hasConversion(this, delegateType.TypeKind, Conversions, delegateParameter.Type, methodParameter.Type, delegateParameter.RefKind, methodParameter.RefKind, ref useSiteInfo))
+                if (!hasConversion(delegateType.TypeKind, Conversions, source: delegateParameter.Type, destination: methodParameter.Type, sourceRefKind: delegateParameter.RefKind, destinationRefKind: methodParameter.RefKind, checkingReturns: false, ref useSiteInfo))


if (!hasConversion(delegateType.TypeKind, Conversions, source: delegateParameter.Type, destination: methodParameter.Type, sourceRefKind: delegateParameter.RefKind, destinationRefKind: methodParameter.RefKind, checkingReturns: false, ref useSiteInfo))

It looks like this line became too long to fit an average screen. Consider splitting it #Closed

AlekseyTs · 2024-01-11T17:18:38Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

@@ -1958,7 +1958,7 @@ internal bool MethodIsCompatibleWithDelegateOrFunctionPointer(BoundExpression? r
            bool returnsMatch = delegateOrFuncPtrMethod switch
            {
                { RefKind: RefKind.None, ReturnsVoid: true } => method.ReturnsVoid,
-                { RefKind: var destinationRefKind } => hasConversion(this, delegateType.TypeKind, Conversions, methodReturnType, delegateReturnType, method.RefKind, destinationRefKind, ref useSiteInfo),
+                { RefKind: var destinationRefKind } => hasConversion(delegateType.TypeKind, Conversions, source: methodReturnType, destination: delegateReturnType, sourceRefKind: method.RefKind, destinationRefKind: destinationRefKind, checkingReturns: true, ref useSiteInfo),


{ RefKind: var destinationRefKind } => hasConversion(delegateType.TypeKind, Conversions, source: methodReturnType, destination: delegateReturnType, sourceRefKind: method.RefKind, destinationRefKind: destinationRefKind, checkingReturns: true, ref useSiteInfo),

Consider splitting this line too #Closed

AlekseyTs · 2024-01-11T17:24:05Z

src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs

+                // Allowed ref kind mismatches between parameters have already been checked by overload resolution.
+                if (checkingReturns
+                    ? sourceRefKind != destinationRefKind
+                    : (sourceRefKind != RefKind.None) != (destinationRefKind != RefKind.None))


(sourceRefKind != RefKind.None) != (destinationRefKind != RefKind.None)

(sourceRefKind == RefKind.None) != (destinationRefKind == RefKind.None)? It feels like there are too many != in the code #Closed

AlekseyTs · 2024-01-11T17:27:01Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/ConversionsBase.cs

@@ -1488,7 +1488,7 @@ private static LambdaConversionResult IsAnonymousFunctionCompatibleWithDelegate(
                {
                    for (int p = 0; p < delegateParameters.Length; ++p)
                    {
-                        if (!OverloadResolution.AreRefsCompatibleForMethodConversion(delegateParameters[p].RefKind, anonymousFunction.RefKind(p), compilation) ||
+                        if (!OverloadResolution.AreRefsCompatibleForMethodConversion(source: anonymousFunction.RefKind(p), destination: delegateParameters[p].RefKind, compilation) ||


source:

Naming of parameters feels confusing to me. Data flow from delegate parameter to the lambda parameter. So, I would expect the delegate parameter to be the "source". #Closed

AlekseyTs · 2024-01-11T17:28:57Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

@@ -3305,7 +3305,7 @@ internal EffectiveParameters(ImmutableArray<TypeWithAnnotations> types, Immutabl
                    return argRefKind;
                }
            }
-            else if (AreRefsCompatibleForMethodConversion(paramRefKind, argRefKind, binder.Compilation))
+            else if (AreRefsCompatibleForMethodConversion(source: paramRefKind, destination: argRefKind, binder.Compilation))


source

Similar comment here about parameter naming. Argument is the "source", its value flows to the parameter #Closed

AlekseyTs · 2024-01-11T17:31:34Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

-        // In method group conversions, 'in' is allowed to match 'ref' and 'ref readonly' is allowed to match 'ref' or 'in'.
-        internal static bool AreRefsCompatibleForMethodConversion(RefKind x, RefKind y, CSharpCompilation compilation)
+        // In method group conversions,
+        // - 'in' is allowed to be converted to 'ref',


'in' is allowed to be converted to 'ref',

Same comment as for the speclet update. I don't think the language has a concept conversion for modifiers. #Closed

AlekseyTs · 2024-01-11T17:36:22Z

src/Compilers/CSharp/Portable/Binder/Semantics/OverloadResolution/OverloadResolution.cs

+        // - 'in' is allowed to be converted to 'ref',
+        // - 'ref readonly' is allowed to be converted to 'ref' or 'in',
+        // - 'in' is allowed to be converted to 'ref readonly'.
+        internal static bool AreRefsCompatibleForMethodConversion(RefKind source, RefKind destination, CSharpCompilation compilation)


source

As mentioned earlier, I find the naming of parameters confusing. Would it make sense to switch to terms like "delegateParameterRefKind" and "candidateMethodParameterRefKind"? #Closed

AlekseyTs · 2024-01-11T17:40:52Z

Done with review pass (commit 4), tests are not looked at

AlekseyTs

LGTM (commit 5). I didn't do thorough review of test, relied on @jaredpar sign-off for that.

jaredpar · 2024-01-12T19:08:47Z

I didn't do thorough review of test, relied on @jaredpar sign-off for that.

Primarily reviewed those, made sure they lined up with spec and eexpectations.

Disallow ref to in/ref readonly method parameter conversion

9328bb3

jjonescz added the New Feature - Ref Readonly Parameters `ref readonly` parameters label Jan 8, 2024

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 8, 2024

jjonescz mentioned this pull request Jan 8, 2024

Disallow method parameter conversion of ref to in/ref readonly dotnet/csharplang#7820

Merged

jjonescz added 2 commits January 9, 2024 13:37

Fixup a test

55cb90b

Simplify delegate compatibility checking

221df36

jjonescz marked this pull request as ready for review January 9, 2024 14:55

jjonescz requested a review from a team as a code owner January 9, 2024 14:55

jjonescz marked this pull request as draft January 9, 2024 14:56

jjonescz marked this pull request as ready for review January 9, 2024 14:56

jaredpar reviewed Jan 10, 2024

View reviewed changes

Move diagnostics closer to sources

b24ce54

jaredpar approved these changes Jan 10, 2024

View reviewed changes

AlekseyTs reviewed Jan 11, 2024

View reviewed changes

Improve naming and code style

6484441

AlekseyTs approved these changes Jan 12, 2024

View reviewed changes

Merge branch 'main' into 71383-RefInConversion

bd90f49

jjonescz enabled auto-merge (squash) January 15, 2024 13:04

Fixup a test after merge

0e62832

jjonescz merged commit c02fb4f into dotnet:main Jan 16, 2024
25 checks passed

jjonescz deleted the 71383-RefInConversion branch January 16, 2024 09:23

ghost added this to the Next milestone Jan 16, 2024

Cosifne modified the milestones: Next, 17.10 P1 Jan 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow `ref` to `in`/`ref readonly` method parameter conversion #71531

Disallow `ref` to `in`/`ref readonly` method parameter conversion #71531

jjonescz commented Jan 8, 2024

jaredpar Jan 10, 2024

jjonescz Jan 11, 2024

jaredpar Jan 11, 2024

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs commented Jan 11, 2024

AlekseyTs left a comment •

edited

Loading

jaredpar commented Jan 12, 2024

Disallow ref to in/ref readonly method parameter conversion #71531

Disallow ref to in/ref readonly method parameter conversion #71531

Conversation

jjonescz commented Jan 8, 2024

jaredpar Jan 10, 2024

Choose a reason for hiding this comment

jjonescz Jan 11, 2024

Choose a reason for hiding this comment

jaredpar Jan 11, 2024

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jan 11, 2024

AlekseyTs left a comment • edited Loading

Choose a reason for hiding this comment

jaredpar commented Jan 12, 2024

Disallow `ref` to `in`/`ref readonly` method parameter conversion #71531

Disallow `ref` to `in`/`ref readonly` method parameter conversion #71531

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs Jan 11, 2024 •

edited

Loading

AlekseyTs left a comment •

edited

Loading