Support ref field assignment in object initializers by jcouv · Pull Request #62584 · dotnet/roslyn

jcouv · 2022-07-12T23:47:05Z

Fixes #62120
Corresponding spec update: dotnet/csharplang#6277
Relates to test plan #59194

Youssef1313 · 2022-07-13T07:51:11Z

Fixes #62578

Should be #62120?

jcouv · 2022-07-14T23:04:12Z

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

-                    {
-                        // D = { ..., <identifier> = <expr>, ... }, where D : dynamic
-                        var memberName = ((IdentifierNameSyntax)leftSyntax).Identifier.Text;
-                        boundLeft = new BoundDynamicObjectInitializerMember(leftSyntax, memberName, implicitReceiver.Type, initializerType, hasErrors: false);


📝 The binding of dynamic LHS was moved into BindObjectInitializerMember which handled other bindings of LHS. #Resolved

jcouv · 2022-07-14T23:11:45Z

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

            {
-                return this.ParseExpressionCore();
+                // <expr>
+                // ref <expr>


📝 This case and the dictionary initializer case above remain errors in binding, but the parser now tolerates ref (more graceful failure). #Resolved

jcouv · 2022-07-18T15:59:13Z

@cston @RikkiGibson for review. Thanks

cston · 2022-07-18T21:03:49Z

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

                case BoundKind.ArrayAccess:
                case BoundKind.PointerElementAccess:
-                    return boundMember;
+                    return CheckValue(boundMember, valueKind, diagnostics);


CheckValue

Was this a missing case previously? #Closed

Right. We were returning so bypassing the CheckValueKind check below.
Without this fix, we'd have no error in RefInitializer_OnArray or RefInitializer_OnPointer

cston · 2022-07-18T21:12:49Z

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

+                }
+                if (expr is BoundArrayAccess)
+                {
+                    return "array access";


"array access"

Do these strings need to be localized? #Closed

cston · 2022-07-18T21:21:29Z

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

+    }
+}
+";
+            var comp = CreateCompilation(source, parseOptions: TestOptions.Regular11);


parseOptions: TestOptions.Regular11

Should parseOptions be dropped, to ensure we're testing with the latest language version? #Closed

cston · 2022-07-18T21:42:14Z

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

+";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();
+        }


}

Consider testing assignment of in parameter:

int x = 42; static void F(in int i) { _ = new R1 { _f = ref i }; _ = new R2 { _f = ref i }; } ref struct R1 { public ref readonly int _f; } ref struct R2 { public ref int _f; } ``` #Closed

Added RefInitializer_AssignInParameter

cston · 2022-07-18T21:53:15Z

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

+{
+    public dynamic D;
+}
+ref struct R


R

Minor: R is unused. #Closed

cston · 2022-07-18T21:56:12Z

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

+        {
+            var source = @"
+int x = 42;
+var r = new dynamic { field = ref x }; // 1


Perhaps:

int i = 42; var r = new R<dynamic> { F = ref i }; ref struct R<T> { public ref T F; } ``` #Closed

Added RefInitializer_DynamicField

cston · 2022-07-19T17:22:05Z

        comp = CreateCompilation(sourceB, references: new[] { refA }, parseOptions: TestOptions.Regular11);

I think this parseOptions: should be kept, to ensure we're not reporting an error for -langversion:11 specifically, not just the latest version.

In reply to: 1189357786

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RefFieldTests.cs:152 in 6980e20. [](commit_id = 6980e20, deletion_comment = True)

jcouv · 2022-07-19T17:24:42Z

I think this parseOptions: should be kept, to ensure we're not reporting an error for -langversion:11 specifically, not just the latest version.

Yes, I've reverted that line just after pushing. It was over-eager search & replace

In reply to: 1189360227

RikkiGibson · 2022-07-19T18:57:37Z

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

+                // var r = new R() { field = ref x }; // 1
+                Diagnostic(ErrorCode.ERR_FeatureNotAvailableInVersion10, "field").WithArguments("ref fields", "11.0").WithLocation(3, 19),
+                // (6,15): error CS8936: Feature 'ref fields' is not available in C# 10.0. Please use language version 11.0 or greater.
+                // _ = r2 with { field = ref x }; // 2


do we separately have a test which verifies the use site diagnostic when assigning by value here? #Resolved

RikkiGibson · 2022-07-19T19:01:22Z

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

+        [Fact]
+        public void RefInitializer_RHSTypeMustMatch()
+        {
+            // The right operand must be an expression that yields an lvalue designating a value of the same type as the left operand.


consider testing a case where there is an implicit conversion from the RHS type to the LHS type. #Resolved

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

RikkiGibson · 2022-07-19T19:15:11Z

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

+                // var r = new C { a = ref x }; // 1
+                Diagnostic(ErrorCode.ERR_BadEventUsage, "a").WithArguments("C.a", "C").WithLocation(3, 17),
+                // (6,3): error CS0070: The event 'C.a' can only appear on the left hand side of += or -= (except when used from within the type 'C')
+                // c.a = ref x; // 2


might be nice to verify the behavior here on e.g. void M() { this.a = ref x; } or void M() { this.a += ref x; }. #Resolved

RikkiGibson · 2022-07-19T19:17:43Z

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

+            comp.VerifyDiagnostics(
+                // (3,34): error CS8173: The expression must be of type 'dynamic' because it is being assigned by reference
+                // var r = new R<dynamic> { F = ref i }; // 1
+                Diagnostic(ErrorCode.ERR_RefAssignmentMustHaveIdentityConversion, "i").WithArguments("dynamic").WithLocation(3, 34)


consider including a success case here (e.g. dynamic i = 42). #Resolved

RikkiGibson · 2022-07-19T19:19:40Z

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

+                Diagnostic(ErrorCode.ERR_RefReturnLocal, "field = ref x").WithArguments("x").WithLocation(36, 21)
+                );
+
+            source = @"


consider makign this a separate test and/or including a comment which indicates what the difference is between the two scenarios. #Resolved

RikkiGibson · 2022-07-19T19:21:23Z

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

+        Container r = default;
+        {
+            int x = 42;
+            var r = new Container { item = { field = ref x } }; // 1


consider including some success cases. #Resolved

RikkiGibson · 2022-07-19T19:22:54Z

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

+
+        [Theory]
+        [InlineData(LanguageVersion.CSharp10)]
+        [InlineData(LanguageVersionFacts.CSharpNext)]


nit: I expected use of LanguageVersion.CSharp11 here instead. #Resolved

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

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

333fred

LGTM (commit 6)

cston · 2022-07-20T22:56:17Z

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

+}
+";
+            var comp = CreateCompilation(source);
+            comp.VerifyDiagnostics();


Does the generated code run successfully? Consider running the code. #Resolved

* upstream/main: (53 commits) Remove 'mangleName' parameter in PENamedTypeSymbolNonGeneric (dotnet#62813) Produce errors for 'partial file record' (dotnet#62686) [EnC] Allow renaming methods, properties, events etc. (dotnet#62364) Update AbstractFileHeaderDiagnosticAnalyzer.cs Rename file Make static Move tests Refactor Support ref field assignment in object initializers (dotnet#62584) Fix cref tags on generated VB's SyntaxFactory (dotnet#62578) Simplify Simplify Simplify Simplify Simplify Simplify Simplify Simplify Move helper methods Remove serialization ...

jcouv self-assigned this Jul 12, 2022

ghost added the Area-Compilers label Jul 12, 2022

jcouv force-pushed the ref-init branch from 3442215 to 90c9c5a Compare July 13, 2022 00:19

jcouv force-pushed the ref-init branch 2 times, most recently from fac37f2 to e657188 Compare July 14, 2022 22:57

jcouv commented Jul 14, 2022

View reviewed changes

jcouv force-pushed the ref-init branch from e657188 to 5ade733 Compare July 14, 2022 23:09

jcouv commented Jul 14, 2022

View reviewed changes

jcouv force-pushed the ref-init branch from 5ade733 to c9274db Compare July 15, 2022 01:40

Allow ref initialization

2c7d809

jcouv force-pushed the ref-init branch from c9274db to 2c7d809 Compare July 15, 2022 17:32

jcouv marked this pull request as ready for review July 15, 2022 17:56

jcouv requested a review from a team as a code owner July 15, 2022 17:56

jcouv requested review from RikkiGibson and cston July 15, 2022 17:57

fix a test

6980e20

RikkiGibson self-assigned this Jul 18, 2022

cston reviewed Jul 18, 2022

View reviewed changes

jcouv added 3 commits July 19, 2022 09:59

Address feedback

64cb0af

Localize

74ad78e

tweak

73150a0

cston approved these changes Jul 19, 2022

View reviewed changes

jcouv requested a review from cston July 19, 2022 17:27

cston approved these changes Jul 19, 2022

View reviewed changes

RikkiGibson approved these changes Jul 19, 2022

View reviewed changes

333fred reviewed Jul 19, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs Show resolved Hide resolved

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

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

Address feedback

7c5136d

333fred approved these changes Jul 20, 2022

View reviewed changes

cston reviewed Jul 20, 2022

View reviewed changes

Test execution

53fc72c

jcouv enabled auto-merge (squash) July 21, 2022 03:42

cston approved these changes Jul 21, 2022

View reviewed changes

Julien Couvreur added 2 commits July 20, 2022 21:38

Merge remote-tracking branch 'dotnet/main' into ref-init

984528f

Merge conflict

bf99c5d

jcouv merged commit e7c8471 into dotnet:main Jul 21, 2022

ghost added this to the Next milestone Jul 21, 2022

jcouv deleted the ref-init branch July 21, 2022 06:34

runfoapp bot mentioned this pull request Jul 21, 2022

Roslyn.VisualStudio.NewIntegrationTests.CSharp.CSharpSourceGenerators.InvokeNavigateToForGeneratedFile is Flaky #60477

Closed

allisonchou modified the milestones: Next, 17.4 P1 Jul 26, 2022

ryzngard mentioned this pull request Jul 27, 2022

Enable new inline rename by default #62992

Merged

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jcouv Jul 14, 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 14, 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 commented Jul 18, 2022

Uh oh!

cston Jul 18, 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 19, 2022

Choose a reason for hiding this comment

Uh oh!

cston Jul 18, 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 Jul 18, 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 Jul 18, 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 19, 2022

Choose a reason for hiding this comment

Uh oh!

cston Jul 18, 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 Jul 18, 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 19, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

jcouv commented Jul 12, 2022 •

edited

Loading

Youssef1313 commented Jul 13, 2022 •

edited by jcouv

Loading

jcouv Jul 14, 2022 •

edited

Loading

jcouv Jul 14, 2022 •

edited

Loading

cston Jul 18, 2022 •

edited

Loading

cston Jul 18, 2022 •

edited

Loading

cston Jul 18, 2022 •

edited

Loading

cston Jul 18, 2022 •

edited

Loading

cston Jul 18, 2022 •

edited

Loading

cston Jul 18, 2022 •

edited

Loading

cston commented Jul 19, 2022 •

edited by jcouv

Loading

jcouv commented Jul 19, 2022 •

edited

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

RikkiGibson Jul 19, 2022 •

edited by jcouv

Loading

cston Jul 20, 2022 •

edited by jcouv

Loading