Support UnscopedRefAttribute #62783

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

@@ -978,7 +982,9 @@ public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences
            }
        }

-        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

@@ -162,7 +162,7 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r

            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

@@ -101,7 +101,7 @@ internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, r

            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

@@ -218,6 +218,8 @@ public bool IsWriteOnly

        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)

dotnet-issue-labeler bot 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support UnscopedRefAttribute #62783

Support UnscopedRefAttribute #62783

cston commented Jul 20, 2022 •

edited by jcouv

Loading

cston commented Jul 28, 2022

jcouv Jul 28, 2022 •

edited

Loading

cston Jul 28, 2022

jcouv Jul 28, 2022 •

edited

Loading

cston Jul 29, 2022

jcouv Jul 28, 2022 •

edited by cston

Loading

cston Jul 29, 2022

cston Aug 2, 2022

jcouv Jul 28, 2022 •

edited

Loading

cston Jul 29, 2022

jcouv Jul 29, 2022

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

cston Jul 29, 2022

cston Jul 29, 2022

jcouv Jul 29, 2022 •

edited

Loading

cston Jul 29, 2022

jcouv left a comment

jcouv left a comment

jcouv Jul 30, 2022 •

edited

Loading

cston Jul 30, 2022

cston Jul 31, 2022

333fred left a comment

333fred Aug 2, 2022

333fred left a comment

jcouv left a comment

		@@ -218,6 +218,8 @@ public bool IsWriteOnly

		internal abstract bool MustCallMethodsDirectly { get; }

		internal abstract bool HasUnscopedRefAttribute { get; }

Support UnscopedRefAttribute #62783

Support UnscopedRefAttribute #62783

Conversation

cston commented Jul 20, 2022 • edited by jcouv Loading

cston commented Jul 28, 2022

jcouv Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 28, 2022 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 28, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv Jul 28, 2022 • edited by cston Loading

Choose a reason for hiding this comment

jcouv Jul 29, 2022 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jul 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Jul 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

333fred left a comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

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