Report error implementing interface with `[UnscopedRef]` member by cston · Pull Request #66024 · dotnet/roslyn

cston · 2022-12-15T00:47:43Z

…parameter targets

jcouv · 2022-12-15T03:34:21Z

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

            var comp = CreateCompilation(new[] { source, UnscopedRefAttributeDefinition });
            comp.VerifyDiagnostics(
-                // (4,6): error CS9063: UnscopedRefAttribute cannot be applied to this item because it is unscoped by default.
+                // (4,6): error CS9101: UnscopedRefAttribute can only be applied to struct instance methods, and cannot be applied to constructors or init-only methods.


cannot be applied to constructors

nit: The diagnostic says "cannot be applied to constructors" but it looks like we don't report this diagnostic on constructors (see UnscopedRefAttribute_Constructor) so it doesn't seem particularly relevant. Consider removing this portion of the diagnostic. #Resolved

Good catch, thanks. I will leave the diagnostic message as is since the message is correct that the attribute cannot be applied to constructors. However, we typically report ErrorCode.ERR_AttributeOnBadSymbolType instead of this error because the BCL attribute definition does not include AttributeTargets.Constructor. To ensure we are catching this case, regardless of the attribute definition, I've added a version of the constructor test with an attribute that does allow targeting constructors.

jcouv

LGTM Thanks (iteration 3)

AlekseyTs · 2022-12-19T15:05:34Z

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

+                return false;
+            }
+            return method.HasUnscopedRefAttribute ||
+                method.AssociatedSymbol is PropertySymbol { HasUnscopedRefAttribute: true };


method.AssociatedSymbol is PropertySymbol { HasUnscopedRefAttribute: true };

What about event accessors? #Closed

What about event accessors?

There was no response to this question.

Logged #66274 to follow up in a subsequent PR.

AlekseyTs · 2022-12-19T15:12:08Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

                else
                {
-                    diagnostics.Add(ErrorCode.ERR_UnscopedRefAttributeUnsupportedTarget, arguments.AttributeSyntaxOpt.Location);
+                    diagnostics.Add(ErrorCode.ERR_UnscopedRefAttributeUnsupportedMethodTarget, arguments.AttributeSyntaxOpt.Location);


ERR_UnscopedRefAttributeUnsupportedMethodTarget

The wording is probably going to sound strange for a property. It excludes properties. #Closed

AlekseyTs · 2022-12-19T15:18:12Z

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

                                invokedAsExtensionMethod: false);
                        }
+
+                        if (implementingMethod.HasUnscopedRefAttributeOnMethodOrProperty())


implementingMethod

It looks like we are going to report this error even for implicit implementations. Is this really what we want? Even for implementations that are in a base method? #Closed

[UnscopedRef] attribute can only be applied to struct instance methods and properties, so base types can be ignored.

AlekseyTs · 2022-12-19T15:25:34Z

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


+        [Fact]
+        [WorkItem(64508, "https://github.com/dotnet/roslyn/issues/64508")]
+        public void UnscopedRefAttribute_InterfaceImplementation_01()


UnscopedRefAttribute_InterfaceImplementation_01

Are there any interesting scenarios around events? #Closed

Added tests.

AlekseyTs · 2022-12-19T15:30:40Z

Done with review pass (commit 4)

AlekseyTs · 2023-01-05T16:28:25Z

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

+                """;
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
+            comp.VerifyEmitDiagnostics(
+                // (9,6): error CS9101: UnscopedRefAttribute can only be applied to struct instance methods and properties, and cannot be applied to constructors or init-only members.


UnscopedRefAttribute can only be applied to struct instance methods and properties

Are we testing this scenario in a struct? #Closed

Or interface?

Are we testing this scenario in a struct?

UnscopedRefAttribute_InterfaceImplementation_04

Or interface?

UnscopedRefAttribute_InterfaceImplementation_02

Or interface?

UnscopedRefAttribute_InterfaceImplementation_02

It doesn't look like UnscopedRefAttribute_InterfaceImplementation_02 tests an implementation in an interface

AlekseyTs · 2023-01-05T16:29:07Z

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

+                """;
+            var comp = CreateCompilation(new[] { source, UnscopedRefAttributeDefinition });
+            comp.VerifyEmitDiagnostics(
+                // (10,6): error CS9101: UnscopedRefAttribute can only be applied to struct instance methods and properties, and cannot be applied to constructors or init-only members.


UnscopedRefAttribute can only be applied to struct instance methods and properties

Are we testing these scenarios in a struct? #Closed

Or interface?

AlekseyTs · 2023-01-05T16:30:20Z

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

+                """;
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
+            comp.VerifyEmitDiagnostics(
+                // (11,31): error CS9101: UnscopedRefAttribute can only be applied to struct instance methods and properties, and cannot be applied to constructors or init-only members.


UnscopedRefAttribute can only be applied to struct instance methods and propertie

Are we testing these scenarios in a struct? #Closed

Or interface?

AlekseyTs · 2023-01-05T16:30:57Z

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

+}";
+            var comp = CreateCompilation(new[] { source, UnscopedRefAttributeDefinition });
+            comp.VerifyDiagnostics(
+                // (6,6): error CS9101: UnscopedRefAttribute can only be applied to struct instance methods and properties, and cannot be applied to constructors or init-only members.


UnscopedRefAttribute can only be applied to struct instance methods and properties,

Are we testing these scenarios in a struct? #Closed

Or interface?

AlekseyTs · 2023-01-05T16:38:03Z

Done with review pass (commit 7)

AlekseyTs · 2023-01-05T16:47:40Z

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

+                class C2 : I<int>
+                {
+                    private int _f2;
+                    [UnscopedRef] ref int I<int>.F1() => ref _f2; // 4


[UnscopedRef] ref int I.F1() => ref _f2; // 4

Are we testing scenario like this in an interface? #Closed

UnscopedRefAttribute_InterfaceImplementation_02 has interface members marked as [UnscopedRef]. Does that cover the scenario?

UnscopedRefAttribute_InterfaceImplementation_02 has interface members marked as [UnscopedRef]. Does that cover the scenario?

Does it test implementing an interface member in an interface?

AlekseyTs · 2023-01-05T16:48:53Z

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

+                """;
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net70);
+            comp.VerifyEmitDiagnostics(
+                // (5,6): error CS9101: UnscopedRefAttribute can only be applied to struct instance methods and properties, and cannot be applied to constructors or init-only members.


UnscopedRefAttribute can only be applied to struct instance methods and properties

Are we testing static members declaration and implementation? #Closed

AlekseyTs · 2023-01-05T21:46:35Z

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

+struct S
+{
+    event D E7 { [UnscopedRef] add { } remove { } } // 3
+    event D E8 { add { } [UnscopedRef] remove { } } // 4


event D E8 { add { } [UnscopedRef] remove { } } // 4

What about an interface? #Closed

AlekseyTs · 2023-01-05T21:48:49Z

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

+class C
+{
+    [UnscopedRef] event D E1; // 1
+    [UnscopedRef] event D E2 { add { } remove { } } // 2


[UnscopedRef] event D E2 { add { } remove { } } // 2

I think this form is also supported in an interface #Closed

AlekseyTs · 2023-01-05T22:01:09Z

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

+                }
+                struct S3 : I<object>
+                {
+                    [UnscopedRef] event D<object> I<object>.E { add { } remove { } } // 6


[UnscopedRef] event D I.E { add { } remove { } } // 6

Are we testing this scenario in an interface? #Closed

AlekseyTs · 2023-01-05T22:02:24Z

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

+                struct S2 : I<object>
+                {
+                    event D<object> I<object>.E1 { [UnscopedRef] add { } remove { } } // 7
+                    event D<object> I<object>.E2 { add { } [UnscopedRef] remove { } } // 8


Are we testing these scenarios in an interface? #Closed

AlekseyTs · 2023-01-05T22:18:47Z

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

+                }
+                struct S : I<object>
+                {
+                    [UnscopedRef] event D<object> I<object>.E { add { } remove { } } // 3


[UnscopedRef] event D I.E { add { } remove { } } // 3

In an interface? #Closed

AlekseyTs · 2023-01-05T22:20:31Z

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

+                    public static R<int> P2 { [UnscopedRef] get { return default; } [UnscopedRef] set { } } // 3, 4
+                    public static event D<int> E { [UnscopedRef] add { } [UnscopedRef] remove { } } // 5, 6
+                }
+                {{type}} C2 : I<object>


{{type}} C2 : I

In an interface? #Closed

AlekseyTs · 2023-01-05T22:39:04Z

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

+                struct S : I<int>
+                {
+                    private int _f;
+                    [UnscopedRef] public ref int F() => ref _f;


[UnscopedRef] public ref int F() => ref _f;

What is the intent here? It looks like this isn't an interface implementation #Closed

Testing that it's possible to explicitly implement an interface in addition to providing a corresponding [UnscopedRef] API.

AlekseyTs · 2023-01-05T22:40:03Z

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

            return !method.IsStatic &&
                method.ContainingType?.IsStructType() == true &&
-                !method.IsConstructor() &&
+                method.MethodKind is (MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.PropertyGet or MethodKind.PropertySet) &&


method.MethodKind is (MethodKind.Ordinary or MethodKind.ExplicitInterfaceImplementation or MethodKind.PropertyGet or MethodKind.PropertySet) &&

What test scenario needed this change? #Closed

Annotated local functions and lambdas within a struct - see UnscopedRefAttribute_Method_05.

AlekseyTs · 2023-01-05T22:42:04Z

Done with review pass (commit 8)

AlekseyTs · 2023-01-05T23:14:49Z

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

                Diagnostic(ErrorCode.ERR_RefAssignNarrower, "value.F = ref this").WithArguments("F", "this").WithLocation(20, 16));
        }

+        [ConditionalFact(typeof(CoreClrOnly))]


[ConditionalFact(typeof(CoreClrOnly))]

Minor: Since we are targeting specific TargetFramework, [Fact] would work, I think.

AlekseyTs

LGTM (commit 9)

jcouv

LGTM Thanks (iteration 9)

Use specific error messages for unsupported [UnscopedRef] method and …

774debd

…parameter targets

ghost added the Area-Compilers label Dec 15, 2022

cston added the Feature - Ref Fields label Dec 15, 2022

cston force-pushed the 64508 branch from 6199b75 to 9b6c440 Compare December 15, 2022 01:07

Report error implementing interface with [UnscopedRef] member

9b6c440

cston marked this pull request as ready for review December 15, 2022 01:37

cston requested a review from a team as a code owner December 15, 2022 01:37

IsBuildOnlyDiagnostic

47b7606

jcouv reviewed Dec 15, 2022

View reviewed changes

jcouv self-assigned this Dec 15, 2022

jcouv approved these changes Dec 15, 2022

View reviewed changes

cston requested review from a team and RikkiGibson December 15, 2022 14:51

Additional constructor test

20cb121

AlekseyTs reviewed Dec 19, 2022

View reviewed changes

cston added 3 commits January 4, 2023 00:42

Address feedback

2e61715

Add test

26cc462

Fix build

7c7426f

cston changed the base branch from main to release/dev17.5 January 4, 2023 16:22

runfoapp bot mentioned this pull request Jan 4, 2023

WorkCoordinatorTests.AdditionalDocumentOpenedClosedEvents is flaky #64047

Closed

AlekseyTs reviewed Jan 5, 2023

View reviewed changes

Address feedback

dc12c56

cston mentioned this pull request Jan 5, 2023

Handle event accessors from metadata marked [UnscopedRef] #66274

Open

AlekseyTs reviewed Jan 5, 2023

View reviewed changes

Address feedback

e65236e

AlekseyTs reviewed Jan 5, 2023

View reviewed changes

AlekseyTs approved these changes Jan 5, 2023

View reviewed changes

jcouv approved these changes Jan 5, 2023

View reviewed changes

cston merged commit 90de707 into dotnet:release/dev17.5 Jan 6, 2023

cston deleted the 64508 branch January 6, 2023 01:17

cston mentioned this pull request Jan 6, 2023

Report error implementing unannotated interface member with [UnscopedRef] member #64508

Closed

allisonchou mentioned this pull request Jan 6, 2023

[Automated] PRs inserted in VS build main-33306.131 #66298

Closed

Thealexbarney mentioned this pull request Apr 3, 2023

Possible bug related to UnscopedRef introduced in 7.1.200 #67626

Closed

Conversation

cston commented Dec 15, 2022

Uh oh!

jcouv Dec 15, 2022 • edited by cston Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 19, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 19, 2022 • 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 Dec 19, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Dec 19, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Dec 19, 2022

Uh oh!

AlekseyTs Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jan 5, 2023

Uh oh!

AlekseyTs Jan 5, 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!

cston Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Dec 15, 2022 •

edited by cston

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

cston Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading

AlekseyTs Jan 5, 2023 •

edited

Loading