Brace completion and IOperation for `with` expression by jcouv · Pull Request #44712 · dotnet/roslyn

jcouv · 2020-05-30T00:46:13Z

Relates to #40726 (test plan)

CyrusNajmabadi

IDE side lgtm.

jcouv · 2020-06-03T01:37:40Z

@dotnet/roslyn-compiler for review.
@CyrusNajmabadi Could you take another look? I added more tests and fixed a couple more small issues.

CyrusNajmabadi · 2020-06-03T02:47:13Z

src/EditorFeatures/CSharpTest/Formatting/Indentation/SmartTokenFormatterFormatTokenTests.cs

 {
    public class SmartTokenFormatterFormatTokenTests : CSharpFormatterTestsBase
    {
+        public SmartTokenFormatterFormatTokenTests(ITestOutputHelper output) : base(output) { }


nit, we don't place {} on the same line ever. #Resolved

CyrusNajmabadi · 2020-06-03T02:47:30Z

src/EditorFeatures/TestUtilities/Formatting/CoreFormatterTestsBase.cs

+        public CoreFormatterTestsBase(ITestOutputHelper output)
+        {
+            this._output = output;
+        }


Suggested change

public CoreFormatterTestsBase(ITestOutputHelper output)

{

this._output = output;

}

public CoreFormatterTestsBase(ITestOutputHelper output)

=> _output = output;

CyrusNajmabadi · 2020-06-03T02:47:55Z

src/EditorFeatures/TestUtilities/Formatting/CoreFormatterTestsBase.cs

-            Assert.Equal(expected, actual);
+            if (actual != expected)
+            {
+                _output.WriteLine(actual);


what't he value of this? #Resolved

This gives easy access to the actual result in Test Explorer, in addition to the usual diff output with pluses and minuses. #Resolved

CyrusNajmabadi · 2020-06-03T02:49:14Z

src/Workspaces/CSharp/Portable/Simplification/CSharpSimplificationService.Expander.cs

                while (parent != null)
                {
-                    if (parent.Kind() == SyntaxKind.ObjectInitializerExpression)
+                    if (parent.IsKind(SyntaxKind.ObjectInitializerExpression, SyntaxKind.WithInitializerExpression))


i'm fine with this. however, actual question about with. does it allow for nested initializers like object initializers do? #Resolved

with is limited to one level, simple assignments only.

{ Property = 1 } is okay

{ [0] = 1 } is illegal

{ 1, 2, 3 } is illegal

{ Property = { ... } } is illegal #Resolved

RikkiGibson · 2020-06-03T16:19:13Z

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

            return new UsingDeclarationOperation(Visit(operation.DeclarationGroup), operation.IsAsynchronous, ((Operation)operation).OwningSemanticModel, operation.Syntax, operation.Type, operation.ConstantValue, operation.IsImplicit);
        }
+
+        // TODO2


If work still needs to be done here, could you please create a follow-up issue and reference it instead of including a TODO in source? #Resolved

Oops. I'll remove the comment and file an issue.
I think OperationCloner is used by CFG, we'll probably need to do some work there for records. #Resolved

Added a note to test plan #Resolved

AlekseyTs · 2020-06-03T17:58:34Z

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

      </Comments>
    </Property>
  </Node>
+  <Node Name="IWithExpressionOperation" Base="IOperation" ChildrenOrder="Value,Initializer">


IWithExpressionOperation [](start = 14, length = 24)

I think we would want a special rewrite for this node in CFG, something very similar to a an object initializer operation #Closed

AlekseyTs · 2020-06-03T18:00:16Z

src/Test/Utilities/Portable/Compilation/OperationTreeVerifier.cs

            VisitArray(operation.DimensionSizes, "DimensionSizes", logElementCount: true);
        }

+        public override void VisitWithExpression(IWithExpressionOperation operation)


VisitWithExpression [](start = 29, length = 19)

We have another test visitor that should be adjusted as well #Closed

RikkiGibson · 2020-06-03T18:03:07Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/SyntaxNodeExtensions.cs

                    return (switchExpression.OpenBraceToken, switchExpression.CloseBraceToken);
                case PropertyPatternClauseSyntax property:
                    return (property.OpenBraceToken, property.CloseBraceToken);
+#if !CODE_STYLE


do we have any way of tracking when this #if should be removed? #Resolved

There is a general issue for removing all CODE_STYLE conditions:
#41462 #Resolved

AlekseyTs · 2020-06-03T18:03:13Z

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

      </Comments>
    </Property>
  </Node>
+  <Node Name="IWithExpressionOperation" Base="IOperation" ChildrenOrder="Value,Initializer">


IWithExpressionOperation [](start = 14, length = 24)

Please run all tests with -testIOperation switch locally and confirm that none of the failures are related to the feature. #Closed

AlekseyTs · 2020-06-04T18:39:26Z

src/Compilers/Core/Portable/Operations/OperationInterfaces.xml

      </Comments>
    </Property>
  </Node>
+  <Node Name="IWithExpressionOperation" Base="IOperation" ChildrenOrder="Value,Initializer">


IWithExpressionOperation [](start = 14, length = 24)

I think we need to adjust OperationCloner for this node #Closed

AlekseyTs · 2020-06-04T18:45:55Z

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

+            IOperation visitedInstance = Visit(operation.Value);
+
+            IOperation cloned = operation.CloneMethod is null
+                ? visitedInstance


visitedInstance [](start = 18, length = 15)

Is this for an error case? If that is the case, I think we should use MakeInvalidOperation helper to create an invalid operation on top of the instance. #Closed

AlekseyTs · 2020-06-04T18:47:15Z

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

+                ? visitedInstance
+                : new InvocationOperation(operation.CloneMethod, visitedInstance,
+                    isVirtual: false, arguments: ImmutableArray<IArgumentOperation>.Empty,
+                    semanticModel: null, operation.Syntax, operation.Type, operation.ConstantValue, IsImplicit(operation));


IsImplicit(operation) [](start = 100, length = 21)

I think the call should always be implicit. #Closed

AlekseyTs · 2020-06-04T18:53:18Z

src/Compilers/CSharp/Portable/Operations/CSharpOperationNodes.cs

+        private readonly CSharpOperationFactory _operationFactory;
+        private readonly BoundWithExpression _withExpression;
+
+        internal CSharpLazyWithExpressionOperation(CSharpOperationFactory operationFactory, BoundWithExpression withExpression, IMethodSymbol constructor, SemanticModel semanticModel, SyntaxNode syntax, ITypeSymbol type, Optional<object> constantValue, bool isImplicit) :


constructor [](start = 142, length = 11)

Is this the clone method? The name is confusing. #Closed

AlekseyTs · 2020-06-04T18:55:29Z

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

+
+            IOperation cloned = operation.CloneMethod is null
+                ? visitedInstance
+                : new InvocationOperation(operation.CloneMethod, visitedInstance,


visitedInstance [](start = 65, length = 15)

Is Clone method an instance method? #Closed

Yes, it is an instance method #Resolved

AlekseyTs · 2020-06-04T18:57:37Z

It doesn't look like any tests were added on the compiler side for IOperation or CFG #Closed

AlekseyTs · 2020-06-04T18:57:53Z

Done with review pass (iteration 9) #Closed

jcouv · 2020-06-04T19:38:45Z

It doesn't look like any tests were added on the compiler side for IOperation or CFG

I don't know what you mean. Look for calls to VerifyFlowGraph and VerifyOperationTree in RecordsTest #Closed

AlekseyTs · 2020-06-04T22:18:39Z

I don't know what you mean. Look for calls to VerifyFlowGraph and VerifyOperationTree in RecordsTest

I accidentally collapsed that folder. Sorry.

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

AlekseyTs · 2020-06-04T22:25:36Z

Consider adding a note to Test Plan to add tests for scenarios involving control flow inside 'with' expression #Closed

AlekseyTs · 2020-06-04T22:26:36Z

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

+            Left:
+              IPropertyReferenceOperation: System.Int32 C.X { get; init; } (OperationKind.PropertyReference, Type: System.Int32) (Syntax: 'X')
+                Instance Receiver:
+                  IInstanceReferenceOperation (ReferenceKind: ImplicitReceiver) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'X')


IInstanceReferenceOperation (ReferenceKind: ImplicitReceiver) (OperationKind.InstanceReference, Type: C, IsImplicit) (Syntax: 'X') [](start = 18, length = 130)

Consider adjusting the comment for the node and the ReferenceKind to include information about this usage #Closed

@AlekseyTs I didn't understand this comment. #Resolved

I didn't understand this comment.

IInstanceReferenceOperation and OperationKind.InstanceReference are now used in a new scenario. Consider reflecting this in the doc comment for them

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

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

jcouv · 2020-06-05T19:24:01Z

Added " test control flow within with expression initializer"

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

AlekseyTs · 2020-06-05T20:14:26Z

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

    ///  (1) C# this or base expression.
    ///  (2) VB Me, MyClass, or MyBase expression.
-    ///  (3) C# object or collection initializers.
+    ///  (3) C# object or collection or 'with' expression initializers.


or 'with' expression [](start = 37, length = 20)

This is a generated file, should probably modify XML instead #Resolved

AlekseyTs

LGTM (iteration 12), modulo direct modification of a comment in a generated file

RikkiGibson · 2020-06-05T22:08:56Z

@jcouv FYI it looks like the InlineIntoWithExpression test is broken.

AlekseyTs · 2020-06-06T03:07:27Z

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

+            IOperation cloned = operation.CloneMethod is null
+                ? MakeInvalidOperation(visitedInstance.Type, visitedInstance)
+                : new InvocationOperation(operation.CloneMethod, visitedInstance,
+                    isVirtual: false, arguments: ImmutableArray<IArgumentOperation>.Empty,


false [](start = 31, length = 5)

Based on #44852, the call probably should be virtual

Thanks. Will fix in a follow-up PR

@AlekseyTs It looks like Andy fixed this already (we now have isVirtual: true in master.

333fred · 2020-06-08T15:03:10Z

@jcouv did we discuss using something like ICloneOperation for the name of this node, or adding a flag to the existing IWithOperation to indicate whether the operation cloned the node? We don't use "Expression" in the names of IOperation nodes, with the exception of IExpressionStatementOperation.

jcouv · 2020-06-08T16:43:19Z

@333fred I discussed re-using IWithOperation with Aleksey, but we concluded it was not a good fit.
In VB, that operation is a statement (doesn't return a value) and it contains statements (Body).
For C#, we would want a value for the expression, and we have an initializer (IObjectOrCollectionInitializerOperation) rather than a body.

I'll start a thread with broader IOperation v-team to confirm shape and names. Thanks

CyrusNajmabadi approved these changes May 30, 2020

View reviewed changes

jcouv force-pushed the with-ide branch from 65a9538 to bfb6c1a Compare June 1, 2020 20:05

agocke and others added 6 commits June 2, 2020 11:36

Fix brace completion for the with expression

52b4c8a

Tweak for new syntax

26c5612

Implement IOperation for 'with' expression

154b634

Test formatting 'with' expressions

c833047

Public API

57048ed

Fix issue with InlineTemp

96f7341

jcouv force-pushed the with-ide branch from 25dccb3 to 96f7341 Compare June 3, 2020 01:36

jcouv marked this pull request as ready for review June 3, 2020 01:36

jcouv requested review from a team as code owners June 3, 2020 01:36

jcouv self-assigned this Jun 3, 2020

jcouv added Area-Compilers Area-IDE Feature - Records Records labels Jun 3, 2020

CyrusNajmabadi reviewed Jun 3, 2020

View reviewed changes

CyrusNajmabadi approved these changes Jun 3, 2020

View reviewed changes

Address feedback from Cyrus

dceba97

RikkiGibson reviewed Jun 3, 2020

View reviewed changes

Remove TODO comment

751af04

AlekseyTs reviewed Jun 3, 2020

View reviewed changes

RikkiGibson reviewed Jun 3, 2020

View reviewed changes

AlekseyTs reviewed Jun 3, 2020

View reviewed changes

Implement CFG for with expression

b9cc321

AlekseyTs reviewed Jun 4, 2020

View reviewed changes

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

Merge remote-tracking branch 'dotnet/features/records' into with-ide

5580493

Julien Couvreur added 2 commits June 5, 2020 13:05

Address feedback from Aleksey

9d3ee98

Fix and Verify Clone

b612da6

AlekseyTs reviewed Jun 5, 2020

View reviewed changes

AlekseyTs approved these changes Jun 5, 2020

View reviewed changes

Fix comment in xml file instead

60406da

RikkiGibson self-requested a review June 5, 2020 20:36

RikkiGibson approved these changes Jun 5, 2020

View reviewed changes

Update to use new 'record' syntax

81dc5a5

jcouv merged commit 3a5c406 into dotnet:features/records Jun 6, 2020

jcouv deleted the with-ide branch June 6, 2020 02:27

AlekseyTs reviewed Jun 6, 2020

View reviewed changes

AlekseyTs mentioned this pull request Jun 6, 2020

Change name of record Clone method to unspeakable #44852

Merged

jcouv mentioned this pull request Jun 15, 2020

Test plan for "records" #40726

Closed

Conversation

jcouv commented May 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Jun 3, 2020

Uh oh!

CyrusNajmabadi Jun 3, 2020 • 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!

CyrusNajmabadi Jun 3, 2020

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Jun 3, 2020 • 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 Jun 3, 2020 • 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!

CyrusNajmabadi Jun 3, 2020 • 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 Jun 3, 2020 • 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!

RikkiGibson Jun 3, 2020 • 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 Jun 3, 2020 • 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 Jun 3, 2020 • 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 Jun 3, 2020 • 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 Jun 3, 2020 • 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!

RikkiGibson Jun 3, 2020 • 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 Jun 4, 2020 • 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 Jun 3, 2020 • 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 Jun 4, 2020 • 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 Jun 4, 2020 • 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 Jun 4, 2020 • 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 Jun 4, 2020 • 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 May 30, 2020 •

edited

Loading

CyrusNajmabadi Jun 3, 2020 •

edited by jcouv

Loading

CyrusNajmabadi Jun 3, 2020 •

edited by jcouv

Loading

jcouv Jun 3, 2020 •

edited

Loading

CyrusNajmabadi Jun 3, 2020 •

edited by jcouv

Loading

jcouv Jun 3, 2020 •

edited

Loading

RikkiGibson Jun 3, 2020 •

edited by jcouv

Loading

jcouv Jun 3, 2020 •

edited

Loading

jcouv Jun 3, 2020 •

edited

Loading

AlekseyTs Jun 3, 2020 •

edited

Loading

AlekseyTs Jun 3, 2020 •

edited

Loading

RikkiGibson Jun 3, 2020 •

edited by jcouv

Loading

jcouv Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 3, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

jcouv Jun 5, 2020 •

edited

Loading

AlekseyTs commented Jun 4, 2020 •

edited

Loading

AlekseyTs commented Jun 4, 2020 •

edited

Loading

jcouv commented Jun 4, 2020 •

edited by AlekseyTs

Loading

AlekseyTs commented Jun 4, 2020 •

edited

Loading

AlekseyTs Jun 4, 2020 •

edited

Loading

jcouv Jun 5, 2020 •

edited

Loading

AlekseyTs Jun 5, 2020 •

edited by jcouv

Loading