Address PROTOTYPE comments in features/ref-fields #62088

cston · 2022-06-22T21:03:03Z

Address PROTOTYPE comments.

Also fixes #62085.

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

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

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

…m.Runtime.CompilerServices.RuntimeFeature.ByRefFields

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeExtensions.cs

AlekseyTs · 2022-06-24T19:39:33Z

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

@@ -326,7 +326,6 @@ private void EnsureSignatureIsLoaded()
                _packedFlags.SetRefKind(refKind);
                _packedFlags.SetIsVolatile(customModifiersArray.Any(static m => !m.IsOptional && ((CSharpCustomModifier)m).ModifierSymbol.SpecialType == SpecialType.System_Runtime_CompilerServices_IsVolatile));

-                // PROTOTYPE: `fixed ref` field use should be disallowed.


// PROTOTYPE: fixed ref field use should be disallowed.

I didn't see any special handling around that for PE. #Closed

AlekseyTs · 2022-06-24T19:42:08Z

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

@@ -326,7 +326,6 @@ private void EnsureSignatureIsLoaded()
                _packedFlags.SetRefKind(refKind);
                _packedFlags.SetIsVolatile(customModifiersArray.Any(static m => !m.IsOptional && ((CSharpCustomModifier)m).ModifierSymbol.SpecialType == SpecialType.System_Runtime_CompilerServices_IsVolatile));

-                // PROTOTYPE: `fixed ref` field use should be disallowed.


// PROTOTYPE: fixed ref field use should be disallowed.

Are we tracking similar issue for volatile or already handling that? #Closed

AlekseyTs · 2022-06-24T19:49:52Z

        // PROTOTYPE: `fixed ref` field use should be disallowed.

The comment doesn't appear to be addressed. There probably should be an error about the field being unsupported.

In reply to: 1165887859

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:454 in 9ace414. [](commit_id = 9ace414, deletion_comment = True)

AlekseyTs · 2022-06-24T19:58:43Z

Done with review pass (commit 10)

RikkiGibson · 2022-06-24T21:31:34Z

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

@@ -55,7 +55,7 @@ protected override ImmutableArray<LocalSymbol> BuildLocals()

                foreach (VariableDeclaratorSyntax declarator in declarationSyntax.Variables)
                {
-                    locals.Add(MakeLocal(declarationSyntax, declarator, LocalDeclarationKind.UsingVariable, hasScopedModifier: false)); // PROTOTYPE: Handle 'scoped' modifier.
+                    locals.Add(MakeLocal(declarationSyntax, declarator, LocalDeclarationKind.UsingVariable, hasScopedModifier: false));


if a tracking issue exists for these, consider replacing the prototype comment with a link to the issue rather than deleting. #Resolved

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

RikkiGibson · 2022-06-25T00:05:01Z

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

-            // PROTOTYPE: Should report duplicate modifiers.
-            comp.VerifyEmitDiagnostics();
+            comp.VerifyEmitDiagnostics(
+                // (4,27): error CS1107: A parameter can only have one 'scoped' modifier


I found this slightly surprising. I thought we were going to make 'scoped' as a type name an error like we did for 'required' as a type name. #Resolved

We're not reporting an error for scoped as a type name yet.

RikkiGibson · 2022-06-25T00:06:50Z

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

+    static void F1(this scoped R<object> r) { }
+    static void F2<T>(scoped this R<T> r) { }
+    static void F3<T>(this scoped ref T t) where T : struct { }
+    static void F4(this ref scoped R<int> r) { }


consider also checking a this parameter with no scoped modifiers as a baseline. #Resolved

AlekseyTs · 2022-06-27T11:47:07Z

.field public int32& modreq([mscorlib]System.Runtime.CompilerServices.IsVolatile) F

Are we testing scenario with the modifier before the &?

In reply to: 1167252641

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:473 in 972408c. [](commit_id = 972408c, deletion_comment = False)

AlekseyTs

LGTM (commit 13), assuming CI is passing

dotnet-issue-labeler bot added the Area-Compilers label Jun 22, 2022

cston added the Feature - Ref Fields label Jun 22, 2022

cston force-pushed the ref-fields-cleanup branch 5 times, most recently from 85ef462 to c8a7b60 Compare June 24, 2022 03:49

Address PROTOTYPE comments

0574002

cston force-pushed the ref-fields-cleanup branch from c8a7b60 to 0574002 Compare June 24, 2022 05:14

cston marked this pull request as ready for review June 24, 2022 05:26

cston requested a review from a team as a code owner June 24, 2022 05:26

Misc.

4181079

cston changed the title ~~Address comments referencing work items in features/ref-fields~~ Address PROTOTYPE comments in features/ref-fields Jun 24, 2022

cston added 2 commits June 24, 2022 06:19

Disable error for LifetimeAnnotationAttribute in source

a07467b

Add SyntaxNormalizer tests

f3d99d3

cston requested review from RikkiGibson and AlekseyTs June 24, 2022 15:17

AlekseyTs reviewed Jun 24, 2022

View reviewed changes

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

AlekseyTs reviewed Jun 24, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Symbols/Symbol.cs Outdated Show resolved Hide resolved

RikkiGibson self-assigned this Jun 24, 2022

cston added 5 commits June 24, 2022 11:11

Use updated escape rules for -langversion:preview only, not for Syste…

d9b7f17

…m.Runtime.CompilerServices.RuntimeFeature.ByRefFields

Removed ReservedAttributes.LifetimeAnnotationAttribute

f91de79

Add workaround

025d883

Update workaround

e1aa9e8

Removed ReservedAttributes.LifetimeAnnotationAttribute

9ace414

AlekseyTs reviewed Jun 24, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Syntax/SyntaxNodeExtensions.cs Show resolved Hide resolved

AlekseyTs reviewed Jun 24, 2022

View reviewed changes

Add assert

88e23ca

Add use-site error for fixed ref field

76cdbb7

RikkiGibson approved these changes Jun 25, 2022

View reviewed changes

PR feedback

972408c

Volatile

a889716

AlekseyTs approved these changes Jun 27, 2022

View reviewed changes

cston merged commit f27dec7 into dotnet:features/ref-fields Jun 27, 2022

cston deleted the ref-fields-cleanup branch June 27, 2022 15:24

cston mentioned this pull request Jul 14, 2022

Missing error for invalid 'scoped' modifier on return type (including no errors in C# 10) #62645

Closed

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

Address PROTOTYPE comments in features/ref-fields #62088

Address PROTOTYPE comments in features/ref-fields #62088

cston commented Jun 22, 2022 •

edited

Loading

AlekseyTs Jun 24, 2022 •

edited

Loading

AlekseyTs Jun 24, 2022 •

edited

Loading

AlekseyTs commented Jun 24, 2022 •

edited

Loading

AlekseyTs commented Jun 24, 2022

RikkiGibson Jun 24, 2022 •

edited by cston

Loading

RikkiGibson Jun 25, 2022 •

edited by cston

Loading

cston Jun 25, 2022

RikkiGibson Jun 25, 2022 •

edited by cston

Loading

AlekseyTs commented Jun 27, 2022 •

edited

Loading

AlekseyTs left a comment

Address PROTOTYPE comments in features/ref-fields #62088

Address PROTOTYPE comments in features/ref-fields #62088

Conversation

cston commented Jun 22, 2022 • edited Loading

AlekseyTs Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 24, 2022 • edited Loading

AlekseyTs commented Jun 24, 2022

RikkiGibson Jun 24, 2022 • edited by cston Loading

Choose a reason for hiding this comment

RikkiGibson Jun 25, 2022 • edited by cston Loading

Choose a reason for hiding this comment

cston Jun 25, 2022

Choose a reason for hiding this comment

RikkiGibson Jun 25, 2022 • edited by cston Loading

Choose a reason for hiding this comment

AlekseyTs commented Jun 27, 2022 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

cston commented Jun 22, 2022 •

edited

Loading

AlekseyTs Jun 24, 2022 •

edited

Loading

AlekseyTs Jun 24, 2022 •

edited

Loading

AlekseyTs commented Jun 24, 2022 •

edited

Loading

RikkiGibson Jun 24, 2022 •

edited by cston

Loading

RikkiGibson Jun 25, 2022 •

edited by cston

Loading

RikkiGibson Jun 25, 2022 •

edited by cston

Loading

AlekseyTs commented Jun 27, 2022 •

edited

Loading