added CSharpOperationFactory #18825

heejaechang · 2017-04-19T18:18:38Z

made BoundNode to not implement IOperation and replaced GetOperationInternal to return Operation type.

currently, it uses Lazy for laziness and ConditionalWeakTable for identity

this only contains common and csharp side change. vb side change will follow.

Part of #18246

…ionFactory

otherwise, it throws all over the places.

heejaechang · 2017-04-20T05:21:02Z

retest windows_release_vs-integration_prtest please

heejaechang · 2017-04-20T13:30:48Z

@dotnet/analyzer-ioperation can you take a look?

CyrusNajmabadi · 2017-04-20T18:54:32Z

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

@@ -272,7 +272,8 @@ internal BoundExpression ConvertPatternExpression(TypeSymbol inputType, CSharpSy
                    hasErrors = CheckRestrictedTypeInAsync(this.ContainingMemberOrLambda, declType, diagnostics, typeSyntax);
                }

-                return new BoundDeclarationPattern(node, localSymbol, boundDeclType, isVar, hasErrors);
+                var expression = localSymbol == null ? new BoundDiscardExpression(node, boundDeclType.Type) : (BoundExpression)new BoundLocal(node, localSymbol, null, boundDeclType.Type);


not understanding why it's the case that we have a discard if we don't have a local symbol.

@CyrusNajmabadi I dont know I didn't change the logic. @genlu do you know?

CyrusNajmabadi · 2017-04-20T18:55:50Z

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

+{
+    internal static partial class CSharpOperationFactory
+    {
+        private static readonly ConditionalWeakTable<BoundNode, IOperation> s_cache = new ConditionalWeakTable<BoundNode, IOperation>();


i feel like a cache on SemanticModel might be more appropriate, with Operations having a pointer back to the semantic model that was used to create them.

CWT's are ok when you really have no appropritae entity on which to associate data ownership. In this case, it feels like the SemanticModel is the right owner.

@CyrusNajmabadi my next work is adding parent pointer and doing better identity mechanism.

current thinking is making factory an instance owned by semantic model that have explicit cache tied to semantic model.

only thing I am not sure is whether the cache will be tied to bound node or not.

Ok great. Maybe today at our design meeting you can talk us through yoru next steps.

CyrusNajmabadi · 2017-04-20T19:05:34Z

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

+        {
+            switch (boundNode)
+            {
+                case BoundDeconstructValuePlaceholder boundDeconstructValuePlaceholder:


while this pattern matching is attractive because it doesn't require test+cast, it's also really expensive as every node may require dozens of type checks.

is there a visitor for bound nodes we can use to do this work?

if not, i think we should just use a regular Kind switch so that we don't do tons of type checks for every operation we create.

--

Note: this isn't a hypothetical concern. I've run into i several time in throughput oriented domains. type-switchs are great when small, or when you need to them rarely. However, if we're converting tons of nodes over to operations, this isn't desirable.

sure, I can change it to different pattern.

CyrusNajmabadi · 2017-04-20T19:06:06Z

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

+            Optional<object> constantValue = ConvertToOptional(boundDeconstructValuePlaceholder.ConstantValue);
+            return new PlaceholderExpression(isInvalid, syntax, type, constantValue);
+        }
+        private static IInvocationExpression CreateBoundCallOperation(BoundCall boundCall)


are these handwritten, or generated? If handwritten, add newline.

generated. styling. let me do once I am done vb side and done with all other code reviews.

CyrusNajmabadi · 2017-04-20T19:06:52Z

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

+        private static ICompoundAssignmentExpression CreateBoundCompoundAssignmentOperatorOperation(BoundCompoundAssignmentOperator boundCompoundAssignmentOperator)
+        {
+            BinaryOperationKind binaryOperationKind = CSharp.Expression.DeriveBinaryOperationKind(boundCompoundAssignmentOperator.Operator.Kind);
+            Lazy<IOperation> target = new Lazy<IOperation>(() => (IOperation)Create(boundCompoundAssignmentOperator.Left));


the cast to IOperation feels like it shoudn't be necessary.

yep. its there since it is generated. I will clean those up.

CyrusNajmabadi · 2017-04-20T19:07:29Z

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

+        private static IVariableDeclarationStatement CreateBoundLocalDeclarationOperation(BoundLocalDeclaration boundLocalDeclaration)
+        {
+            Lazy<ImmutableArray<IVariableDeclaration>> declarations = new Lazy<ImmutableArray<IVariableDeclaration>>(() => GetVariableDeclarationStatementVariables(boundLocalDeclaration));
+            bool isInvalid = boundLocalDe


Sync with Gen to make sure this isn't a problem with teh work he's doing.

@genlu @mavasani @333fred do you guys have pending work on IOperation? this includes the Evaluation order and IVariableDecl changes.

I'm not working on this.

CyrusNajmabadi · 2017-04-20T19:08:08Z

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

+        private static IVariableDeclarationStatement CreateBoundLocalDeclarationOperation(BoundLocalDeclaration boundLocalDeclaration)
+        {
+            Lazy<ImmutableArray<IVariableDeclaration>> declarations = new Lazy<ImmutableArray<IVariableDeclaration>>(() => GetVariableDeclarationStatementVariables(boundLocalDeclaration));
+            bool isInvalid = boundLocalDe


why is this typed to 'object'?

which one are you talking about?

CyrusNajmabadi · 2017-04-20T19:08:51Z

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

+    /// <summary>
+    /// Represents an expression that creates a pointer value by taking the address of a reference.
+    /// </summary>
+    internal sealed partial class AddressOfExpression : AddressOfExpressionBase, IAddressOfExpression


I think we use the pattern BaseXXX, not XXX base. i.e. BaseTypeDeclarationSyntax

heejaechang · 2017-04-21T18:20:06Z

@AlekseyTs can you take a look? tagging @dotnet/roslyn-compiler since I need 2 compiler dev sign off.

AlekseyTs · 2017-04-21T20:59:04Z

@heejaechang Are there any behavior changes in this PR? #Closed

AlekseyTs · 2017-04-21T21:14:28Z

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

@@ -246,7 +246,7 @@ internal BoundExpression ConvertPatternExpression(TypeSymbol inputType, CSharpSy
                case SyntaxKind.SingleVariableDesignation:
                    break;
                case SyntaxKind.DiscardDesignation:
-                    return new BoundDeclarationPattern(node, null, boundDeclType, isVar, hasErrors);
+                    return new BoundDeclarationPattern(node, null, new BoundDiscardExpression(node, boundDeclType.Type), boundDeclType, isVar, hasErrors);


return new BoundDeclarationPattern(node, null, new BoundDiscardExpression(node, boundDeclType.Type), boundDeclType, isVar, hasErrors); [](start = 20, length = 134)

Please revert this file to the original state. #Closed

Also, restore the constructor that is, apparently, deleted in this PR. Place it into Constructors.cs

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

sure will do.

AlekseyTs · 2017-04-21T21:17:09Z

src/Compilers/CSharp/Portable/BoundTree/Expression.cs

-        protected override OperationKind ExpressionKind => OperationKind.None;
-    }
-
-    internal partial class BoundDeclarationPattern


internal partial class BoundDeclarationPattern [](start = 4, length = 46)

This partial declaration should be moved to Constructors.cs rather than deleted. #Closed

AlekseyTs · 2017-04-21T21:19:06Z

src/Compilers/CSharp/Portable/Compilation/MemberSemanticModel.cs

@@ -1542,7 +1542,7 @@ private static Binder GetQueryEnclosingBinder(int position, CSharpSyntaxNode sta
            }
            while (node != null);

-done:
+            done:
            return GetEnclosingBinderInternalWithinRoot(AdjustStartingNodeAccordingToNewRoot(startingNode, queryClause.Syntax),


Please revert formatting change for this line. #Closed

AlekseyTs · 2017-04-21T21:23:42Z

Done with review pass. #Closed

genlu · 2017-04-21T21:51:54Z

src/Compilers/CSharp/Portable/BoundTree/Expression.cs

-
-        public override TResult Accept<TArgument, TResult>(OperationVisitor<TArgument, TResult> visitor, TArgument argument)
-        {
-            return visitor.VisitNoneOperation(this, argument);


Why not move this to CSharpOperationFactory and delete this file?

sure. I can do that.

heejaechang · 2017-04-21T22:21:40Z

@AlekseyTs this shouldn't contain any behavior change.

genlu · 2017-04-21T22:26:31Z

LGTM besides changes @AlekseyTs requested.

heejaechang · 2017-04-21T23:24:53Z

retest windows_eta_open_prtest please

AlekseyTs · 2017-04-24T16:48:07Z

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

@@ -180,8 +180,8 @@ internal BoundExpression ConvertPatternExpression(TypeSymbol inputType, CSharpSy
                {
                    case ConversionKind.ExplicitDynamic:
                    case ConversionKind.ImplicitDynamic:
-                        // Since the input was `dynamic`, which is equivalent to `object`, there must also
-                        // exist some unboxing, identity, or reference conversion as well, making the conversion legal.
+                    // Since the input was `dynamic`, which is equivalent to `object`, there must also


// Since the input was dynamic, which is equivalent to object, there must also [](start = 20, length = 82)

Please revert this file to its original state. #Closed

AlekseyTs · 2017-04-24T16:54:16Z

Done with review pass. Please revert src/Compilers/CSharp/Portable/Binder/Binder_Patterns.cs to its original state.

mavasani · 2017-04-24T21:29:54Z

@AlekseyTs can you take a look? I did what you asked. this is HeeJae.

AlekseyTs

LGTM

heejaechang · 2017-04-24T21:45:28Z

@cston @jaredpar @VSadov @jcouv can you take a look?

333fred · 2017-04-24T22:38:27Z

src/Compilers/CSharp/Portable/CSharpCodeAnalysis.csproj

@@ -867,6 +866,7 @@
    <InternalsVisibleToTest Include="Roslyn.Services.Editor.CSharp.UnitTests" />
    <InternalsVisibleToTest Include="Roslyn.Services.Editor2.UnitTests" />
  </ItemGroup>
+  <ItemGroup />


Empty group?

cston · 2017-04-26T22:00:16Z

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

+            Optional<object> constantValue = default(Optional<object>);
+            return new LazyVariableDeclarationStatement(declarations, isInvalid, syntax, type, constantValue);
+        }
+        private static IVariableDeclarationStatement C


return blockStatement.Statements.SelectAsArray(s => Create(s)).WhereAsArray(s => s.Kind != OperationKind.None);

@cston I think the current form is less allocation since SelectAs or WhereAs create new immutablearray. if there is only one SelectAs or WhereAsArray then they are better but in this case, there are 2 linq attached. so I think what I have is less allocation?

@heejaechang You may be right that LINQ may be cheaper than the combination of SelectAs and WhereAs. For LINQ, I suspect there are 4 allocations: boxing the Statements struct, the Select result, the Where result, and the ToImmutableArray() result. The alternative should be at most two allocations: one for SelectAs, and zero or one for WhereAs depending on whether WhereAs returns a non-empty subset, That said, if there are two allocations, the total bytes allocated may be larger than the LINQ approach. Feel free to keep as is.

cston · 2017-04-26T23:25:54Z

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

@@ -901,119 +1600,304 @@ public override void Accept(OperationVisitor visitor)
    }

    /// <summary>
+    /// Represents a C# foreach statement or a VB For Each staement.


Typo: statement

cston · 2017-04-26T23:36:58Z

LGTM

333fred · 2017-04-27T17:56:27Z

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

+        public static IExpressionStatement CreateCompoundAssignmentExpressionStatement(
+            IOperation target, IOperation value, BinaryOperationKind binaryOperationKind, IMethodSymbol operatorMethod, SyntaxNode syntax)
+        {
+            var isInvalid = target == null || target.IsInvalid || value == null || value.IsInvalid;


Should you be checking if binaryOperationKind is BinaryOperationKind.Invalid here?

@333fred you might be correct but for now, I am keeping code exactly same as before.

Can we file a bug to keep track?

we just decide to remove isInvalid from IOperation. so that solves this issue :)

heejaechang added 8 commits March 31, 2017 14:00

add place holder

f1b5719

csharp factory

07914e3

made sealed and remove remark

78ab56b

slit types to base and sealed.

b75730c

added lazy version of operation types.

7efa0ac

first move to lazy type.

23bba45

regenerated xml.generated file

2898c12

new factory file

f6c025e

dnfclas added the cla-already-signed label Apr 19, 2017

heejaechang added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 19, 2017

heejaechang added 6 commits April 19, 2017 11:22

removed unused files

24576c8

trying to fix merge conflict

d32fe96

Merge branch 'master' of https://github.com/dotnet/roslyn into Operat…

768e9e8

…ionFactory

fixed up merge conflicts

3c6858d

fixed test failures

b622903

removed argument null check for none lazy parameter

d871d6d

otherwise, it throws all over the places.

heejaechang changed the title ~~Operation factory~~ added CSharpOperationFactory Apr 20, 2017

CyrusNajmabadi reviewed Apr 20, 2017

View reviewed changes

heejaechang added 2 commits April 20, 2017 17:05

moved Base to front from the end

41ea8ca

use kind instead of type matching which is expansive than kind

79dde73

heejaechang added the Area-Analyzers label Apr 21, 2017

heejaechang removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Apr 21, 2017

AlekseyTs reviewed Apr 21, 2017

View reviewed changes

genlu reviewed Apr 21, 2017

View reviewed changes

PR feedback

1814743

AlekseyTs reviewed Apr 24, 2017

View reviewed changes

heejaechang added 2 commits April 24, 2017 14:38

removed formatting change

5c783b2

move new line back

50a5136

AlekseyTs approved these changes Apr 24, 2017

View reviewed changes

333fred reviewed Apr 24, 2017

View reviewed changes

jinujoseph added the Area-Compilers label Apr 26, 2017

cston reviewed Apr 26, 2017

View reviewed changes

333fred reviewed Apr 27, 2017

View reviewed changes

PR feedback

5400f82

heejaechang changed the base branch from master to features/ioperation April 27, 2017 22:58

heejaechang merged commit 29c7b6e into dotnet:features/ioperation Apr 27, 2017

added CSharpOperationFactory #18825

added CSharpOperationFactory #18825

Conversation

heejaechang commented Apr 19, 2017 • edited by jinujoseph Loading

heejaechang commented Apr 20, 2017

heejaechang commented Apr 20, 2017

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

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

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

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

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

heejaechang commented Apr 21, 2017

AlekseyTs commented Apr 21, 2017 • edited Loading

AlekseyTs Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Apr 21, 2017

genlu commented Apr 21, 2017

heejaechang commented Apr 21, 2017

AlekseyTs Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Apr 24, 2017

mavasani commented Apr 24, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

heejaechang commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston commented Apr 26, 2017

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

heejaechang commented Apr 19, 2017 •

edited by jinujoseph

Loading

AlekseyTs commented Apr 21, 2017 •

edited

Loading

AlekseyTs Apr 21, 2017 •

edited

Loading

AlekseyTs Apr 21, 2017 •

edited

Loading

AlekseyTs Apr 21, 2017 •

edited

Loading

AlekseyTs Apr 21, 2017 •

edited

Loading

AlekseyTs commented Apr 21, 2017 •

edited

Loading

AlekseyTs Apr 24, 2017 •

edited

Loading

cston Apr 27, 2017 •

edited

Loading