Include [UnscopedRef] parameter attributes in synthesized delegate types by cston · Pull Request #66035 · dotnet/roslyn

cston · 2022-12-16T06:56:31Z

jjonescz · 2022-12-19T13:01:59Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs

        internal sealed override DeclarationScope EffectiveScope => _scope;

-        internal sealed override bool HasUnscopedRefAttribute => false;
+        internal abstract override bool HasUnscopedRefAttribute { get; }


internal abstract override bool HasUnscopedRefAttribute { get; }

Why not remove this property instead? It's inherited exactly as written from ParameterSymbol. #Closed

AlekseyTs · 2022-12-19T13:22:20Z

src/Compilers/Core/Portable/WellKnownMembers.cs

                case WellKnownMember.System_Runtime_CompilerServices_AsyncStateMachineAttribute__ctor:
                case WellKnownMember.System_Runtime_CompilerServices_IteratorStateMachineAttribute__ctor:
                case WellKnownMember.System_Runtime_CompilerServices_AsyncIteratorStateMachineAttribute__ctor:
+                case WellKnownMember.System_Diagnostics_CodeAnalysis_UnscopedRefAttribute__ctor:


WellKnownMember.System_Diagnostics_CodeAnalysis_UnscopedRefAttribute__ctor

I am curious what scenarios does this enable. Is presence of the attribute not important for consumers? #Closed

It looks like we are not testing effect of this change. Also, I think that, unless we have a compelling scenario that we would like to enable, it might be better to detect the lack of the API and report an error instead of silently dropping the attribute. We already do this for default value and param array synthesized attributes.

jjonescz · 2022-12-19T13:22:34Z

src/Compilers/CSharp/Portable/Symbols/AnonymousTypes/AnonymousTypeManager.Templates.cs

            {
+                if (field.HasUnscopedRefAttribute)
+                {
+                    return false;


return false;

Have we considered synthesizing generic templates similarly to what we did with default parameter values recently? For example, synthesizing delegate like delegate void <>f__AnonymousDelegate0<T>([UnscopedRef] ref T x). #Closed

[UnscopedRef] parameters should be uncommon, so perhaps we should hold off making that change for now.

AlekseyTs · 2022-12-19T13:40:32Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/SynthesizedParameterSymbol.cs

                AddSynthesizedAttribute(ref attributes, compilation.TrySynthesizeAttribute(WellKnownMember.System_ParamArrayAttribute__ctor));
            }
+
+            if (this.HasUnscopedRefAttribute && this.ContainingSymbol is SynthesizedDelegateInvokeMethod)


this.ContainingSymbol is SynthesizedDelegateInvokeMetho

Consider unifying this check with the previous if #Closed

Left as is for now, but switched the order of the two if statements to avoid a conflict with the main branch.

AlekseyTs · 2022-12-19T13:51:12Z

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


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


SynthesizedDelegateTypes_UnscopedRefAttribute_01

It feels like we should verify that the attribute is present on the delegate parameter. #Closed

AlekseyTs · 2022-12-19T13:56:52Z

Done with review pass (commit 1)

jjonescz · 2023-01-05T08:38:31Z

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

                {
-                    Binder.AddUseSiteDiagnosticForSynthesizedAttribute(
-                        Compilation,
+                    checkWellKnownMemberAvailable(


checkWellKnownMemberAvailable

Why not use the Binder.AddUseSiteDiagnosticForSynthesizedAttribute method? #Closed

Updated, thanks. (I'm not sure why I'd overlooked AddUseSiteDiagnosticForSynthesizedAttribute previously.)

AlekseyTs · 2023-01-05T17:12:18Z

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

+                <>f__AnonymousDelegate0
+                <>F{00000015}`3[System.Int32,System.Int32,System.Int32]
+                """);
+            verifier.VerifyTypeIL("<>f__AnonymousDelegate1",


f__AnonymousDelegate1

Is f__AnonymousDelegate0 not interesting? #Closed

f__AnonymousDelegate0 is inferred from a method group without [UnscopedRef].

It looks like we are using the attribute on different parameters (first and the second)

Sorry, I'd overlooked the differently named delegate type before <>f__AnonymousDelegate0. Added <>f__AnonymousDelegate0.

AlekseyTs · 2023-01-05T17:16:49Z

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

+            comp = CreateCompilation(sourceB, references: new[] { refA }, targetFramework: TargetFramework.Net60);
+            comp.VerifyEmitDiagnostics(
+                // (6,17): error CS0656: Missing compiler required member 'System.Diagnostics.CodeAnalysis.UnscopedRefAttribute..ctor'
+                //         var d = A.F;


var d = A.F;

Are we testing this scenario for a lambda? Note, it is possible to get in a situation when user is able to apply the attribute in source successfully, but compiler fails to locate the type. We should test this scenario as well (MakeMemberMissing should help) #Closed

AlekseyTs · 2023-01-05T17:18:08Z

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

+
+            comp = CreateCompilation(sourceB, references: new[] { refA }, targetFramework: TargetFramework.Net60);
+            comp.VerifyEmitDiagnostics(
+                // (6,17): error CS0656: Missing compiler required member 'System.Diagnostics.CodeAnalysis.UnscopedRefAttribute..ctor'


System.Diagnostics.CodeAnalysis.UnscopedRefAttribute..ctor

It doesn't look like we are testing scenario when type is present, but constructor is missing. MakeMemberMissing should help. #Closed

AlekseyTs · 2023-01-05T17:22:57Z

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

+                        var d1 = F1;
+                        var d2 = F2;
+                        var d3 = F3;
+                        var d4 = ([UnscopedRef] ref int x, ref int y) => ref x;


([UnscopedRef] ref int x, ref int y) => ref x;

It might be good to separate method group scenarios and lambda scenarios, to make sure they are properly handled on their own. #Closed

AlekseyTs · 2023-01-05T17:26:28Z

Done with review pass (commit 6)

AlekseyTs

LGTM (commit 7)

cston · 2023-01-06T01:21:34Z

/azp run

azure-pipelines · 2023-01-06T01:21:54Z

Azure Pipelines successfully started running 2 pipeline(s).

ghost added the Area-Compilers label Dec 16, 2022

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

cston force-pushed the 63565 branch 3 times, most recently from 33e6860 to 1a661f6 Compare December 16, 2022 22:30

cston marked this pull request as ready for review December 16, 2022 22:30

cston requested a review from a team as a code owner December 16, 2022 22:30

Include [UnscopedRef] parameter attributes in synthesized delegate types

1a661f6

jjonescz reviewed Dec 19, 2022

View reviewed changes

AlekseyTs reviewed Dec 19, 2022

View reviewed changes

jjonescz reviewed Dec 19, 2022

View reviewed changes

AlekseyTs reviewed Dec 19, 2022

View reviewed changes

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

cston added 4 commits January 4, 2023 10:51

Remove unnecessary abstract override

b79268e

Report error for missing UnscopedRefAttribute constructor

23f7519

Remove duplicated code

6d27891

Merge remote-tracking branch 'upstream/release/dev17.5' into 63565

3fff305

jjonescz reviewed Jan 5, 2023

View reviewed changes

Binder.AddUseSiteDiagnosticForSynthesizedAttribute

48a6f51

jjonescz approved these changes Jan 5, 2023

View reviewed changes

AlekseyTs reviewed Jan 5, 2023

View reviewed changes

Address feedback

c634275

AlekseyTs approved these changes Jan 5, 2023

View reviewed changes

jjonescz approved these changes Jan 6, 2023

View reviewed changes

cston merged commit 9b5e355 into dotnet:release/dev17.5 Jan 6, 2023

cston deleted the 63565 branch January 6, 2023 08:28

allisonchou mentioned this pull request Jan 11, 2023

[Automated] PRs inserted in VS build main-33310.323 #66352

Closed

cston mentioned this pull request Jan 17, 2023

Implicitly typed lambdas with [UnscopedRef] don't match their own anonymous delegate types #63565

Closed

Conversation

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

Uh oh!

Uh oh!

jjonescz 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!

AlekseyTs Dec 19, 2022

Choose a reason for hiding this comment

Uh oh!

jjonescz 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!

cston Jan 5, 2023

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!

cston Jan 4, 2023

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 commented Dec 19, 2022

Uh oh!

jjonescz 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

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!

cston Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

cston Jan 5, 2023

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!

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!

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!

AlekseyTs commented Jan 5, 2023

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

cston commented Jan 6, 2023

Uh oh!

azure-pipelines bot commented Jan 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

cston commented Dec 16, 2022 •

edited

Loading

jjonescz Dec 19, 2022 •

edited

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

jjonescz Dec 19, 2022 •

edited

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

AlekseyTs Dec 19, 2022 •

edited

Loading

jjonescz 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