Check 'scoped' differences in overrides and interface implementations #61738

cston · 2022-06-07T00:13:06Z

Proposal: low-level-struct-improvements.md
Test plan: #59194

Require scoped modifiers to match exactly in overrides and implicit and explicit interface implementations.

Require scoped modifiers to match exactly in implicit and explicit conversions of lambda expressions and method group to delegates.

AlekseyTs · 2022-06-08T18:17:01Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6778,6 +6778,15 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_ScopedRefAndRefStructOnly" xml:space="preserve">
    <value>The 'scoped' modifier can be used for refs and ref struct values only.</value>
  </data>
+  <data name="ERR_ScopedMismatchInParameterOfOverrideOrImplementation" xml:space="preserve">
+    <value>The 'scoped' declaration of parameter '{0}' doesn't match overridden or implemented member.</value>


declaration

It looks like we refer to 'scoped' as modifier in other messages. Can we use the same term here as well? #Closed

AlekseyTs · 2022-06-08T18:22:38Z

src/Compilers/CSharp/Portable/CSharpResources.resx

+  <data name="ERR_ScopedMismatchInParameterOfOverrideOrImplementation" xml:space="preserve">
+    <value>The 'scoped' declaration of parameter '{0}' doesn't match overridden or implemented member.</value>
+  </data>
+  <data name="ERR_ScopedMismatchInParameterOfTargetDelegate" xml:space="preserve">


ERR_ScopedMismatchInParameterOfTargetDelegate

Do we need any checks for function pointer conversions? #Closed

~~I think we won't need to because scoped can't be used in a function pointer parameter.~~ d'oy, we're talking about a scenario where a method group is converted to function pointer.

There could still be a mismatch if the assigned method has a scoped parameter, although the resulting function pointer will be safe presumably because the method has a stronger contract than the function pointer.

static R F1(R x, scoped R y) => x; delegate*<R, R, R> d1 = &F1;

I'll add a test.

AlekseyTs · 2022-06-08T19:43:40Z

src/Compilers/CSharp/Portable/Lowering/DiagnosticsPass_ExpressionTrees.cs

@@ -723,11 +723,18 @@ public override BoundNode VisitConversion(BoundConversion node)
            switch (node.ConversionKind)
            {
                case ConversionKind.MethodGroup:
+                    if (node.Operand is BoundMethodGroup group)
+                    {
+                        checkValidScopedMethodConversion(group.Syntax, node.Conversion.Method, node.Type, invokedAsExtensionMethod: node.IsExtensionMethod, _diagnostics);


checkValidScopedMethodConversion(group.Syntax, node.Conversion.Method, node.Type, invokedAsExtensionMethod: node.IsExtensionMethod, _diagnostics);

I would expect this check and the one below to be performed by the Binder, when it creates the BoundConversion node. #Closed

AlekseyTs · 2022-06-08T20:13:19Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

+    }
+    static void Explicit()
+    {
+        var d1 = (D1)M2;


(D1)

Are we testing delegate creation with new? #Closed

Good catch, thanks.

AlekseyTs · 2022-06-08T20:18:07Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

+        }
+
+        [Fact]
+        public void DelegateConversions_04()


DelegateConversions_04

Consider adding tests for function pointers as well. #Closed

AlekseyTs · 2022-06-08T20:18:14Z

Done with review pass (commit 1)

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_LifetimeAnnotation.cs

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs

AlekseyTs · 2022-06-09T01:37:07Z

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

            var boundLambda = unboundLambda.Bind((NamedTypeSymbol)destination, isExpressionTree: destination.IsGenericOrNonGenericExpressionType(out _));
            diagnostics.AddRange(boundLambda.Diagnostics);
+
+            bool hasErrors = CheckValidScopedMethodConversion(syntax, boundLambda.Symbol, destination, invokedAsExtensionMethod: false, diagnostics);


hasErrors

I wouldn't mark node with hasErrors flag because of this. The purpose of the flag is mostly to suppress cascading diagnostics for completely broken scenarios. When we cannot accurately figure out the result type, etc. It is not required to reflect if there are any errors associated with the node. In this case, it doesn't look like the new error can cause a cascading diagnostics. #Closed

AlekseyTs · 2022-06-09T01:40:32Z

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

+            }
+            else if (targetType is FunctionPointerTypeSymbol functionPointerType)
+            {
+                delegateMethod = functionPointerType.Signature;


delegateMethod = functionPointerType.Signature

Is wording of ERR_ScopedMismatchInParameterOfTargetDelegate going to be confusing for a function pointer scenario? #Closed

AlekseyTs · 2022-06-09T01:43:07Z

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

@@ -1351,6 +1382,8 @@ static ErrorCode getRefMismatchErrorCode(TypeKind type)
            {
                return true;
            }
+
+            bool result = CheckValidScopedMethodConversion(syntax, selectedMethod, delegateOrFuncPtrType, isExtensionMethod, diagnostics);


result

Similar comment. I don't think new errors should affect the result. Similar to the errors we are likely to report below #Closed

AlekseyTs · 2022-06-09T01:44:29Z

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

+                hasErrors = !conversion.IsImplicit;
+                if (!hasErrors)
+                {
+                    hasErrors = CheckValidScopedMethodConversion(unboundLambda.Syntax, boundLambda.Symbol, type, invokedAsExtensionMethod: false, diagnostics);


hasErrors

Same comment about hasErrors. #Closed

AlekseyTs · 2022-06-09T01:46:57Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

@@ -4424,25 +4438,43 @@ static unsafe void Main()
                // (8,19): error CS8755: 'scoped' cannot be used as a modifier on a function pointer parameter.
                //         delegate*<scoped R, void> f1 = &F1;
                Diagnostic(ErrorCode.ERR_BadFuncPointerParamModifier, "scoped").WithArguments("scoped").WithLocation(8, 19),
+                // (8,40): error CS8989: The 'scoped' modifier of parameter 'r1' doesn't match target delegate 'delegate*<R, void>'.


target delegate

The usage of "target delegate" feels confusing in this scenario. #Closed

AlekseyTs · 2022-06-09T01:51:14Z

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs

@@ -5062,6 +5089,903 @@ static void Main()
                });
        }

+        [Fact]
+        public void DelegateConversions_01()


DelegateConversions_01

Are we testing conversion to a Linq expression tree? #Closed

AlekseyTs · 2022-06-09T01:59:52Z

Done with review pass (commit 2)

AlekseyTs

LGTM (commit 3)

dotnet-issue-labeler bot added the Area-Compilers label Jun 7, 2022

cston force-pushed the scoped-overrides branch 5 times, most recently from a955c94 to 6be761e Compare June 7, 2022 20:57

Check 'scoped' differences in overrides and interface implementations

26cef29

cston force-pushed the scoped-overrides branch from 6be761e to 26cef29 Compare June 7, 2022 21:11

cston marked this pull request as ready for review June 7, 2022 21:14

cston requested a review from a team as a code owner June 7, 2022 21:14

cston added the Feature - Ref Fields label Jun 7, 2022

RikkiGibson self-assigned this Jun 8, 2022

AlekseyTs reviewed Jun 8, 2022

View reviewed changes

RikkiGibson reviewed Jun 8, 2022

View reviewed changes

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_LifetimeAnnotation.cs Show resolved Hide resolved

RikkiGibson reviewed Jun 8, 2022

View reviewed changes

src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs Outdated Show resolved Hide resolved

RikkiGibson reviewed Jun 8, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/TypeSymbol.cs Show resolved Hide resolved

PR feedback

369a4cd

RikkiGibson approved these changes Jun 8, 2022

View reviewed changes

AlekseyTs reviewed Jun 9, 2022

View reviewed changes

PR feedback

e8aa1b8

AlekseyTs approved these changes Jun 9, 2022

View reviewed changes

cston merged commit a0ba31c into dotnet:features/ref-fields Jun 9, 2022

cston deleted the scoped-overrides branch June 9, 2022 03:51

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check 'scoped' differences in overrides and interface implementations #61738

Check 'scoped' differences in overrides and interface implementations #61738

cston commented Jun 7, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

RikkiGibson Jun 8, 2022 •

edited

Loading

cston Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

cston Jun 8, 2022

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs commented Jun 8, 2022

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs commented Jun 9, 2022

AlekseyTs left a comment

Check 'scoped' differences in overrides and interface implementations #61738

Check 'scoped' differences in overrides and interface implementations #61738

Conversation

cston commented Jun 7, 2022 • edited Loading

AlekseyTs Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

RikkiGibson Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

cston Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

cston Jun 8, 2022

Choose a reason for hiding this comment

AlekseyTs Jun 8, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 8, 2022

AlekseyTs Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 9, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 9, 2022

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Jun 7, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

RikkiGibson Jun 8, 2022 •

edited

Loading

cston Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 8, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading

AlekseyTs Jun 9, 2022 •

edited

Loading