Support 'scoped' modifier for parameters and locals #61389

cston · 2022-05-18T21:06:49Z

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

Parse and bind scoped modifier for parameters and locals; report binding error for scoped when compiling with -langversion:10 or earlier; emit modifier to metadata with LifetimeAnnotationAttribute. The scoped modifier can be applied to ref or to ref struct values.

The PR does not include:

Changes to escape rules for scoped variables (see spec)
conversions involving scoped
scoped considered in overrides and interface implementation

CyrusNajmabadi · 2022-05-19T16:37:45Z

src/Compilers/CSharp/Portable/Syntax/Syntax.xml

@@ -412,6 +412,9 @@
        <summary>Gets the optional "readonly" keyword.</summary>
      </PropertyComment>
    </Field>
+    <Field Name="ScopedKeyword" Type="SyntaxToken" Optional="true">
+      <Kind Name="ScopedKeyword"/>
+    </Field>


part of the type syntax, and not part of the parameter syntax? #Resolved

The feature supports both scoped ref Span<int> param and ref scoped Span<int> param and they mean different things. Haven't looked closely at this PR yet but it doesn't surprise me that Scoped is part of the ParameterSyntax directly.

It doesn't surprise me that scoped goes into the Modifiers on a ParameterSyntax. I mentioned to Chuck offline that it would be best if our parse for readonly ref readonly Span<int> local; has a similar parse as scoped ref scoped Span<int> local;. I think this makes them match. Although this makes me wonder if this ReadOnlyKeyword should "grow up" into a generalized trailing Modifiers list.

Neme12 · 2022-05-20T17:48:33Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -1163,6 +1165,8 @@ internal static DeclarationModifiers GetModifier(SyntaxKind kind, SyntaxKind con
                            return DeclarationModifiers.Partial;
                        case SyntaxKind.AsyncKeyword:
                            return DeclarationModifiers.Async;
+                        case SyntaxKind.ScopedKeyword:
+                            return AllowScopedModifier() ? DeclarationModifiers.Scoped : DeclarationModifiers.None;


Wouldn't it be better if the keyword was parsed regardless of language version, but in a lower version it would show an error that the feature isn't available? That would make for a better experience when users try this feature but didn't upgrade their lang version, and would also allow the language version code fix to work. #Resolved

I agree. I think the 'file' or 'required' parsing changes should end up looking similar to what we do in here. https://github.com/dotnet/roslyn/pull/58431/files#diff-a655d393d9799eabace5e5109383232749be8052f1fd825fc52b9f170908e838

Thanks. I'd initially expected breaking changes parsing scoped as a modifier, so I'd made the modifier conditional on language version, but the syntax may be unambiguous after all. I've updated the parser to allow scoped as a modifier unconditionally, and moved the language version check to binding.

RikkiGibson

Haven't looked at DeclarationScopeParsingTests yet. Looks like we're on a good track but did have some feedback and questions.

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

src/Compilers/CSharp/Portable/Declarations/DeclarationModifiers.cs

src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs

RikkiGibson · 2022-05-20T20:55:55Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -1163,6 +1165,8 @@ internal static DeclarationModifiers GetModifier(SyntaxKind kind, SyntaxKind con
                            return DeclarationModifiers.Partial;
                        case SyntaxKind.AsyncKeyword:
                            return DeclarationModifiers.Async;
+                        case SyntaxKind.ScopedKeyword:
+                            return AllowScopedModifier() ? DeclarationModifiers.Scoped : DeclarationModifiers.None;


I agree. I think the 'file' or 'required' parsing changes should end up looking similar to what we do in here. https://github.com/dotnet/roslyn/pull/58431/files#diff-a655d393d9799eabace5e5109383232749be8052f1fd825fc52b9f170908e838

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

RikkiGibson · 2022-05-21T01:43:16Z

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

+    static unsafe void Main()
+    {
+        delegate*<scoped R, void> f1 = &F1;
+        delegate*<ref scoped int, scoped ref int, void> f2 = &F2;


let's make sure we test scenarios where 'scoped' doesn't match as well. #Pending

RikkiGibson · 2022-05-21T01:45:36Z

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

+        }
+
+        // PROTOTYPE: Report error for implicit conversion between delegate types that differ by 'scoped',
+        // and between function pointer types and methods that differ by 'scoped'.


seems like all conversions should be disallowed when there are 'scoped' differences. It actually makes me wonder if a 'modreq' is needed to prevent downlevel compilers from attempting to perform such conversions. #Pending

I've added an item to the test plan, and I'll address this in a subsequent PR.

src/Compilers/CSharp/Test/Syntax/Parsing/ParsingTests.cs

src/Compilers/Core/Portable/MetadataReader/PEModule.cs

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

cston · 2022-05-23T06:37:52Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -1283,7 +1296,7 @@ bool isStructOrRecordKeyword(SyntaxToken token)

        private bool ShouldAsyncBeTreatedAsModifier(bool parsingStatementNotDeclaration)
        {
-            Debug.Assert(this.CurrentToken.ContextualKind == SyntaxKind.AsyncKeyword);
+            Debug.Assert(this.CurrentToken.Kind == SyntaxKind.IdentifierToken && GetModifier(this.CurrentToken) != DeclarationModifiers.None);


Debug.Assert

The updated assert is from #60823 where this method is also renamed. #Resolved

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

CyrusNajmabadi · 2022-05-26T04:18:58Z

src/Workspaces/Core/Portable/CodeGeneration/Symbols/CodeGenerationParameterSymbol.cs

@@ -68,6 +68,10 @@ public override TResult Accept<TResult>(SymbolVisitor<TResult> visitor)

        public ImmutableArray<CustomModifier> CustomModifiers => ImmutableArray.Create<CustomModifier>();

+        public bool IsRefScoped => false;
+
+        public bool IsValueScoped => false;


interesting name. curious what the intuition is on 'value scoped' #Resolved

The name is intended to indicate that the members of the ref struct, that is, the value of the ref struct, does not escape the current method.

AlekseyTs · 2022-06-02T01:02:20Z

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

+                (false, false) => DeclarationScope.Unscoped,
+                (false, true) => type.IsRefLikeType ? DeclarationScope.ValueScoped : null,
+                (true, false) => refKind != RefKind.None ? DeclarationScope.RefScoped : null,
+                (true, true) => null,


(true, true) => null,

The spec says: "The declaration scoped ref scoped Span<int> is allowed but is redundant with ref scoped Span<int>." Why not handle this combination this way? #Closed

AlekseyTs · 2022-06-02T01:21:26Z

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

@@ -19,7 +18,7 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
    /// <summary>
    /// A source parameter, potentially with a default value, attributes, etc.
    /// </summary>
-    internal class SourceComplexParameterSymbol : SourceParameterSymbol, IAttributeTargetSymbol
+    internal abstract class SourceComplexParameterSymbol : SourceParameterSymbol, IAttributeTargetSymbol


abstract

I do not see any advantage in making this type abstract especially that now we are forced to create a bigger type where SourceComplexParameterSymbol would work. If the goal is to make RefCustomModifiers property abstract, then we can rename this type to SourceComplexParameterSymbolBase and add sealed derived type called SourceComplexParameterSymbol with old implementation of the property. "Restoring" logic at creation sites. #Closed

AlekseyTs · 2022-06-02T01:32:27Z

        RefKind refKind,

It feels like we should add DeclarationScope to this class since we handle RefKind here as well. This way we will be able to keep the original logic above and create SourceSimpleParameterSymbol instances in more scenarios.

In reply to: 1144323560

Refers to: src/Compilers/CSharp/Portable/Symbols/Source/SourceParameterSymbol.cs:90 in 88f0053. [](commit_id = 88f0053, deletion_comment = False)

AlekseyTs · 2022-06-02T01:36:44Z

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


            if (!isParams &&
                !isExtensionMethodThis &&
+                scope == DeclarationScope.Unscoped &&


scope == DeclarationScope.Unscoped &&

I do not think addition of this condition is justified. We should still be able to create SourceSimpleParameterSymbol in this case. SourceComplexParameterSymbol doesn't add any interesting behavior around DeclarationScope that SourceSimpleParameterSymbol cannot handle, other than the storage, of course. As I suggested below, we should simply move the storage to this class and restore the name for SourceComplexParameterSymbolWithCustomModifiersPrecedingByRef class. #Closed

AlekseyTs · 2022-06-02T01:40:48Z

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

@@ -173,5 +173,7 @@ internal override bool IsMetadataOut
        internal override ImmutableArray<int> InterpolatedStringHandlerArgumentIndexes => ImmutableArray<int>.Empty;

        internal override bool HasInterpolatedStringHandlerArgumentError => false;
+
+        internal override DeclarationScope Scope => DeclarationScope.Unscoped;


DeclarationScope.Unscoped

Is this the right value for this parameter? The spec says the following: "scoped ref T is the default type of this." #Closed

AlekseyTs · 2022-06-02T02:05:33Z

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

@@ -210,12 +237,13 @@ internal override string GetDebuggerDisplay()
            SyntaxToken identifierToken,
            LocalDeclarationKind declarationKind,
            EqualsValueClauseSyntax initializer = null,
-            Binder initializerBinderOpt = null)
+            Binder initializerBinderOpt = null,
+            bool scopedModifier = false)


bool scopedModifier = false

Were all existing call sites examined in case an explicit value should be passed? #Closed

AlekseyTs · 2022-06-02T02:17:27Z

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

@@ -313,7 +314,7 @@ internal void BuildLocalFunctions(StatementSyntax statement, ref ArrayBuilder<Lo
            }
        }

-        protected SourceLocalSymbol MakeLocal(VariableDeclarationSyntax declaration, VariableDeclaratorSyntax declarator, LocalDeclarationKind kind, Binder initializerBinderOpt = null)
+        protected SourceLocalSymbol MakeLocal(VariableDeclarationSyntax declaration, VariableDeclaratorSyntax declarator, LocalDeclarationKind kind, Binder initializerBinderOpt = null, bool scopedModifier = false)


, bool scopedModifier = false

Were all call sites examined incase an explicit value should be passed? #Closed

AlekseyTs · 2022-06-02T02:26:39Z

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

+                (true, true) => null,
+            };
+        }
+
        public override ImmutableArray<CSharpAttributeData> GetAttributes()


GetAttributes

Should LifetimeAnnotationAttribute be filtered out by this method? #Closed

AlekseyTs · 2022-06-02T02:27:19Z

Done with review pass (commit 22)

AlekseyTs · 2022-06-02T14:20:41Z

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

@@ -215,7 +215,7 @@ public static bool IsSourceParameterWithEnumeratorCancellationAttribute(this Par
        {
            switch (parameter)
            {
-                case SourceComplexParameterSymbol source:
+                case SourceComplexParameterSymbolBase source:


SourceComplexParameterSymbolBase

Is this change related to the feature? #Closed

AlekseyTs · 2022-06-02T14:37:46Z

Done with review pass (commit 23)

AlekseyTs · 2022-06-03T15:07:57Z

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

@@ -980,12 +1016,12 @@ public override ImmutableArray<CSharpAttributeData> GetAttributes()
                }

                bool filterIsReadOnlyAttribute = this.RefKind == RefKind.In;
+                bool filterLifetimeAnnotationAttribute = Scope != DeclarationScope.Unscoped;


Scope != DeclarationScope.Unscoped

I think the attribute should be filtered out even in this condition. It looks like it can be used to encode this value as well. #Closed

AlekseyTs · 2022-06-03T15:34:25Z

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

+        {
+            Assert.Equal(expectedRefKind, parameter.RefKind);
+            // https://github.com/dotnet/roslyn/issues/61647: Use public API.
+            //Assert.Equal(expectedScope == DeclarationScope.RefScoped, parameter.IsRefScoped);


//Assert.Equal(expectedScope == DeclarationScope.RefScoped, parameter.IsRefScoped);

Is it worth testing through internal API for now? #Closed

The only caller is the preceding method which tests the internal API ParameterSymbol.Scope.

AlekseyTs · 2022-06-03T15:34:45Z

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

+        {
+            Assert.Equal(expectedRefKind, local.RefKind);
+            // https://github.com/dotnet/roslyn/issues/61647: Use public API.
+            //Assert.Equal(expectedScope == DeclarationScope.RefScoped, local.IsRefScoped);


//Assert.Equal(expectedScope == DeclarationScope.RefScoped, local.IsRefScoped);

Is it worth testing through internal API for now? #Closed

The only caller is the preceding method which tests the internal API LocalSymbol.Scope.

AlekseyTs

LGTM (commit 33), assuming CI is passing.

Support 'scoped' modifier for parameters and locals

0055014

dotnet-issue-labeler bot added the Area-Compilers label May 18, 2022

cston marked this pull request as ready for review May 18, 2022 21:16

cston requested review from a team as code owners May 18, 2022 21:16

cston added the Feature - Ref Fields label May 18, 2022

cston mentioned this pull request May 18, 2022

Ref fields escape analysis #61195

Merged

RikkiGibson self-assigned this May 18, 2022

cston force-pushed the scoped-keyword branch from 4ef51cc to c6f7e22 Compare May 19, 2022 03:49

Misc.

405ce6a

cston force-pushed the scoped-keyword branch from c6f7e22 to 405ce6a Compare May 19, 2022 04:56

CyrusNajmabadi reviewed May 19, 2022

View reviewed changes

Neme12 reviewed May 20, 2022

View reviewed changes

cston added 2 commits May 20, 2022 14:58

Missing ConvertToKeyword()

c67d771

Bind scope for lambda parameters

35672d2

RikkiGibson reviewed May 21, 2022

View reviewed changes

cston added 2 commits May 22, 2022 23:28

PR feedback

88de445

Update parsing

322a61c

cston commented May 23, 2022

View reviewed changes

runfoapp bot mentioned this pull request May 23, 2022

roslyn.visualstudio.newintegrationtests.csharp.csharperrorlistnetcore.errorlist #61367

Open

Report error if scoped value is not ref struct

0d48f5a

cston force-pushed the scoped-keyword branch from e90cda4 to 0d48f5a Compare May 24, 2022 23:30

cston added 3 commits May 24, 2022 21:40

Fix tests

413b85b

More tests

6406e1c

Add IsRefScoped and IsValueScoped to public API

f24747d

RikkiGibson reviewed May 25, 2022

View reviewed changes

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

cston requested a review from a team as a code owner May 26, 2022 04:07

Fix build

290be82

cston force-pushed the scoped-keyword branch from d425261 to 290be82 Compare May 26, 2022 04:14

CyrusNajmabadi reviewed May 26, 2022

View reviewed changes

AlekseyTs reviewed Jun 2, 2022

View reviewed changes

PR feedback

7d31323

AlekseyTs reviewed Jun 2, 2022

View reviewed changes

cston added 9 commits June 2, 2022 09:19

Disallow scoped in function pointer signatures

3432100

PR feedback

1888ac7

Filter out attribute from PEParameterSymbol.GetAttributes()

6028395

Revert some public API changes

d90e587

Formatting

452d6a5

Fix tests

7d82030

Revert public API changes

65fb9fd

Rename helper method

436677a

Fix ScanExplicitlyTypedLambda()

5a3815a

AlekseyTs reviewed Jun 3, 2022

View reviewed changes

Filter out attribute unconditionally

3c5d140

AlekseyTs reviewed Jun 3, 2022

View reviewed changes

AlekseyTs approved these changes Jun 3, 2022

View reviewed changes

RikkiGibson approved these changes Jun 3, 2022

View reviewed changes

cston merged commit 75ac25c into dotnet:features/ref-fields Jun 3, 2022

cston deleted the scoped-keyword branch June 3, 2022 18:50

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

Support 'scoped' modifier for parameters and locals #61389

Support 'scoped' modifier for parameters and locals #61389

cston commented May 18, 2022 •

edited

Loading

CyrusNajmabadi May 19, 2022 •

edited by cston

Loading

RikkiGibson May 19, 2022 •

edited

Loading

Neme12 May 20, 2022 •

edited by cston

Loading

RikkiGibson May 20, 2022

cston May 26, 2022

RikkiGibson left a comment •

edited

Loading

RikkiGibson May 20, 2022

RikkiGibson May 21, 2022 •

edited by cston

Loading

RikkiGibson May 21, 2022 •

edited by cston

Loading

cston May 26, 2022

cston May 23, 2022 •

edited

Loading

CyrusNajmabadi May 26, 2022 •

edited by cston

Loading

cston May 26, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs commented Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs commented Jun 2, 2022

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs commented Jun 2, 2022

AlekseyTs Jun 3, 2022 •

edited

Loading

AlekseyTs Jun 3, 2022 •

edited

Loading

cston Jun 3, 2022

AlekseyTs Jun 3, 2022 •

edited

Loading

cston Jun 3, 2022

AlekseyTs left a comment

Support 'scoped' modifier for parameters and locals #61389

Support 'scoped' modifier for parameters and locals #61389

Conversation

cston commented May 18, 2022 • edited Loading

CyrusNajmabadi May 19, 2022 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson May 19, 2022 • edited Loading

Choose a reason for hiding this comment

Neme12 May 20, 2022 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson May 20, 2022

Choose a reason for hiding this comment

cston May 26, 2022

Choose a reason for hiding this comment

RikkiGibson left a comment • edited Loading

Choose a reason for hiding this comment

RikkiGibson May 20, 2022

Choose a reason for hiding this comment

RikkiGibson May 21, 2022 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson May 21, 2022 • edited by cston Loading

Choose a reason for hiding this comment

cston May 26, 2022

Choose a reason for hiding this comment

cston May 23, 2022 • edited Loading

Choose a reason for hiding this comment

CyrusNajmabadi May 26, 2022 • edited by cston Loading

Choose a reason for hiding this comment

cston May 26, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 2, 2022 • edited Loading

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 2, 2022

AlekseyTs Jun 2, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 2, 2022

AlekseyTs Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

cston Jun 3, 2022

Choose a reason for hiding this comment

AlekseyTs Jun 3, 2022 • edited Loading

Choose a reason for hiding this comment

cston Jun 3, 2022

Choose a reason for hiding this comment

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented May 18, 2022 •

edited

Loading

CyrusNajmabadi May 19, 2022 •

edited by cston

Loading

RikkiGibson May 19, 2022 •

edited

Loading

Neme12 May 20, 2022 •

edited by cston

Loading

RikkiGibson left a comment •

edited

Loading

RikkiGibson May 21, 2022 •

edited by cston

Loading

RikkiGibson May 21, 2022 •

edited by cston

Loading

cston May 23, 2022 •

edited

Loading

CyrusNajmabadi May 26, 2022 •

edited by cston

Loading

cston May 26, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs commented Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 2, 2022 •

edited

Loading

AlekseyTs Jun 3, 2022 •

edited

Loading

AlekseyTs Jun 3, 2022 •

edited

Loading

AlekseyTs Jun 3, 2022 •

edited

Loading