changed how GetOperation work. #21857

heejaechang · 2017-08-31T22:45:13Z

changed GetOperation to always return one from root operation tree and never use recovery mode of semantic model.

…d never use recovery mode of semantic model.

heejaechang · 2017-08-31T22:46:13Z

expect a lot of tests to fail due to behavior change of GetOperation

AlekseyTs · 2017-09-06T21:37:01Z

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

+        {
+            var bindingRoot = GetBindingRoot(node);
+
+            // if binding root is field variable declarator, make it initializer


// if binding root is field variable declarator, make it initializer [](start = 12, length = 68)

// if binding root is field variable declarator, make it initializer [](start = 12, length = 68)

It would be good to provide an explanation why we are doing this. #Closed

added null check and make sure we use field initializer instead of whole field decl for C#

…into Parent2

heejaechang · 2017-09-07T22:32:48Z

@dotnet/roslyn-infrastructure test is not running, is this a known issue?

jasonmalinowski · 2017-09-07T22:35:49Z

@heejaechang http://ddoutages.azurewebsites.net/Outages/Details/26618

heejaechang · 2017-09-11T15:42:46Z

@dotnet/analyzer-ioperation @AlekseyTs @cston can I get some code review? this changes how GetOperation works and fixed all tests that are broken due to behavior changes.

heejaechang · 2017-09-11T15:45:51Z

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

@@ -706,7 +667,7 @@ private IOperation CreateBoundConversionOperation(BoundConversion boundConversio
                bool isChecked = conversion.IsNumeric && boundConversion.Checked;
                ITypeSymbol type = boundConversion.Type;
                Optional<object> constantValue = ConvertToOptional(boundConversion.ConstantValue);
-                return new LazyCSharpConversionExpression(operand, conversion, isExplicit, isTryCast, isChecked, _semanticModel, syntax, type, constantValue, isImplicit);
+                return new LazyCSharpConversionExpression(operand, conversion, isExplicit, isTryCast, isChecked, _semanticModel, syntax, type, constantValue, isImplicit || !isExplicit);


implicit conversion require checking both compiler generated + explicit cast in code since compiler won't mark it as compiler generated

heejaechang · 2017-09-11T15:47:24Z

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

-            });
-        }
+                var statement = (LocalDeclarationStatementSyntax)boundLocalDeclaration.Syntax;
+                var variableDeclarator = statement.Declaration.Variables.First();


use declarator for IVariableDeclaration since that is what multiple local declaration is using to make things consistent.

heejaechang · 2017-09-11T15:48:44Z

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

        }

        private IVariableDeclarationStatement CreateBoundMultipleLocalDeclarationsOperation(BoundMultipleLocalDeclarations boundMultipleLocalDeclarations)
        {
            Lazy<ImmutableArray<IVariableDeclaration>> declarations = new Lazy<ImmutableArray<IVariableDeclaration>>(() =>
-                boundMultipleLocalDeclarations.LocalDeclarations.SelectAsArray(declaration => CreateVariableDeclaration(declaration)));
+                boundMultipleLocalDeclarations.LocalDeclarations.SelectAsArray(declaration => (IVariableDeclaration)Create(declaration)));


made it to follow the pattern rather than cut in the middle.

heejaechang · 2017-09-11T15:49:56Z

src/Compilers/CSharp/Test/Semantic/Diagnostics/DiagnosticAnalyzerTests.cs

                Diagnostic("GeneratedCodeAnalyzerSummary").WithArguments("Node: var mixMethodUserVar = 0,Node: var userVar = 0,Operation: MixMethod,Operation: NonHiddenMethod").WithLocation(1, 1));

            analyzers = new DiagnosticAnalyzer[] { new GeneratedCodeSyntaxAndOperationAnalyzer(GeneratedCodeAnalysisFlags.Analyze, syntaxKinds, operationKinds) };
            compilation.VerifyAnalyzerDiagnostics(analyzers, null, null, true,
                Diagnostic("GeneratedCodeAnalyzerWarning", "var userVar = 0").WithArguments("Node: var userVar = 0").WithLocation(17, 9),
-                Diagnostic("GeneratedCodeAnalyzerWarning", "var userVar = 0;").WithArguments("Operation: NonHiddenMethod").WithLocation(17, 9),
+                Diagnostic("GeneratedCodeAnalyzerWarning", "userVar = 0").WithArguments("Operation: NonHiddenMethod").WithLocation(17, 13),


now it point to declarator rather than declaration statement.

heejaechang · 2017-09-11T15:52:48Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IAnonymousFunctionExpression.cs

@@ -21,7 +21,7 @@ class Program
 {
    static void Main(string[] args)
    {
-        Action x /*<bind>*/= () => F()/*</bind>*/;
+        /*<bind>*/Action x = () => F();/*</bind>*/


before we moved node around to return something. for example, equal value returned declaration statement. now we always return what we got. we only return IOperation with declaration statement when we got declaration statement for GetOperation.

there is no matching IOperation for equal value in this case or varaible decleration.

Do we have a test that requests operation for a VariableDeclaration: Action x = () => F()?

Why will the VariableDeclarationSyntax not return an IVariableDeclaration?

I meant VariableDeclaratorSyntax not VariableDeclarationSyntax...

in Operation tree, there is no matching IOperation concept for VariableDeclarationSyntax. if user call GetOperation with it, it will return null.

in IOperation, statement points declarator directly without middle decleration like syntax node.

variable declaration syntax points to multiple declarator there is no such IOperation that can do that.

heejaechang · 2017-09-11T15:55:53Z

src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IArgument.cs

-        ILiteralExpression (OperationKind.LiteralExpression, Type: System.Int32, Constant: 10, IsInvalid) (Syntax: '10')
-        InConversion: null
-        OutConversion: null
+IInvalidExpression (OperationKind.InvalidExpression, Type: System.Int32, IsInvalid) (Syntax: 'this[10]')


now we will return first IOperation that has matching node given to GetOperation in operation tree. rather than return whatever node returned by node map (with high/low bound node option)

AlekseyTs · 2017-09-20T18:04:04Z

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

+
+    internal partial class BoundMethodGroup
+    {
+        protected override ImmutableArray<BoundNode> Children => ImmutableArray.Create<BoundNode>(this.ReceiverOpt);


protected override ImmutableArray Children => ImmutableArray.Create(this.ReceiverOpt); [](start = 8, length = 108)

Instead of duplicating this for BoundMethodGroup and BoundPropertyGroup, consider overriding this property once in BoundMethodOrPropertyGroup #Closed

This isn't a blocker.

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

AlekseyTs · 2017-09-20T18:20:26Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory.vb

            Dim syntax As SyntaxNode = boundSelectStatement.Syntax
            Dim type As ITypeSymbol = Nothing
            Dim constantValue As [Optional](Of Object) = New [Optional](Of Object)()
            Dim isImplicit As Boolean = boundSelectStatement.WasCompilerGenerated
            Return New LazySwitchStatement(value, cases, _semanticModel, syntax, type, constantValue, isImplicit)
        End Function

+        Private Function CreateBoundCaseBlockOperation(boundCaseBlock As BoundCaseBlock) As ISwitchCase


Private Function CreateBoundCaseBlockOperation(boundCaseBlock As BoundCaseBlock) As ISwitchCase [](start = 8, length = 95)

Is this change a part of an unrelated commit? #Closed

I see, it looks like the code was moved from another file.

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

no it was there before. case block wasn't go through the bound node -> ioperation map before, so I fixed it.

ah, ya, it was called directly from case statement or something, so I made it to go through Create(...) but implementation was already there. I just connected it to Create.

AlekseyTs · 2017-09-20T19:00:40Z

...pilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IParameterReferenceExpression.vb

@@ -856,10 +859,7 @@ End Structure
                references:={MscorlibRef, SystemRef, compilation0.EmitToImageReference(embedInteropTypes:=True)})

            Dim expectedOperationTree = <![CDATA[
-IInvalidExpression (OperationKind.InvalidExpression, Type: I, IsInvalid) (Syntax: 'New I(x)')
-  Children(2):
-      IParameterReferenceExpression: x (OperationKind.ParameterReferenceExpression, Type: System.Object, IsInvalid) (Syntax: 'x')


It looks like we are losing information about parameter reference, this feels wrong, please open an issue to follow up.

opened - #22229

...

the information is still there, just user gets the one of children initially that points to same syntax node rather than InvalidExpression. user can use Parent and children to get to the ParameterReferenceExpression.

AlekseyTs · 2017-09-20T19:08:04Z

IOperation: (OperationKind.None, IsInvalid) (Syntax: 'new I(x)')

It looks like information about parameter reference is lost, please open an issue to follow up.

Refers to: src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IParameterReferenceExpression.cs:890 in 4154db4. [](commit_id = 4154db4, deletion_comment = False)

AlekseyTs

LGTM

heejaechang · 2017-09-20T19:15:22Z

@cston can you take a look as well?

cston · 2017-09-20T19:23:39Z

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

+        {
+            get
+            {
+                System.Diagnostics.Debug.Assert(this.Kind == BoundKind.StatementList || this.Kind == BoundKind.Scope);


Will this assert fail when evaluating the property in the debugger?

let me try. didn't thought of that...

yes. it does. it shows
"Children = 'boundBlock.Children' threw an exception of type 'Microsoft.CodeAnalysis.ThrowingTraceListener.DebugAssertFailureException"
in debug window.

it used to show empty. now that exception for BoundBlock, and child statements for other such as StatementList or Scope.

Consider removing the assert.

but we dont want BoundBlock.Children to be used.

An assert in a property may result in a popup in the debugger though. If it's important to avoid BoundBlock.Children, perhaps return ImmutableArray<BoundNode>.Empty when Kind is not one of the expected values.

sure I can do that.

PS: it doesn't cause pop up, but value in the watch window or data tip says assert exception was thrown.

Aren't statements inside a block children of that block? I think they are.

Children should return something if the bound node is not supported by IOperation yet. this is a workaround we have until we can support all bound nodes we want to support. BoundBlock is already supported so children should never be called. that's why I have put assert.

cston · 2017-09-20T19:29:54Z

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

-                default:
-                    result = lowestBoundNode;
-                    break;
+                bindingRoot = parameter.Default;


Consider return parameter.Default; here rather than checking other cases below that appear to be distinct. Same comment for other bindingRoot = ...; cases below.

cston · 2017-09-20T19:50:01Z

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

@@ -1293,17 +1338,24 @@ private CSharpSyntaxNode GetBindingRoot(CSharpSyntaxNode node)

            StatementSyntax enclosingStatement = null;


Declaration can be moved inside for.

cston · 2017-09-20T19:57:13Z

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

@@ -274,9 +247,14 @@ private ImmutableArray<IOperation> GetIOperationChildren(BoundNode boundNode)
            }

            var builder = ArrayBuilder<IOperation>.GetInstance(boundNodeWithChildren.Children.Length);
-            foreach (var childNode in boundNodeWithChildren.Children)
+            foreach (BoundNode childNode in boundNodeWithChildren.Children)


Are there implementations of Children that allocate? If so, extract a local for boundNodeWithChildren.Children to avoid unnecessary allocations.

@cston can you give me little bit more on what you meant?

ah, I got it. okay.

cston · 2017-09-20T20:17:18Z

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

            return new CSharpArgument(kind,
                parameter,
                value,
                semanticModel: _semanticModel,
-                syntax: value.Syntax,
+                syntax: argumentExist ? value.Syntax.Parent : value.Syntax,


Consider using as above.

var argument = value.Syntax?.Parent as ArgumentSyntax; return new CSharpArgument( ... syntax: argument ?? value.Syntax,

cston · 2017-09-20T20:23:16Z

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

+        private IOperation CreateBoundCallInstanceOperation(BoundCall boundCall)
+        {
+            if (boundCall.Method == null || boundCall.Method.IsStatic)
+            {


Is this check necessary? In error cases, this will drop the receiver when the method is static but a receiver was provided, and will not prevent Create from being called for instance methods when a receiver was not included.

Please add tests for those cases if not currently tested.

I don't know. I converted existing code to the method.

Lazy<IOperation> instance = new Lazy<IOperation>(() => Create(((object)boundCall.Method == null || boundCall.Method.IsStatic) ? null : boundCall.ReceiverOpt));

tagging @dotnet/analyzer-ioperation for people who knows about this.

I think @cston is correct, we probably don't want to drop receiver unless it is a bound type node. We should follow up on this with tests and necessary implementation changes.

opened #22233

cston · 2017-09-20T20:25:04Z

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

+            //  2. the constant value of BoundEventAccess is always null.
+            //  3. the syntax of the boundEventAssignmentOperator is always AssignmentExpressionSyntax, so the syntax for the event reference would be the LHS of the assignment.
+            IEventSymbol @event = boundEventAssignmentOperator.Event;
+            Lazy<IOperation> instance = new Lazy<IOperation>(() => Create(boundEventAssignmentOperator.Event.IsStatic ? null : boundEventAssignmentOperator.ReceiverOpt));


Is the IsStatic check necessary?

I just moved this code from CSharpOperationFactory to this file.

tagging @genlu who wrote the code.

opened #22233

cston · 2017-09-20T20:35:36Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory.vb

@@ -1080,7 +1082,9 @@ Namespace Microsoft.CodeAnalysis.Semantics
            Dim syntax As SyntaxNode = boundBadStatement.Syntax
            Dim type As ITypeSymbol = Nothing
            Dim constantValue As [Optional](Of Object) = New [Optional](Of Object)()
-            Dim isImplicit As Boolean = boundBadStatement.WasCompilerGenerated
+
+            ' if child has syntax node point to same syntax node as bad statement, then this invalid statement Is implicit


is rather than Is.

cston · 2017-09-20T20:37:29Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

+                inConversion:=inConversion,
+                outConversion:=outConversion,
+                semanticModel:=_semanticModel,
+                syntax:=If(argumentExist, value.Syntax.Parent, value.Syntax),


Dim argument = TryCast(value.Syntax?.Parent, ArgumentSyntax) Return New VisualBasicArgument( ... syntax:=If(argument, value.Syntax),

heejaechang · 2017-09-20T21:49:12Z

I talked with @mavasani and decide to rebase this to master and checkin.

heejaechang added 2 commits August 31, 2017 15:25

changed GetOperation to always return one from root operation tree an…

43b6f93

…d never use recovery mode of semantic model.

fixed merge conflict

054a5e5

dnfclas added the cla-already-signed label Aug 31, 2017

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

AlekseyTs reviewed Sep 6, 2017

View reviewed changes

This was referenced Sep 7, 2017

IOperation - IOperation API for BoundMethodGroup and BoundPropertyGroup #19965

Closed

take another look on what to cache on arguments #20193

Closed

issue with VB parent pointer #21142

Closed

heejaechang added 2 commits September 7, 2017 12:22

changed GetOperation to only return explicit operation

9a63a61

added null check and make sure we use field initializer instead of whole field decl for C#

Merge branch 'features/ioperation' of https://github.com/dotnet/roslyn …

f1ba17f

…into Parent2

heejaechang force-pushed the Parent2 branch 2 times, most recently from 14ab0aa to afd0475 Compare September 7, 2017 21:55

heejaechang closed this Sep 7, 2017

heejaechang reopened this Sep 7, 2017

dnfclas added the cla-already-signed label Sep 7, 2017

heejaechang force-pushed the Parent2 branch from 19ad186 to 5351ab8 Compare September 9, 2017 04:49

heejaechang added 2 commits September 8, 2017 23:46

updated bunch of csharp tests

cbdf2f2

updated vb tests

8e91798

heejaechang force-pushed the Parent2 branch from 5351ab8 to 8e91798 Compare September 9, 2017 06:48

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

heejaechang commented Sep 11, 2017

View reviewed changes

mavasani mentioned this pull request Sep 20, 2017

Re-enable failing tests TestParentOperations and TestClone #22224

Closed

AlekseyTs reviewed Sep 20, 2017

View reviewed changes

made BoundMethodGroup and Property to share implementation of Children

4154db4

AlekseyTs reviewed Sep 20, 2017

View reviewed changes

AlekseyTs approved these changes Sep 20, 2017

View reviewed changes

heejaechang mentioned this pull request Sep 20, 2017

IsImplicit for multiple operation nodes with same syntax #22229

Closed

cston reviewed Sep 20, 2017

View reviewed changes

addressed PR feedbacks

a825162

cston reviewed Sep 20, 2017

View reviewed changes

cston approved these changes Sep 20, 2017

View reviewed changes

more pr feedbacks

2b2eac6

heejaechang mentioned this pull request Sep 20, 2017

preserve receiver even in error case where static method or event is called #22233

Closed

heejaechang changed the base branch from features/ioperation to master September 20, 2017 21:49

heejaechang added 4 commits September 20, 2017 17:53

fixed merge conflicts

daf2ff5

put compiler trait back in

7f72177

re-enabled TestClone and TestParentOperation in vb

bb2f1b0

more test fixes due to merge conflicts

a71f157

heejaechang merged commit a55a963 into dotnet:master Sep 21, 2017

heejaechang mentioned this pull request Sep 22, 2017

ported tests from https://github.com/dotnet/roslyn/pull/21263 #22296

Merged

		@@ -1293,17 +1338,24 @@ private CSharpSyntaxNode GetBindingRoot(CSharpSyntaxNode node)

		StatementSyntax enclosingStatement = null;

changed how GetOperation work. #21857

changed how GetOperation work. #21857

Conversation

heejaechang commented Aug 31, 2017

heejaechang commented Aug 31, 2017

AlekseyTs Sep 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Sep 7, 2017

jasonmalinowski commented Sep 7, 2017

heejaechang commented Sep 11, 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

heejaechang Sep 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Sep 20, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 20, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

heejaechang commented Sep 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

cston Sep 20, 2017 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 20, 2017 • 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

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 Sep 20, 2017

AlekseyTs Sep 6, 2017 •

edited

Loading

heejaechang Sep 13, 2017 •

edited

Loading

AlekseyTs Sep 20, 2017 •

edited

Loading

AlekseyTs Sep 20, 2017 •

edited

Loading

cston Sep 20, 2017 •

edited

Loading

heejaechang Sep 20, 2017 •

edited

Loading