Allow `with` on anonymous types #53248

jcouv · 2021-05-07T03:32:01Z

Test plan: #51199

Also includes:

disallow with on pointer types
don't produce an invalid operation in with on structures

Youssef1313 · 2021-05-07T04:40:05Z

Should target main instead per #53125 (comment)?

We won't be using features/compiler anymore in the foreseeable future.

In reply to: 834059406

jcouv · 2021-05-07T04:41:27Z

Indeed. This can go in main now.

In reply to: 834059943

CyrusNajmabadi · 2021-05-07T17:40:04Z

Can you check if the 'with' keyword recommender and intellisense for properties both work here? thanks!

In reply to: 834646583

Youssef1313 · 2021-05-10T14:36:23Z

src/Features/Core/Portable/Completion/Providers/AbstractObjectInitializerCompletionProvider.cs

+                || symbol.ContainingType.IsAnonymousType
                || CanSupportObjectInitializer(symbol);


Not sure if the new check should be moved to CanSupportObjectInitializer?

i'm fine with either approach.

AlekseyTs · 2021-05-11T14:39:30Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+    /// - Regions group blocks together and represent the lifetime of locals and captures, loosely similar to scopes in C#.
+    ///   There are different kinds of regions, <see cref="ControlFlowRegionKind"/>.
+    /// - <see cref="ControlFlowGraphBuilder.SpillEvalStack"/> converts values on the stack into captures. It should be
+    ///   called before entering a new region (to avoid doing it later thus giving those captures an incorrect lifetime).


I do not think this is 100% accurate. I suggest removing this statement. What is the "correct" lifetime region depends on particular situation. #Closed

AlekseyTs · 2021-05-11T14:40:32Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

@@ -5624,7 +5633,12 @@ public override IOperation VisitFlowCapture(IFlowCaptureOperation operation, int

        public override IOperation VisitFlowCaptureReference(IFlowCaptureReferenceOperation operation, int? captureIdForResult)
        {
-            throw ExceptionUtilities.Unreachable;
+            Debug.Assert(operation.Parent is SimpleAssignmentOperation assignment


Debug.Assert(operation.Parent is SimpleAssignmentOperation assignment

Is this code still reachable? #Closed

Good catch. Restored exception.

AlekseyTs · 2021-05-11T14:45:00Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+            IOperation cloned;
+            if (operation.Type.IsValueType)
+            {
+                cloned = visitedInstance;


cloned = visitedInstance

Is this also implementing supprt for with on structures? It doesn't look like PR's description mentions that. #Closed

Yes, I noticed that for structures we were producing an invalid operation because no clone method is available. I added a test and fixed it here, but forgot to add to description.

Updated description.

Also includes fix for with on pointer types.

I'm thinking this fix may be incorrect though :-/ Do we need to capture it instead? I'll add some tests with control flow and invocations on value types....

Never mind. I recall that HandleObjectOrCollectionInitializer handles that.
I'll add the tests though

AlekseyTs · 2021-05-11T15:03:22Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+
+            static bool setAllProperties(ImmutableArray<IOperation> initializers, ImmutableArray<IPropertySymbol> properties)
+            {
+                var set = PooledHashSet<string>.GetInstance();


string

Why use name as the key instead of the property symbol? #Closed

Is it possible to have duplicate names in some error situations?

Name seems simpler.
Yes, it's possible to have duplicate names in error scenarios and this code handles that (we ignore the return value from Add and Remove)

Name seems simpler.
Yes, it's possible to have duplicate names in error scenarios and this code handles that (we ignore the return value from Add and Remove)

Does that mean that we will ignore the fact that the duplicate property isn't initialized? And perhaps not include its copy into the AnonymousObjectCreationOperation, thus failing to match the shape of the constructor?

AlekseyTs · 2021-05-11T15:10:36Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+                // calls to Visit may enter regions, so we reset things
+                LeaveRegionsUpTo(innerCaptureRegion);
+
+                var explicitProperties = PooledDictionary<string, IOperation>.GetInstance();


string

Using names as keys feels fragile, I think we should use property symbols for that. #Closed

AlekseyTs · 2021-05-12T14:14:43Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+                        simpleAssignment.Target is not InvalidOperation)
+                    {
+                        var propertyReference = (IPropertyReferenceOperation)simpleAssignment.Target;
+                        _ = set.Remove(propertyReference.Property);


set.Remove(propertyReference.Property);

It looks like we are making an assumption that a property that we get here belongs to the Anonymous Type that is cloned (otherwise we might be allocating captures that won't be used at the end). I think it is a valid assumption to make, but it might be worth adding an assert about that several lines above, where we asert other aspects of each property reference. Based on that assumption, consider if it is worth reducing the amount of HashSet operations (additions/removals) that we perform. For example, instead of first adding all properties into the set and then removing those that we run into, we could start with an empty set, add properties that we run into and at the end check if the count of items in the set equals to the count of properties in the type. #Closed

AlekseyTs · 2021-05-12T14:20:51Z

    public void WithExprOnStruct_ControlFlow_NestedInitializer()

I think we would actually want to see what happens with the Control Flow Graph for this error scenario. My understanding is that the purpose of the test is to make sure the builder doesn't crash and the graph looks reasonable.

In reply to: 839812576

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs:6596 in f930d4b. [](commit_id = f930d4b, deletion_comment = False)

AlekseyTs · 2021-05-12T14:29:16Z

    var b = a with { Error = Identity(20) };

Consider testing a scenario when the target of an assignment is a name of a function that actually exists in the type. "ToString" for example.

In reply to: 839819415

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs:8998 in f930d4b. [](commit_id = f930d4b, deletion_comment = False)

AlekseyTs · 2021-05-12T14:29:40Z

    public void WithExpr_AnonymousType_NestedInitializer()

I think we would actually want to see what happens with the Control Flow Graph for this error scenario.

In reply to: 839819719

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs:9103 in f930d4b. [](commit_id = f930d4b, deletion_comment = False)

AlekseyTs · 2021-05-12T14:31:11Z

    public void WithExpr_AnonymousType_IndexerAccess()

I think we would actually want to see what happens with the Control Flow Graph for this error scenario.

In reply to: 839820875

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs:9266 in f930d4b. [](commit_id = f930d4b, deletion_comment = False)

AlekseyTs · 2021-05-12T14:42:25Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+                }
+                LeaveRegionsUpTo(outerCaptureRegion);
+
+                return new AnonymousObjectCreationOperation(initializerBuilder.ToImmutableAndFree(), semanticModel: null, operation.Syntax, operation.Type, operation.IsImplicit);


initializerBuilder

Consider adding an assert that the amount of initializers matches the amount of properties. #Pending

AlekseyTs · 2021-05-12T14:45:32Z

src/Compilers/Test/Core/Compilation/ControlFlowGraphVerifier.cs

+                                && withExpr.Initializer.Expressions.Any()
+                                && withExpr.Expression == (object)syntax)
+                            {
+                                return true;


return true;

This is for scenarios like WithExprOnStruct_ControlFlow_WithCoalescingExpressionForValue so that the verifier won't complain about the long-lived capture with "Operation [{operationIndex}] in [{getBlockId(block)}] uses capture [{id.Value}] from another region. Should the regions be merged?" assertion above.

This would be more informative if the memo actually included the actual ids so that we could look at the graph and confirm that we indeed want to suppress the assertion. #Closed

AlekseyTs · 2021-05-12T14:57:48Z

                    IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: B, IsImplicit) (Syntax: 'Identity(this)')

Is this the capture reference that ControlFlowGraphVerifier was complaining about?

In reply to: 839841592

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs:6953 in f930d4b. [](commit_id = f930d4b, deletion_comment = False)

AlekseyTs · 2021-05-12T15:04:03Z

Done with review pass (commit 9)

jcouv · 2021-05-12T21:33:41Z

                    IFlowCaptureReferenceOperation: 0 (OperationKind.FlowCaptureReference, Type: B, IsImplicit) (Syntax: 'Identity(this)')

Yes (capture 0 used in B5)

In reply to: 839841592

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/RecordStructTests.cs:6953 in f930d4b. [](commit_id = f930d4b, deletion_comment = False)

AlekseyTs

LGTM (commit 10)

jcouv · 2021-05-12T23:06:11Z

@dotnet/roslyn-compiler for a second review. Thanks

RikkiGibson · 2021-05-18T21:57:56Z

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

+            static bool isAllowedDespiteReadonly(BoundExpression receiver)
+            {
+                // ok: anonymousType with { Property = ... }
+                if (receiver is BoundObjectOrCollectionValuePlaceholder && receiver.Type.IsAnonymousType)


nit: it feels like the && can be collapsed into a single recursive pattern and this can be inlined at the call site :)

RikkiGibson · 2021-05-18T22:13:22Z

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

+            ImmutableArray<BoundExpression> getAnonymousTypeValues(BoundWithExpression withExpr, BoundExpression oldValue, AnonymousTypeManager.AnonymousTypePublicSymbol anonymousType,
+                ArrayBuilder<BoundExpression> sideEffects, ArrayBuilder<LocalSymbol> temps)
+            {
+                var dictionary = PooledDictionary<PropertySymbol, BoundExpression>.GetInstance();


nit: it feels like in this scenario we could use an ArrayBuilder which maps the AnonymousTypePropertySymbol.MemberIndexOpt.Value to a BoundLocal?. Also could consider exposing a member on AnonymousTypePropertySymbol like int MemberIndex => _index. #Resolved

Made the change. Thanks

RikkiGibson · 2021-05-18T22:58:01Z

src/Compilers/Test/Utilities/CSharp/CSharpTestBase.cs

@@ -1881,6 +1881,7 @@ protected static void VerifyFlowGraphForTest<TSyntaxNode>(CSharpCompilation comp
        {
            var tree = compilation.SyntaxTrees[0];
            SyntaxNode syntaxNode = GetSyntaxNodeOfTypeForBinding<TSyntaxNode>(GetSyntaxNodeList(tree));
+            Debug.Assert(syntaxNode is not null, "Did you forget to place /*<bind>*/ comments in your source?");


Thank you for this. #Closed

RikkiGibson · 2021-05-18T23:49:32Z

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

+                    Debug.Assert(left.MemberSymbol is not null);
+
+                    // We evaluate the values provided in source first
+                    BoundLocal valueTemp = _factory.StoreToTemp(assignment.Right, out BoundAssignmentOperator boundAssignmentToTemp);


nit: it feels like it's possible to avoid storing to temps when the assignments occur in the same order as the constructor parameters, similar to the way method call arguments are rewritten during lowering. However, I don't think it's necessary to implement this optimization in this PR. #WontFix

Agreed. Also, the cases where this optimization could actually play out aren't the common case so it seems fine to punt.

RikkiGibson · 2021-05-18T23:51:06Z

src/Compilers/Core/Portable/Operations/ControlFlowGraphBuilder.cs

+    ///   There are different kinds of regions, <see cref="ControlFlowRegionKind"/>.
+    /// - <see cref="ControlFlowGraphBuilder.SpillEvalStack"/> converts values on the stack into captures.
+    /// - Error scenarios from initial binding need to be handled.
+    /// </summary>


Thanks for this. It helped me review this PR and I'm sure it will help future readers. #Closed

CyrusNajmabadi

ide side lgmt.

jcouv · 2021-05-20T23:33:38Z

/azp run

azure-pipelines · 2021-05-20T23:34:01Z

Azure Pipelines successfully started running 3 pipeline(s).

…ures/interpolated-string * upstream/main: (92 commits) Keep casts when necessary to prefer a constant pattern over a type pattern Remove SyntaxKind.DataKeyword (#53614) Display 'readonly' for record structs (#53634) Update Building, Debugging, and Testing on Windows.md (#53543) Update dependencies from https://github.com/dotnet/arcade build 20210521.3 (#53617) Introduce resx for BuildValidator and MS.CA.Rebuild to allow localization (#53447) Report obsoletion diagnostics for slice and indexer (#53463) Update BasicGenerateConstructorDialog.cs Add searchbox in generate overrides dialog Allow `with` on anonymous types (#53248) Report diagnostic on correct node (#53538) Fix NotNullIfNotNull delegate conversion (#53409) Verify quick info session in InvokeQuickInfo Remove unnecessary retry Ensure no navbar IO on the UI thread Enable nullable reference types Fix timeout behavior in GetQuickInfo Add a semantic model based GetQuickInfoAsync entry point into QuickInfoServiceWithProviders Move semantic model based quick info API up to CommonQuickInfoProvider type Fix dnceng build by forcing the use of xcopy msbuild ...

jcouv added Area-Compilers New Language Feature - Records Records labels May 7, 2021

jcouv added this to the C# 10 milestone May 7, 2021

jcouv force-pushed the with-anonymous branch from d8e2fb0 to 72a069a Compare May 7, 2021 03:43

jcouv mentioned this pull request May 7, 2021

Test plan for "record structs" #51199

Closed

92 tasks

jcouv changed the base branch from features/compiler to main May 7, 2021 04:41

Allow with on anonymous types

1365a81

jcouv force-pushed the with-anonymous branch from 72a069a to 1365a81 Compare May 7, 2021 17:13

jcouv added 3 commits May 7, 2021 12:41

Fix scenario with invalid assignment

302fbcc

Enable IOperation flag temporarily

ff57fe8

IDE: fix property completion in 'with'

dc0c581

Youssef1313 reviewed May 10, 2021

View reviewed changes

Avoid capture if unused

85b60cf

jcouv marked this pull request as ready for review May 11, 2021 04:56

jcouv requested review from a team as code owners May 11, 2021 04:56

jcouv requested a review from CyrusNajmabadi May 11, 2021 04:56

jcouv added the Area-IDE label May 11, 2021

jcouv self-assigned this May 11, 2021

jcouv requested review from AlekseyTs and RikkiGibson May 11, 2021 05:57

AlekseyTs reviewed May 11, 2021

View reviewed changes

Address feedback

ff73199

AlekseyTs reviewed May 11, 2021

View reviewed changes

AlekseyTs reviewed May 12, 2021

View reviewed changes

runfoapp bot mentioned this pull request May 12, 2021

SimpleCovariantReturnEndToEndTest is stack overflowing in CI #53361

Closed

Address feedback

0e97cf9

AlekseyTs approved these changes May 12, 2021

View reviewed changes

runfoapp bot mentioned this pull request May 13, 2021

Attempt to load System.Runtime reference assembly for execution causes test failure #53391

Open

RikkiGibson self-assigned this May 17, 2021

RikkiGibson approved these changes May 18, 2021

View reviewed changes

Use ArrayBuilder

2bad303

CyrusNajmabadi approved these changes May 19, 2021

View reviewed changes

jcouv added 2 commits May 19, 2021 12:31

Remove cast

9f3cdac

Formatting

3a6e4e0

jcouv merged commit 828c0a0 into dotnet:main May 21, 2021

ghost modified the milestones: C# 10, Next May 21, 2021

jcouv deleted the with-anonymous branch May 21, 2021 03:48

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

		\|\| symbol.ContainingType.IsAnonymousType
		\|\| CanSupportObjectInitializer(symbol);

Allow with on anonymous types #53248

Allow with on anonymous types #53248

Conversation

jcouv commented May 7, 2021 • edited Loading

Youssef1313 commented May 7, 2021 • edited by jcouv Loading

jcouv commented May 7, 2021 • edited Loading

CyrusNajmabadi commented May 7, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2021 • edited Loading

Choose a reason for hiding this comment

jcouv May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs May 11, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs May 12, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 12, 2021 • edited Loading

AlekseyTs commented May 12, 2021 • edited by jcouv Loading

AlekseyTs commented May 12, 2021 • edited Loading

AlekseyTs commented May 12, 2021 • edited Loading

AlekseyTs May 12, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

AlekseyTs May 12, 2021 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented May 12, 2021 • edited Loading

AlekseyTs commented May 12, 2021

jcouv commented May 12, 2021

AlekseyTs left a comment

Choose a reason for hiding this comment

jcouv commented May 12, 2021

Choose a reason for hiding this comment

RikkiGibson May 18, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson May 18, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

RikkiGibson May 18, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson May 18, 2021 • edited by jcouv Loading

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

jcouv commented May 20, 2021

azure-pipelines bot commented May 20, 2021

Allow `with` on anonymous types #53248

Allow `with` on anonymous types #53248

jcouv commented May 7, 2021 •

edited

Loading

Youssef1313 commented May 7, 2021 •

edited by jcouv

Loading

jcouv commented May 7, 2021 •

edited

Loading

CyrusNajmabadi commented May 7, 2021 •

edited by jcouv

Loading

AlekseyTs May 11, 2021 •

edited

Loading

AlekseyTs May 11, 2021 •

edited

Loading

AlekseyTs May 11, 2021 •

edited

Loading

jcouv May 11, 2021 •

edited

Loading

AlekseyTs May 11, 2021 •

edited

Loading

AlekseyTs May 11, 2021 •

edited

Loading

AlekseyTs May 12, 2021 •

edited

Loading

AlekseyTs commented May 12, 2021 •

edited

Loading

AlekseyTs commented May 12, 2021 •

edited by jcouv

Loading

AlekseyTs commented May 12, 2021 •

edited

Loading

AlekseyTs commented May 12, 2021 •

edited

Loading

AlekseyTs May 12, 2021 •

edited by jcouv

Loading

AlekseyTs May 12, 2021 •

edited

Loading

AlekseyTs commented May 12, 2021 •

edited

Loading

RikkiGibson May 18, 2021 •

edited by jcouv

Loading

RikkiGibson May 18, 2021 •

edited by jcouv

Loading

RikkiGibson May 18, 2021 •

edited by jcouv

Loading

RikkiGibson May 18, 2021 •

edited by jcouv

Loading