Allow 'with' expression on value types by jcouv · Pull Request #52319 · dotnet/roslyn

jcouv · 2021-03-31T22:47:50Z

Test plan #51199

…th-structs

AlekseyTs · 2021-04-06T15:40:10Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -6275,7 +6275,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
    <value>The receiver of a `with` expression must have a non-void type.</value>
  </data>
  <data name="ERR_NoSingleCloneMethod" xml:space="preserve">


ERR_NoSingleCloneMethod [](start = 14, length = 23)

Consider renaming the error to more accurately reflect its purpose/conditions under which it is reported #Closed

AlekseyTs · 2021-04-06T15:45:16Z

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

+                case BoundKind.WithExpression:
+                    {
+                        var withExpr = (BoundWithExpression)expr;
+                        var escape = CheckValEscape(node, withExpr.Receiver, escapeFrom, escapeTo, checkingReceiver: false, diagnostics);


CheckValEscape(node, withExpr.Receiver, escapeFrom, escapeTo, checkingReceiver: false, diagnostics); [](start = 37, length = 100)

Were we missing this check for 'with' on records? #Closed

From what I understand, val escape analyses are initiated when we do an assignment (we call GetValEscape and ValidateEscape from BindAssignment, BindDeconstructionAssignment, BindReturn, etc). But it's only relevant if we're assigning a ref-like type (GetValEscape returns ExternalScope for all other types).
So I think record structs introduce the need to analyze with expressions, because the type of the receiver and the with expression now can be a ref-like struct.

For the initializer part, I'm not sure. I've added a test WithExpr_ValEscape which would have thought should produce diagnostics because the initializer contains dangerous assignments. Could you take a look?
If I'm correct, this is an existing bug (an assignment for which we are not triggering val-escape checks).

In reply to: 607968314 [](ancestors = 607968314)

Chatted with Jared. The scenario in WithExpr_ValEscape with assigning a ref struct value to a property is fine. The setter is essentially void Set(S1), which cannot do anything dangerous (it could not store S1 and it returns void so may not sneak it out).

In reply to: 608258240 [](ancestors = 608258240,607968314)

AlekseyTs · 2021-04-06T15:51:15Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectCreationExpression.cs

            //      receiver with { P1 = e1, P2 = e2 }
            //
-            // we want to lower it to a call to the receiver's `Clone` method, then
+            // if the receiver is a struct, then set the given struct properties:


given [](start = 57, length = 5)

This sounds somewhat confusing. We are not mutating the given struct, but rather returning a new value. #Closed

The sentence was incomplete. Fixed

In reply to: 607973334 [](ancestors = 607973334)

AlekseyTs · 2021-04-06T16:01:43Z

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

+            var src = @"
+var b = new B() { X = 1 };
+var b2 = b.M();
+System.Console.Write(b2.X);


Consider observing that the original structure is not mutated. #Closed

AlekseyTs · 2021-04-06T16:06:54Z

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

+            var comp = CreateCompilation(src);
+            comp.VerifyDiagnostics();
+            var verifier = CompileAndVerify(comp, expectedOutput: "4243");
+            verifier.VerifyIL("<Program>$.<<Main>$>g__M|0_0(System.ValueTuple<int, int>)", @"


$.<
$>g__M|0_0(System.ValueTuple<int, int>) [](start = 31, length = 57)

It feels like it would be better to have a regualr method within a type. #Closed

AlekseyTs · 2021-04-06T16:08:51Z

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

+    public readonly int X;
+    public B M()
+    {
+        return this with { X = 42 };


this with { X = 42 }; [](start = 15, length = 21)

Consider testing this error inside a constructor. #Closed

AlekseyTs

LGTM (commit 4) with some comments.

RikkiGibson · 2021-04-06T23:39:58Z

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


            MethodSymbol? cloneMethod = null;
-            if (!receiverType.IsErrorType())
+            if (receiverType.IsValueType)


It feels like for enums for example there is nothing you can assign in the right side of the with. Is there any item tracking giving a warning when the right side of the with is empty? #Resolved

I have a couple of tests with an empty initializers in with. I don't think that should be a warning, as empty initializers are also allowed in object creation.

In reply to: 608246244 [](ancestors = 608246244)

RikkiGibson · 2021-04-06T23:45:55Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_ObjectCreationExpression.cs

+            }
+            else
+            {
+                RoslynDebug.AssertNotNull(withExpr.CloneMethod);


nit: We should be using an annotated Debug.Assert at this point, so it should be possible to just say Debug.Assert(withExpr.CloneMethod is not null) #Resolved

RikkiGibson · 2021-04-06T23:47:42Z

src/Compilers/CSharp/Portable/CSharpResources.resx

-  <data name="ERR_NoSingleCloneMethod" xml:space="preserve">
-    <value>The receiver type '{0}' is not a valid record type.</value>
+  <data name="ERR_CannotClone" xml:space="preserve">
+    <value>The receiver type '{0}' is not a valid record type and is not a struct type.</value>


Consider adding a <comment> that 'record' and 'struct' are language keywords for loc team #WontFix

In general, I'm not sure about such comments. In this instance, they feel wrong. record and struct are not keywords in this sentence.
If they were keywords, then they could be marked with backticks in markdown, but that doesn't work here: "...is not a valid record type and is not a struct type."

In reply to: 608248749 [](ancestors = 608248749)

AlekseyTs · 2021-04-07T15:10:08Z

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

                    var switchExpr = (BoundSwitchExpression)expr;
                    return GetValEscape(switchExpr.SwitchArms.SelectAsArray(a => a.Value), scopeOfTheContainingExpression);

+                case BoundKind.WithExpression:


case BoundKind.WithExpression: [](start = 16, length = 30)

It might be convenient to place this case next to BoundKind.ObjectCreationExpression: since they both have to deal with an initializer. Same for the other function getting a similar case. #Closed

AlekseyTs

LGTM (commit 10)

jcouv added Area-Compilers Feature - Records Records labels Mar 31, 2021

jcouv added this to the C# 10 milestone Mar 31, 2021

jcouv self-assigned this Mar 31, 2021

jcouv mentioned this pull request Mar 31, 2021

Test plan for "record structs" #51199

Closed

92 tasks

Allow 'with' expression on value types

54a5c86

jcouv force-pushed the with-structs branch from 4f3b0e3 to 54a5c86 Compare March 31, 2021 23:44

jcouv marked this pull request as ready for review April 2, 2021 14:37

jcouv requested a review from a team as a code owner April 2, 2021 14:37

jcouv marked this pull request as draft April 2, 2021 18:07

jcouv force-pushed the with-structs branch from 9fee667 to 2bf2338 Compare April 2, 2021 18:52

Test tuples and other types, fix ref structs

14f5826

jcouv force-pushed the with-structs branch from 2bf2338 to 14f5826 Compare April 2, 2021 19:30

jcouv marked this pull request as ready for review April 2, 2021 19:31

Julien Couvreur added 2 commits April 2, 2021 14:46

Also check expression

9c533ab

Merge remote-tracking branch 'dotnet/features/record-structs' into wi…

c8d9edd

…th-structs

AlekseyTs reviewed Apr 6, 2021

View reviewed changes

AlekseyTs approved these changes Apr 6, 2021

View reviewed changes

Julien Couvreur added 2 commits April 6, 2021 16:28

Address feedback

91a6c93

tweak

a8cc8bd

RikkiGibson self-assigned this Apr 6, 2021

RikkiGibson approved these changes Apr 6, 2021

View reviewed changes

Julien Couvreur added 3 commits April 6, 2021 17:02

Also implement GetValEscape

fa84319

Another val escape test

0393610

assertion

e0c0852

jcouv requested a review from AlekseyTs April 7, 2021 00:24

typo

c1908e1

AlekseyTs reviewed Apr 7, 2021

View reviewed changes

AlekseyTs approved these changes Apr 7, 2021

View reviewed changes

reorder cases

7590222

jcouv enabled auto-merge (squash) April 7, 2021 19:19

jcouv merged commit a959cbd into dotnet:features/record-structs Apr 7, 2021

jcouv deleted the with-structs branch April 8, 2021 00:36

Conversation

jcouv commented Mar 31, 2021

Uh oh!

AlekseyTs Apr 6, 2021 • 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!

AlekseyTs Apr 6, 2021 • 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 Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

jcouv Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2021 • 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 Apr 6, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 6, 2021 • 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!

AlekseyTs Apr 6, 2021 • 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!

AlekseyTs Apr 6, 2021 • 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!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 6, 2021 • 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!

jcouv Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Apr 6, 2021 • 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 Apr 6, 2021 • 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!

jcouv Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Apr 7, 2021 • 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!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

AlekseyTs Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

AlekseyTs Apr 6, 2021 •

edited

Loading

RikkiGibson Apr 6, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 6, 2021 •

edited by jcouv

Loading

RikkiGibson Apr 6, 2021 •

edited by jcouv

Loading

AlekseyTs Apr 7, 2021 •

edited

Loading