Support UnscopedRefAttribute by cston · Pull Request #62783 · dotnet/roslyn

cston · 2022-07-20T06:18:34Z

See dotnet/runtime#72074.

See low-level-struct-improvements.md.

Corresponding spec update: dotnet/csharplang#6329

Fixes #62780.

Relates to test plan #59194

cston · 2022-07-28T14:21:02Z

cc @jaredpar, @AaronRobinsonMSFT

src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs

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

jcouv · 2022-07-28T18:04:48Z

docs/compilers/CSharp/Compiler Breaking Changes - DotNet 7.md

-A possible workaround is to change the method signature to pass the parameter by `ref` instead.
+Possible workarounds are:
+1. Use `System.Diagnostics.CodeAnalysis.UnscopedRefAttribute` to mark the reference as unscoped.
+    ```csharp


nit: other csharp blocks are left-indented. Not sure whether it's material (does that affect markup rendering on GitHub or docs.microsoft.com), but I'd stick with that to be safe. #Closed

Without indenting, subsequent bullets start at 1.

jcouv · 2022-07-28T22:13:51Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

-                if (_moduleSymbol.Module.HasLifetimeAnnotationAttribute(_handle, out var pair))
+                if (_moduleSymbol.Module.HasUnscopedRefAttribute(_handle))
+                {
+                    scope = DeclarationScope.Unscoped;


Should we mark the parameter as bad if there's both attributes? #Closed

jcouv · 2022-07-28T22:15:35Z

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEParameterSymbol.cs

        }

-        internal sealed override DeclarationScope Scope => _packedFlags.Scope;
+        internal sealed override DeclarationScope DeclaredScope => _packedFlags.Scope;


nit: Is PEParameterSymbol.DeclaredScope reachable (aside from the use on next line)? #Resolved

It looks like PEParameterSymbol.DeclaredScope is only used below, so perhaps we could limit DeclaredScope to source symbols only. That change may be a little involved though, so perhaps that could be done later if necessary.

jcouv · 2022-07-28T23:16:54Z

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

            if (ParameterHelpers.RequiresLifetimeAnnotationAttribute(this))
            {
-                AddSynthesizedAttribute(ref attributes, moduleBuilder.SynthesizeLifetimeAnnotationAttribute(this, Scope));
+                AddSynthesizedAttribute(ref attributes, moduleBuilder.SynthesizeLifetimeAnnotationAttribute(this, DeclaredScope));


DeclaredScope

nit: there's not really a difference but this feels more like EffectiveScope #Closed

I agree, there shouldn't be a difference here, but I think I prefer DeclaredScope since that is what was declared.

Still nit but want to clarify my reasoning: Declared scope is only a step towards producing the real/effective scope. I think most places should use the real/effective scope unless there's a special reason. When it comes to emitting (like here), clearly we should use the real/effective scope, not its precedessor. If we added any more factors into the computations of real/effective scope, we'd want them to affect what we emit.

Currently, we're emitting implicitly scoped parameters with no [ScopedRef] attribute. If we use EffectiveScope for emit, then emitting [UnscopedRef] out int i (where the EffectiveScope is Unscoped) will require ensuring an [UnscopedRef] attribute is emitted, perhaps by assuming an [UnscopedRef] attribute was specified in source. Instead, if we use DeclaredScope here (which is perhaps poorly named since DeclaredScope is (implicitly )RefScoped for out int i), then any [UnscopedRef] from source will provided the expected result in metadata.

A better alternative might be to emit implicitly scoped parameters with a [ScopedRef] attribute. Then EffectiveScope may be simpler here. #63078

jcouv · 2022-07-28T23:19:36Z

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

            if (ParameterHelpers.RequiresLifetimeAnnotationAttribute(this))
            {
-                AddSynthesizedAttribute(ref attributes, moduleBuilder.SynthesizeLifetimeAnnotationAttribute(this, Scope));
+                AddSynthesizedAttribute(ref attributes, moduleBuilder.SynthesizeLifetimeAnnotationAttribute(this, DeclaredScope));


DeclaredScope

nit: Same here, feels like EffectiveScope #Closed

jcouv · 2022-07-28T23:22:13Z

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

+                }
+                return scope;
+
+                static bool hasUnscopedRefAttribute(MethodSymbol? containingMethod, NamedTypeSymbol? containingType)


NamedTypeSymbol? containingType

this parameter is unused #Resolved

jcouv · 2022-07-29T17:13:36Z

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

+}";
+            var comp = CreateCompilation(new[] { source, UnscopedRefAttributeDefinition });
+            comp.VerifyEmitDiagnostics(
+                // (6,23): error CS9064: UnscopedRefAttribute can only be applied to parameters that are passed by reference and implicitly scoped.


UnscopedRefAttribute can only be applied to parameters that are passed by reference and implicitly scoped.

nit: Consider "UnscopedRefAttribute can only be applied to implicitly-scoped 'out' parameters"? #Resolved

UnscopedRefAttribute can also be applied to ref parameters of ref struct types.

Updated the text to cover the two cases as we discussed.

jcouv · 2022-07-29T17:15:17Z

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

+        t3 = default;
+        return ref t3;
+    }
+    public ref T F4A([UnscopedRef] scoped out T t4)


I'd expect an error (this is not "implicitly-scoped") #Closed

jcouv

Done with review pass (iteration 8)

jcouv

LGTM Thanks (iteration 8)

jcouv · 2022-07-30T04:51:27Z

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

+}
+public class A<T>
+{
+    public ref T F1A(ref R<T> r1)


Looking at the verification below (VerifyParameterSymbol(baseType.GetMethod("F1A").Parameters[0], "ref R<System.Int32> r1", RefKind.Ref, DeclarationScope.Unscoped);), it seems we're not treating this parameter as scoped. But shouldn't it be implicitly-scoped according to the rule "ref parameters which refer to a ref struct"? #Closed

The change to treat ref parameters to ref struct types as implicitly scoped is handled in a subsequent PR: #62778.

cston · 2022-07-31T06:33:46Z

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


        internal abstract bool MustCallMethodsDirectly { get; }

+        internal abstract bool HasUnscopedRefAttribute { get; }


HasUnscopedRefAttribute

Is PropertySymbol.HasUnscopedRefAttribute necessary? Should the attribute be supported on properties in general, just on properties from source, or only on accessors directly?

333fred

Done review pass (commit 11). I did not look at tests yet.

...p/Portable/Symbols/Attributes/WellKnownAttributeData/ParameterEarlyWellKnownAttributeData.cs

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEMethodSymbol.cs

src/Compilers/CSharp/Portable/Symbols/Metadata/PE/PEPropertySymbol.cs

src/Compilers/CSharp/Portable/Symbols/Retargeting/RetargetingMethodSymbol.cs

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

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

src/Compilers/CSharp/Portable/CSharpResources.resx

333fred · 2022-08-02T22:27:48Z

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

+        public void UnscopedRefAttribute_Method_01(string type)
+        {
+            var source =
+$@"using System.Diagnostics.CodeAnalysis;


Nit: raw interpolated strings would be more readable here :).

333fred

LGTM (commit 14).

jcouv

LGTM Thanks (iteration 14)

ghost added the Area-Compilers label Jul 20, 2022

cston added the Feature - Ref Fields label Jul 20, 2022

cston force-pushed the RefEscapesAttribute branch 4 times, most recently from 2751f37 to 1574564 Compare July 21, 2022 14:10

cston changed the title ~~Support RefEscapesAttribute~~ Support UnscopedRefAttribute Jul 21, 2022

cston force-pushed the RefEscapesAttribute branch 3 times, most recently from 3f8b7b2 to 6442a41 Compare July 27, 2022 18:57

cston changed the base branch from main to release/dev17.4 July 27, 2022 18:58

cston force-pushed the RefEscapesAttribute branch 5 times, most recently from 85d875c to e1c0157 Compare July 28, 2022 05:52

Support UnscopedRefAttribute

7884b57

cston force-pushed the RefEscapesAttribute branch from e1c0157 to 7884b57 Compare July 28, 2022 14:16

AaronRobinsonMSFT reviewed Jul 28, 2022

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs Show resolved Hide resolved

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

cston marked this pull request as ready for review July 28, 2022 16:37

cston requested a review from a team as a code owner July 28, 2022 16:37

cston mentioned this pull request Jul 28, 2022

VS 17.3 Preview 3 Regression from Preview 2 with ref-returning an out parameter #62900

Closed

jcouv reviewed Jul 28, 2022

View reviewed changes

jcouv self-assigned this Jul 28, 2022

jcouv reviewed Jul 28, 2022

View reviewed changes

Fix build

7a7a084

jcouv reviewed Jul 29, 2022

View reviewed changes

jcouv approved these changes Jul 29, 2022

View reviewed changes

Update error text

2d2deac

This was referenced Jul 29, 2022

Mark parameter from metadata with ScopedRefAttribute and UnscopedRefAttribute as invalid #63070

Closed

Remove or reduce use of ParameterSymbol.DeclaredScope #63078

Closed

jcouv mentioned this pull request Jul 29, 2022

Auto-default ref fields with a null ref-assignment #63065

Merged

jcouv reviewed Jul 30, 2022

View reviewed changes

Add HasUnscopedRefAttribute property to MethodSymbol and PropertySymbol

5c1101a

cston commented Jul 31, 2022

View reviewed changes

cston mentioned this pull request Jul 31, 2022

Update method invocation escape analysis to match spec changes #62886

Merged

Update test

06a3c3b

cston changed the base branch from release/dev17.4 to main August 1, 2022 18:54

333fred reviewed Aug 2, 2022

View reviewed changes

cston mentioned this pull request Aug 2, 2022

Binder cycle leads to NullReferenceException #63140

Closed

Respond to feedback

df68917

cston force-pushed the RefEscapesAttribute branch from 71f521a to df68917 Compare August 2, 2022 19:51

cston added 2 commits August 2, 2022 14:15

Combine errors

28081a1

Merge remote-tracking branch 'upstream/main' into RefEscapesAttribute

ccc843c

333fred reviewed Aug 2, 2022

View reviewed changes

333fred approved these changes Aug 2, 2022

View reviewed changes

jcouv approved these changes Aug 3, 2022

View reviewed changes

cston merged commit 7ab83bb into dotnet:main Aug 3, 2022

cston deleted the RefEscapesAttribute branch August 3, 2022 04:25

ghost added this to the Next milestone Aug 3, 2022

allisonchou mentioned this pull request Aug 9, 2022

[Automated] PRs inserted in VS build main-32808.858 #63272

Closed

dibarbet modified the milestones: Next, 17.4 P2 Sep 1, 2022


		internal abstract bool MustCallMethodsDirectly { get; }

		internal abstract bool HasUnscopedRefAttribute { get; }

Conversation

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

Uh oh!

Uh oh!

cston commented Jul 28, 2022

Uh oh!

Uh oh!

Uh oh!

jcouv Jul 28, 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!

jcouv Jul 28, 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!

jcouv Jul 28, 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 28, 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!

cston Jul 29, 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!

jcouv Jul 28, 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!

jcouv Jul 28, 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!

jcouv Jul 29, 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!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 29, 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!

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

jcouv Jul 30, 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!

333fred left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cston commented Jul 20, 2022 •

edited by jcouv

Loading

jcouv Jul 28, 2022 •

edited

Loading

jcouv Jul 28, 2022 •

edited

Loading

jcouv Jul 28, 2022 •

edited by cston

Loading

jcouv Jul 28, 2022 •

edited

Loading

cston Jul 29, 2022 •

edited

Loading

jcouv Jul 28, 2022 •

edited

Loading

jcouv Jul 28, 2022 •

edited by cston

Loading

jcouv Jul 29, 2022 •

edited by cston

Loading

jcouv Jul 29, 2022 •

edited

Loading

jcouv Jul 30, 2022 •

edited

Loading