Do not expose operation nodes for BoundRangeVariables in C# query exp… #23080

mavasani · 2017-11-08T21:38:42Z

…ression

These nodes contain the bound parameter reference to the underlying lambda parameter, which is now exposed.
Fixes #23079
vso : https://devdiv.visualstudio.com/DevDiv/NET%20Developer%20Experience%20Productivity/_workitems/edit/521947

…ression These nodes contain the bound parameter reference to the underlying lambda parameter, which is now exposed. Fixes dotnet#23079

mavasani · 2017-11-08T22:58:23Z

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

@@ -43,14 +43,14 @@ protected override BoundExpression BindRangeVariable(SimpleNameSyntax node, Rang
                        // the range variable maps directly to a use of the parameter of that name
                        var value = base.parameterMap[qv.Name];
                        Debug.Assert(value.Count == 1);
-                        translation = new BoundParameter(node, value.Single()) { WasCompilerGenerated = true };


This is causing test failures where GetSymbolInfo is now returning parameter symbol instead of range variable symbol. I will revert the changes to this file and attempt to handle this correctly in the operation factory.

…meter is marked explicit

tmat · 2017-11-09T00:38:19Z

test ubuntu_16_debug_prtest please

mavasani · 2017-11-09T00:55:59Z

test ubuntu_16_debug_prtest please

natidea · 2017-11-09T01:10:16Z

Approved to merge via https://devdiv.visualstudio.com/DevDiv/_workitems/edit/521947

AlekseyTs · 2017-11-10T00:36:22Z

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

@@ -456,7 +459,8 @@ private IParameterReferenceOperation CreateBoundParameterOperation(BoundParamete
            SyntaxNode syntax = boundParameter.Syntax;
            ITypeSymbol type = boundParameter.Type;
            Optional<object> constantValue = ConvertToOptional(boundParameter.ConstantValue);
-            bool isImplicit = boundParameter.WasCompilerGenerated;
+            // We want to mark compiler generated parameter reference for range variables as explicit because we don't expose any operation nodes for range variables.
+            bool isImplicit = boundParameter.WasCompilerGenerated && boundParameter.AssociatedRangeVariable == null;


&& boundParameter.AssociatedRangeVariable == null [](start = 65, length = 50)

This doesn't feel right only top level node in the BoundRangeVariable.Value should be explicit and only if BoundRangeVariable itself wasnt compiler generated #Closed

AlekseyTs · 2017-11-10T00:39:09Z

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

@@ -996,11 +992,15 @@ public static void Main(string[] args)
                                      IArgumentOperation (ArgumentKind.Explicit, Matching Parameter: z) (OperationKind.Argument, Type: null, IsImplicit) (Syntax: 'g + x*100')
                                        IBinaryOperation (BinaryOperatorKind.Add) (OperationKind.BinaryOperator, Type: System.Int32) (Syntax: 'g + x*100')
                                          Left: 
-                                            IOperation:  (OperationKind.None, Type: null) (Syntax: 'g')
+                                            IPropertyReferenceOperation: System.Int32 <anonymous type: System.Int32 x, System.Int32 g>.g { get; } (OperationKind.PropertyReference, Type: System.Int32, IsImplicit) (Syntax: 'g')


IsImplicit [](start = 200, length = 10)

This node should be explicit, receiver should be implicit. #Closed

AlekseyTs · 2017-11-10T00:40:12Z

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

+                              Instance Receiver: 
+                                IPropertyReferenceOperation: <anonymous type: System.Int32 x, System.Int32 g> <anonymous type: <anonymous type: System.Int32 x, System.Int32 g> <>h__TransparentIdentifier0, System.Int32 z>.<>h__TransparentIdentifier0 { get; } (OperationKind.PropertyReference, Type: <anonymous type: System.Int32 x, System.Int32 g>, IsImplicit) (Syntax: 'x')
+                                  Instance Receiver: 
+                                    IParameterReferenceOperation: <>h__TransparentIdentifier1 (OperationKind.ParameterReference, Type: <anonymous type: <anonymous type: System.Int32 x, System.Int32 g> <>h__TransparentIdentifier0, System.Int32 z>) (Syntax: 'x')


IParameterReferenceOperation [](start = 36, length = 28)

This node should be implicit. #Closed

AlekseyTs · 2017-11-10T00:42:37Z

src/Compilers/CSharp/Test/Symbol/Symbols/AnonymousTypesSemanticsTests.cs

-          IOperation:  (OperationKind.None, Type: null) (Syntax: 'x')
-      IOperation:  (OperationKind.None, Type: null) (Syntax: 'x')
+          IParameterReferenceOperation: x (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'x')
+      IParameterReferenceOperation: x (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'x')


IParameterReferenceOperation: x (OperationKind.ParameterReference, Type: System.Int32) (Syntax: 'x') [](start = 6, length = 100)

It doesn't look like the initializers are in the right order. #Closed

Tracked with #23115

AlekseyTs · 2017-11-10T00:48:26Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1021,6 +1021,8 @@
    <Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>

    <Field Name="ParameterSymbol" Type="ParameterSymbol"/>
+    <!-- If this is a compiler generated parameter reference created for a range variable access in query expression, then this contains the range variable -->
+    <Field Name="AssociatedRangeVariable" Type="RangeVariableSymbol" Null="allow"/>


[](start = 4, length = 79)

I think we don't actually need this property because, often, the parameter itself is not associated with a range variable, but a property accessed of off the parameter is associated instead. #Closed

AlekseyTs · 2017-11-10T00:51:58Z

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

@@ -43,14 +43,14 @@ protected override BoundExpression BindRangeVariable(SimpleNameSyntax node, Rang
                        // the range variable maps directly to a use of the parameter of that name
                        var value = base.parameterMap[qv.Name];
                        Debug.Assert(value.Count == 1);
-                        translation = new BoundParameter(node, value.Single()) { WasCompilerGenerated = true };
+                        translation = new BoundParameter(node, value.Single(), associatedRangeVariable: qv) { WasCompilerGenerated = true };


{ WasCompilerGenerated = true } [](start = 108, length = 31)

I think you initial change too remove WasCompilerGenerated = true was the right thing to do. We simply need to override VisitRangeVariable in Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.NodeMapBuilder in such a way so that BoundRangeVariable.Value isn't visited. #Closed

AlekseyTs · 2017-11-10T00:52:53Z

                return new BoundRangeVariable(node, qv, translation, translation.Type);

We should make sure that the top node remains WasCompilerGenerated == false #Closed

Refers to: src/Compilers/CSharp/Portable/Binder/Binder.WithQueryLambdaParametersBinder.cs:62 in 43e6ce0. [](commit_id = 43e6ce0, deletion_comment = False)

AlekseyTs · 2017-11-10T01:05:53Z

Done with review pass (iteration 3). #Closed

mavasani · 2017-11-10T16:51:39Z

@AlekseyTs Addressed your feedback

AlekseyTs · 2017-11-13T01:29:56Z

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

@@ -411,7 +411,7 @@ public BoundParameter(SyntaxNode syntax, ParameterSymbol parameterSymbol, bool h
        }

        public BoundParameter(SyntaxNode syntax, ParameterSymbol parameterSymbol)
-            : this(syntax, parameterSymbol, parameterSymbol.Type)
+            : this(syntax, parameterSymbol, type: parameterSymbol.Type)


: this(syntax, parameterSymbol, type: parameterSymbol.Type) [](start = 12, length = 59)

Consider reverting changes to this file. #Closed

AlekseyTs · 2017-11-13T01:37:42Z

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

@@ -3597,7 +3639,9 @@ where x.ToString() == y.ToString()
                    Children(1):
                        IOperation:  (OperationKind.None, Type: null, IsInvalid) (Syntax: 'x.ToString')
                          Children(1):
-                              IOperation:  (OperationKind.None, Type: null, IsInvalid) (Syntax: 'x')
+                              IInvalidOperation (OperationKind.Invalid, Type: ?, IsInvalid, IsImplicit) (Syntax: 'x')


IInvalidOperation (OperationKind.Invalid, Type: ?, IsInvalid, IsImplicit) (Syntax: 'x') [](start = 30, length = 87)

It feels like there should be an explicit node associated with (Syntax: 'x') and probably it should be this node. #Closed

This seems to be due to incorrect implementation of IsImplicit for bad expression and statements in both C# and VB operation factories. Filed #23164 to track this issue.

AlekseyTs · 2017-11-13T01:40:44Z

Done with review pass (iteration 4). #Closed

mavasani · 2017-11-14T19:30:55Z

@AlekseyTs addressed your feedback.

@cston @jcouv @dotnet/analyzer-ioperation Can I get a review?

AlekseyTs

LGTM

mavasani · 2017-11-15T18:07:05Z

Ping @dotnet/roslyn-compiler for one more review.

Do not expose operation nodes for BoundRangeVariables in C# query exp…

64fe5e5

…ression These nodes contain the bound parameter reference to the underlying lambda parameter, which is now exposed. Fixes dotnet#23079

mavasani added Area-Analyzers Area-Compilers Feature - IOperation IOperation labels Nov 8, 2017

mavasani requested review from AlekseyTs, cston, jcouv and a team November 8, 2017 21:38

jinujoseph added the Needs Shiproom Approval label Nov 8, 2017

Remove a comment in tests that no longer applies

5c8444d

mavasani commented Nov 8, 2017

View reviewed changes

Use a different approach to ensure the operation node for lambda para…

43e6ce0

…meter is marked explicit

natidea added Approved to merge and removed Needs Shiproom Approval labels Nov 9, 2017

AlekseyTs reviewed Nov 10, 2017

View reviewed changes

Address PR feedback

7efc21f

AlekseyTs reviewed Nov 13, 2017

View reviewed changes

Revert changes to file

3b422a0

AlekseyTs approved these changes Nov 14, 2017

View reviewed changes

mavasani changed the base branch from master to post-dev15.5-contrib November 14, 2017 19:53

cston approved these changes Nov 15, 2017

View reviewed changes

mavasani merged commit 52a759e into dotnet:post-dev15.5-contrib Nov 15, 2017

mavasani mentioned this pull request Nov 15, 2017

BoundRangeVariable should not exist in the C# operation tree #23079

Closed

jcouv added this to the 15.6 milestone Jan 9, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not expose operation nodes for BoundRangeVariables in C# query exp… #23080

Do not expose operation nodes for BoundRangeVariables in C# query exp… #23080

mavasani commented Nov 8, 2017 •

edited by jinujoseph

Loading

mavasani Nov 8, 2017

tmat commented Nov 9, 2017

mavasani commented Nov 9, 2017

natidea commented Nov 9, 2017

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

mavasani Nov 10, 2017

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs commented Nov 10, 2017 •

edited

Loading

AlekseyTs commented Nov 10, 2017 •

edited

Loading

mavasani commented Nov 10, 2017

AlekseyTs Nov 13, 2017 •

edited

Loading

AlekseyTs Nov 13, 2017 •

edited

Loading

mavasani Nov 14, 2017

AlekseyTs commented Nov 13, 2017 •

edited

Loading

mavasani commented Nov 14, 2017

AlekseyTs left a comment

mavasani commented Nov 15, 2017

Do not expose operation nodes for BoundRangeVariables in C# query exp… #23080

Do not expose operation nodes for BoundRangeVariables in C# query exp… #23080

Conversation

mavasani commented Nov 8, 2017 • edited by jinujoseph Loading

mavasani Nov 8, 2017

Choose a reason for hiding this comment

tmat commented Nov 9, 2017

mavasani commented Nov 9, 2017

natidea commented Nov 9, 2017

AlekseyTs Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

mavasani Nov 10, 2017

Choose a reason for hiding this comment

AlekseyTs Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Nov 10, 2017 • edited Loading

AlekseyTs commented Nov 10, 2017 • edited Loading

mavasani commented Nov 10, 2017

AlekseyTs Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Nov 13, 2017 • edited Loading

Choose a reason for hiding this comment

mavasani Nov 14, 2017

Choose a reason for hiding this comment

AlekseyTs commented Nov 13, 2017 • edited Loading

mavasani commented Nov 14, 2017

AlekseyTs left a comment

Choose a reason for hiding this comment

mavasani commented Nov 15, 2017

mavasani commented Nov 8, 2017 •

edited by jinujoseph

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 10, 2017 •

edited

Loading

AlekseyTs commented Nov 10, 2017 •

edited

Loading

AlekseyTs commented Nov 10, 2017 •

edited

Loading

AlekseyTs Nov 13, 2017 •

edited

Loading

AlekseyTs Nov 13, 2017 •

edited

Loading

AlekseyTs commented Nov 13, 2017 •

edited

Loading