Expose if a Binary/Unary operator was 'Lifted' at the IExpression level. #14779

CyrusNajmabadi · 2016-10-27T20:18:27Z

CyrusNajmabadi · 2016-10-27T20:18:41Z

Tagging @dotnet/roslyn-compiler @JohnHamby @ManishJayaswal

AlekseyTs · 2016-10-28T17:52:10Z

src/Compilers/Core/Portable/Compilation/Expression.cs

+            BinaryOperationKind binaryOperationKind,
+            IOperation left, IOperation right, 
+            ITypeSymbol resultType, 
+            SyntaxNode syntax, bool isLifted)


, bool isLifted [](start = 29, length = 15)

Consider placing this parameter after the binaryOperationKind. #Closed

i preferred this approach because it made using named-arguments later much easier... But if you prefer it next to the kind, i can oblige :)

AlekseyTs · 2016-10-28T18:23:56Z

src/Compilers/VisualBasic/Portable/BoundTree/Statement.vb

@@ -636,11 +636,19 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                            ' Use the operator methods. Figure out the precise rules first.
                            Return Nothing
                        Else
+                            ' Operators are not used here.  So this is all simple, non-lifted stepping.


' Operators are not used here. So this is all simple, non-lifted stepping. [](start = 28, length = 75)

Is this true? For example:

Dim x As Integer? Dim y As Integer? = 16 For x = 12 To y System.Console.WriteLine(x) If x = 14 Then x = Nothing End If Next ``` #Closed

The case where there are user defined operators is in the higher 'if' branch (which is currently not implemented for ioperation). That was my reading of the code at least.

The example doesn't use user-defined operators. #Closed

oh. good point... will take a looksee.

AlekseyTs · 2016-10-28T18:26:53Z

src/Compilers/VisualBasic/Portable/BoundTree/Expression.vb

@@ -742,6 +760,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
            End Get
        End Property

+        Private ReadOnly Property IUnaryOperatorExpression_IsLifted As Boolean Implements IBinaryOperatorExpression.IsLifted


IUnaryOperatorExpression_IsLifted As Boolean Implements IBinaryOperatorExpression.IsLifted [](start = 34, length = 90)

Name mismatch. #Closed

AlekseyTs · 2016-10-28T18:27:38Z

src/Compilers/VisualBasic/Portable/BoundTree/Expression.vb

@@ -864,6 +888,12 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
            End Get
        End Property

+        Private ReadOnly Property IUnaryOperatorExpression_IsLifted As Boolean Implements IBinaryOperatorExpression.IsLifted


IUnaryOperatorExpression_IsLifted As Boolean Implements IBinaryOperatorExpression.IsLifted [](start = 33, length = 91)

Name mismatch #Closed

AlekseyTs · 2016-10-28T18:35:50Z

It looks like there are no tests for C#. It looks like not all touched code paths in VB are covered by new tests. Some interesting scenarios are also not covered: user-defined operators, etc. #Closed

AlekseyTs

More testing is needed.

CyrusNajmabadi · 2016-10-28T19:33:42Z

Will add more tests!

CyrusNajmabadi · 2016-10-28T21:38:01Z

Ok. A bunch more tests added. And i went and walked through lowering (which was insane BTW). I think the logic ends up being fairly straightforward though once you see how the binary expressions finally lower.

msJohnHamby · 2016-10-28T22:46:02Z

Is lifting a language concept or an implementation concept?

CyrusNajmabadi · 2016-10-28T22:52:01Z

Is lifting a language concept

It's a language concept. In C#, for example, it's in sections: "6.4.2 Lifted Conversion Operators" and "7.3.7 Lifted Operators".

The languages basically will promote existing operators on value types so that they will work on Nullable versions of those value types. That lifting occurs for both built-in opreators (like + on integers) as well as user defined operators.

This is, for example, what allows you to do the following:

int? a = 1;
int? b = 1;
var c = a + b;

Nullable<int> has no + operator. But it gets one for free from the built-in + operator for int.

CyrusNajmabadi · 2016-10-28T22:54:48Z

BTW< i like having the baseline-generating system for IOperation tests. It should be very helpful for banging out tons of tests when we get around to enabling this system again.

Note: for places where IOperation synthesizes operations behind the scenes, has there been any thought about having the languages change their default bound representation to just be that form, and then have the current forms be the 'lowered' stage of the operations? I'm a little bit worried about having the languages pre-lower, and then having IOperation attempt to stitch in nodes that it thinks shoudl be there. It would be nice if the normal bound trees were just what IOperation exposed, without needing to translate.

CyrusNajmabadi · 2016-10-29T19:02:04Z

@AlekseyTs Can you take another look?

AlekseyTs · 2016-10-31T17:51:16Z

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

+
+namespace Microsoft.CodeAnalysis.CSharp.Semantic.UnitTests.Diagnostics
+{
+    public class IOperationTests : CSharpTestBase


IOperationTests [](start = 17, length = 15)

I thought we already had IOperation tests that verified tree shape for C# compiler. @JohnHamby, is this the case? Could you point to the location? I think we would prefer to keep tests in one place.

There is an IOperationTests.vb file with some tests of the proper form. I think that all C# tests of similar form are in PRs that have yet to be merged. For example, PR 11450 has some examples.

AlekseyTs · 2016-10-31T17:53:18Z

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

+    Local_1: System.Int32? y
+    IVariableDeclarationStatement (1 variables) (OperationKind.VariableDeclarationStatement)
+      IVariableDeclaration: System.Int32? y (OperationKind.VariableDeclaration)
+        Initializer: IUnaryOperatorExpression (UnaryOperationKind.IntegerMinus, IsLifted) (OperationKind.UnaryOperatorExpression, Type: System.Int32?)


IsLifted [](start = 80, length = 8)

All tests here are for a "lifted" case. Do we have tests for non-lifted cases? #Closed

I only created non-lifted tests for VB. i'll add some for C# as well!

Tests added!

AlekseyTs · 2016-10-31T18:00:51Z

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

@@ -208,7 +208,8 @@ private void VisitInstanceExpression(IOperation instance)

        internal override void VisitNoneOperation(IOperation operation)
        {
-            Assert.True(false, "Encountered an IOperation with `Kind == OperationKind.None` while walking the operation tree.");


Assert.True(false, "Encountered an IOperation with Kind == OperationKind.None while walking the operation tree."); [](start = 12, length = 116)

It looks like this is not supposed to happen. Bug?

Correct. We have some cases where bound nodes don't get converted to proper operations. I wanted to at least still allow baselines to get generated, rather than having tests that coudln't actually verify anything. We can trivially change this back in the future once we're reasonably sure that we've completed converting all bound nodes to operations.

AlekseyTs · 2016-10-31T18:04:08Z

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

@@ -790,7 +807,8 @@ public override void VisitLiteralExpression(ILiteralExpression operation)
        {
            LogString(nameof(ILiteralExpression));

-            if (operation.ConstantValue.HasValue && operation.ConstantValue.Value.ToString() == operation.Text)
+            if (operation.ConstantValue.HasValue &&
+                operation.ConstantValue.Value?.ToString() == operation.Text)


operation.ConstantValue.Value?.ToString() == operation.Text [](start = 16, length = 59)

Do we ever want to get in to the if when operation.Text is null?

Note: this was an issue where the user used the 'null' literal. Null literal is a constant with a constant value of 'null'. so "Value.ToString()" was crashing.

@JohnHamby to comment on what we want to do here.

Dunno if this is correct when the value is null and the text is "null". Maybe null requires a special case here.

I feel like the appropriate thing to do is just log the constant value. It's not clear to me why we're looking at the operation text at all.

AlekseyTs · 2016-10-31T18:11:12Z

src/Compilers/VisualBasic/Portable/BoundTree/Statement.vb

+                                ' This will be a lifted comparison if either the control variable or
+                                ' limit value is nullable itself.
+                                Dim isLifted = controlVariable.Type.SpecialType = SpecialType.System_Nullable_T OrElse
+                                               limitValue.Type.SpecialType = SpecialType.System_Nullable_T


This code looks incorrect, I believe constructed types are never special and always return None from SpecialType property. #Closed

There are more cases like this below.

Good point. Thanks!

AlekseyTs · 2016-10-31T18:34:42Z

src/Compilers/VisualBasic/Test/Semantic/Diagnostics/IOperationTests.vb

@@ -218,6 +218,773 @@ IExpressionStatement (OperationKind.ExpressionStatement)
        End Sub

        <Fact>
+        Public Sub VerifyLiftedUnaryOperators1()


VerifyLiftedUnaryOperators1 [](start = 19, length = 27)

Is there similar test for non-lifted scenario? #Closed

AlekseyTs · 2016-10-31T18:34:53Z

src/Compilers/VisualBasic/Test/Semantic/Diagnostics/IOperationTests.vb

+        End Sub
+
+        <Fact>
+        Public Sub VerifyLiftedUserDefinedUnaryOperators1()


VerifyLiftedUserDefinedUnaryOperators1 [](start = 19, length = 38)

Is there similar test for non-lifted scenario? #Closed

Will check!

AlekseyTs · 2016-10-31T18:36:09Z

src/Compilers/VisualBasic/Test/Semantic/Diagnostics/IOperationTests.vb

+        End Sub
+
+        <Fact>
+        Public Sub VerifyLiftedUserDefinedBinaryOperators1()


VerifyLiftedUserDefinedBinaryOperators1 [](start = 19, length = 39)

Are there tests for BoundUserDefinedShortCircuitingOperator?

There are now :)

AlekseyTs · 2016-10-31T18:38:25Z

src/Compilers/VisualBasic/Test/Semantic/Diagnostics/IOperationTests.vb

+
+  IForLoopStatement (LoopKind.For) (OperationKind.LoopStatement)
+    Condition: IConditionalChoiceExpression (OperationKind.ConditionalChoiceExpression, Type: System.Boolean, IsInvalid)
+        Condition: IBinaryOperatorExpression (BinaryOperationKind.Invalid) (OperationKind.BinaryOperatorExpression, Type: System.Boolean, IsInvalid)


BinaryOperationKind.Invalid [](start = 46, length = 27)

What does this mean here and below?

AlekseyTs · 2016-10-31T18:45:11Z

@CyrusNajmabadi Tests for for statements are not readable and, given the issues found in implementation, you didn't verify the output yourself. I will not be verifying them and will not sign-off on them in this form. Please change the tests so they are targeted to the changes that you are making.

cston · 2017-07-20T23:07:07Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IUnaryOperatorExpression.vb

+        Public Sub VerifyLiftedUnaryOperators1()
+            Dim source = <![CDATA[
+Class C
+    Sub Foo(x as Integer?)


Please rename to F, here and in other tests.

Would FU work? 👼

Please rename here and in other tests.

Wait, was this serious? Why would this be renamed?

cston · 2017-07-20T23:14:14Z

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

+        }
+
+        [Fact]
+        public void VerifyLiftedBinaryOperators1()


The BinaryOperator tests are in _IUnaryOperatorExpression.cs. Should the file be renamed? Similar comment for VB.

yes. i will move.

…ftedOperations

CyrusNajmabadi · 2017-07-28T21:28:41Z

@cston Ready for rereview.

cston · 2017-07-28T21:40:30Z

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

+        /// operator that is defined to work on a value type, 'lifted' operators are 
+        /// created to work on the <see cref="System.Nullable{T}"/> versions of those
+        /// value types.
+        /// </summary>


Should the comment use a term other than operator here?

cston · 2017-07-28T21:41:54Z

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

@@ -1016,6 +1022,13 @@ internal abstract partial class BaseCompoundAssignmentExpression : AssignmentExp
        /// </summary>
        public BinaryOperationKind BinaryOperationKind { get; }
        /// <summary>
+        /// <code>true</code> if this is a 'Lifted' compound assignment.  When there is an 


'lifted' (lowercase L) here and in other comments.

cston · 2017-07-28T21:51:29Z

src/Compilers/VisualBasic/Test/Semantic/IOperation/IOperationTests_IForLoopStatement.vb

+
+            Dim expectedOperationTree = <![CDATA[
+IForLoopStatement (LoopKind.For) (OperationKind.LoopStatement) (Syntax: 'For x = 12  ... Next')
+  Condition: IBinaryOperatorExpression (BinaryOperationKind.IntegerLessThanOrEqual) (OperationKind.BinaryOperatorExpression, Type: System.Boolean) (Syntax: 'y')


This is not a lifted comparison. Is that expected? (It looks like we may have calculated an Int32 value from the limit value before the loop.)

Investigating.

The compiler is somewhat interesting here... the limit value is actually viewed to have a 'narrowing nullable' conversion from "int?" to "int" here.

I've viewed the IL generated here and that matches what's going on:

IL_0004: call instance void valuetype [mscorlib]System.Nullable`1<int32>::.ctor(!0) IL_0009: ldloca.s y IL_000b: call instance !0 valuetype [mscorlib]System.Nullable`1<int32>::get_Value() IL_0010: stloc.2 IL_0011: ldc.i4.s 12 IL_0013: stloc.0 IL_0014: br.s IL_001a // loop start (head: IL_001a) IL_0016: ldloc.0 IL_0017: ldc.i4.1 IL_0018: add.ovf IL_0019: stloc.0 IL_001a: ldloc.0 IL_001b: ldloc.2 IL_001c: ble.s IL_0016 // end loop

In other words, afaict, the way this works is that instead of comparing against the boxed nullable value as we go through the loop, the boxed nullable is unboxed to get its value (which will throw if it is null), and then we compare against that as we go through. So this seems by design right now.

…ftedOperations

CyrusNajmabadi · 2017-08-01T20:31:18Z

@cston I think this is good to go in. Are you satisfied with the explanation for the "non lifted nullable comparison in the for loop" case?

Thanks!

cston · 2017-08-01T20:39:26Z

LGTM. Please squash commits when merging.

CyrusNajmabadi · 2017-08-01T21:36:41Z

Thanks!

…sion-expression-rewrite * dotnet/features/ioperation: (229 commits) Adding Ioperation tests for WhileUntilLoopStatement (dotnet#21047) marked assert that needs to be re-enabled addressing more PR feedbacks PR feedback Remove InvocationReasons enum boxing PR feedbacks Expose if a Binary/Unary operator was 'Lifted' at the IExpression level. (dotnet#14779) addressing PR feedback added comments Update VS Integration machines to 15.3 Preview 6 (dotnet#21240) fixed typo Fixed dotnet#18763 Compiler crash on bad code in the IDE (dotnet#20903) Fix typo in ERR_RefReturnParameter2 (dotnet#21235) Fix unbound recursion with const var field in script (dotnet#21223) Typo fix (dotnet#20513) PR feedbacks and added some more tests When producing character literals, surrogate characters should be escaped. (dotnet#20720) Fix build correctness issues Fix possible null reference warnings Adding ioperation tests for ForEachStatement (dotnet#21048) ...

dnfclas added the cla-already-signed label Oct 27, 2016

CyrusNajmabadi force-pushed the isLiftedOperations branch from e6001b6 to 05c973d Compare October 27, 2016 20:19

CyrusNajmabadi added Area-Compilers Feature - IOperation IOperation labels Oct 27, 2016

AlekseyTs reviewed Oct 28, 2016

View reviewed changes

AlekseyTs previously requested changes Oct 28, 2016

View reviewed changes

AlekseyTs reviewed Oct 31, 2016

View reviewed changes

CyrusNajmabadi force-pushed the isLiftedOperations branch from 6633fe6 to b22697d Compare October 31, 2016 18:23

AlekseyTs reviewed Oct 31, 2016

View reviewed changes

AlekseyTs closed this Oct 31, 2016

AlekseyTs reopened this Oct 31, 2016

CyrusNajmabadi added 2 commits July 20, 2017 14:57

Update test code.

2255d9a

Remove extension method.

a1583f6

cston reviewed Jul 20, 2017

View reviewed changes

CyrusNajmabadi added 10 commits July 21, 2017 12:15

Merge remote-tracking branch 'upstream/features/ioperation' into isLi…

15080e1

…ftedOperations

Merge remote-tracking branch 'upstream/features/ioperation' into isLi…

3f8b36e

…ftedOperations

Fixup tests.

d4359eb

Fixup tests.

bee4bfd

Fixup tests.

7978c7d

Fix instances of Invalid operations with nullable

266e17a

Compound assignments also need to state if they're lifted.

9c7682f

Move tests.

7bce2f4

Move tests.

2b37319

Move tests.

90bed06

cston reviewed Jul 28, 2017

View reviewed changes

CyrusNajmabadi added 4 commits July 31, 2017 14:33

Merge remote-tracking branch 'upstream/features/ioperation' into isLi…

ba46a8d

…ftedOperations

Simplify comment.

25d8d62

Case things consistently.

ee4d972

Use F instead of Foo. Because Foo is bad.

f021ba8

CyrusNajmabadi force-pushed the isLiftedOperations branch from 87e7722 to f021ba8 Compare July 31, 2017 22:04

jcouv added the Area-Compilers label Aug 1, 2017

CyrusNajmabadi merged commit e63e0d4 into dotnet:features/ioperation Aug 1, 2017

CyrusNajmabadi deleted the isLiftedOperations branch August 1, 2017 21:36

Expose if a Binary/Unary operator was 'Lifted' at the IExpression level. #14779

Expose if a Binary/Unary operator was 'Lifted' at the IExpression level. #14779

Conversation

CyrusNajmabadi commented Oct 27, 2016

CyrusNajmabadi commented Oct 27, 2016

AlekseyTs Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Oct 28, 2016 • edited Loading

AlekseyTs left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Oct 28, 2016

CyrusNajmabadi commented Oct 28, 2016

msJohnHamby commented Oct 28, 2016

CyrusNajmabadi commented Oct 28, 2016

CyrusNajmabadi commented Oct 28, 2016

CyrusNajmabadi commented Oct 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2016 • 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

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Oct 31, 2016 • 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

AlekseyTs commented Oct 31, 2016

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

CyrusNajmabadi commented Jul 28, 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

cston Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Aug 1, 2017

cston commented Aug 1, 2017

CyrusNajmabadi commented Aug 1, 2017

AlekseyTs Oct 28, 2016 •

edited

Loading

AlekseyTs Oct 28, 2016 •

edited

Loading

AlekseyTs Oct 28, 2016 •

edited

Loading

AlekseyTs Oct 28, 2016 •

edited

Loading

AlekseyTs Oct 28, 2016 •

edited

Loading

AlekseyTs commented Oct 28, 2016 •

edited

Loading

AlekseyTs Oct 31, 2016 •

edited

Loading

AlekseyTs Oct 31, 2016 •

edited

Loading

AlekseyTs Oct 31, 2016 •

edited

Loading

AlekseyTs Oct 31, 2016 •

edited

Loading

cston Jul 28, 2017 •

edited

Loading