added unary operator expression tests #18136

heejaechang · 2017-03-23T23:51:56Z

this includes only csharp tests. I will add vb tests next.

heejaechang · 2017-03-23T23:52:19Z

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

heejaechang · 2017-03-24T07:00:06Z

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

+
+";
+            string expectedOperationTree = @"
+IUnaryOperatorExpression (UnaryOperationKind.Invalid) (OperationKind.UnaryOperatorExpression, Type: System.Object, IsInvalid)


not sure in C#, how I can get BooleanBitwiseNegation - http://source.roslyn.io/#Microsoft.CodeAnalysis/Compilation/IExpression.cs,529

which seems possible according to this
http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/BoundTree/Expression.cs,2452

heejaechang · 2017-03-24T07:34:16Z

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

+            string expectedOperationTree = @"
+IUnaryOperatorExpression (UnaryOperationKind.Invalid) (OperationKind.UnaryOperatorExpression, Type: System.Object, IsInvalid)
+  IInvocationExpression ( System.Double A.Method()) (OperationKind.InvocationExpression, Type: System.Double)
+    Instance Receiver: IInstanceReferenceExpression (InstanceReferenceKin


not sure how to get DynamicTrue and DynamicFalse
http://source.roslyn.io/#Microsoft.CodeAnalysis/Compilation/IExpression.cs,544

heejaechang · 2017-03-24T07:58:17Z

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

+            Dim expectedOperationTree = <![CDATA[
+IUnaryOperatorExpression (UnaryOperationKind.OperatorMethodBitwiseNegation) (OperatorMethod: Function CustomType.op_OnesComplement(x As CustomType) As CustomType) (OperationKind.UnaryOperatorExpression, Type: CustomType)
+  IInvocationExpression ( Function A.Method() As CustomType) (OperationKind.InvocationExpression, Type: CustomType)
+    Instance Receiver: IInstanceReferenceExpression (InstanceReferenceKind.Implicit) (OperationKind.InstanceReferenceExpression, Type: A)


not sure how to get OperatorMethodTrue/False

even if I implement IsTrue/IsFalse/Or/And Operators, it doesn't get returned by GetOperation

behavior of IfStatement Operation between VB and C# seems different.

even if I implement IsTrue/IsFalse/Or/And Operators, it doesn't get returned by GetOperation

That is probably a bug. Can you add test case, and skip it with a bug?

implemented true and false test

heejaechang · 2017-03-24T07:59:29Z

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

+    ]]>.Value
+
+            Dim expectedOperationTree = <![CDATA[
+IUnaryOperatorExpression (UnaryOperationKind.BooleanBitwiseNegation) (OperationKind.UnaryOperatorExpression, Type: System.Boolean)


no LogicalNot in VB?

There should be some. From language spec, section 11.17 Logical Operators:

The And, Not, Or, and Xor operators, which are called the logical operators, are evaluated as follows: ...

However, we should figure out how do they map to IOperation universe.

@AlekseyTs @cston @jcouv what would be example of VB code that will use LogicalNot in bound node?

Dim b As Boolean = False Return Not b

It looks like Expression.DeriveUnaryOperationKind maps System_Boolean + Not to UnaryOperationKind.BooleanBitwiseNegation rather than UnaryOperationKind.BooleanLogicalNot though.

looks like in VB, there is no way to distinguish between BooleanLogicalNot and BooleanBitwiseNegation and looks like later one is choosen

Is the difference that LogicalNot is an operation on boolean and BitwiseNegation is an operation on integer types? For instance, Not b is a LogicalNot and Not i is BitwiseNegation below (equivalent to !b and ~i in C#?).

Dim b = False System.Console.WriteLine(Not b) Dim i = 25 System.Console.WriteLine(Not i)

@cston for those, there is IntergerBitwiseNegation kind. (http://source.roslyn.io/#Microsoft.CodeAnalysis/Operations/UnaryOperationKind.cs,25)

heejaechang · 2017-03-24T08:02:44Z

@mavasani can you take a look?

mavasani

Very good test coverage! I see there are 2 possible product bugs, please file them.

Someone from the compiler team can answer your language related questions.

mavasani · 2017-03-24T15:13:20Z

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

+
+
+        [Fact]
+        public void Test_UnaryOperatorExpression_Type_Minus_System_SByte()


Tests seem very comprehensive, and I see that you have covered all the possible combinations of the huge UnaryOperationKind enum 👍

mavasani · 2017-03-24T15:16:33Z

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

+    System.Byte Method()
+    {
+        System.Byte i = default(System.Byte);
+        return /*<bind>*/+Method()/*</bind>*/;


Not sure if we are getting much value add in testing +i in prior tests versus +Method() in these tests. You can probably change i to be a constant in previous tests to get different coverage?

Or I would just delete the +Method() tests unless you feel they are covering a different code path for binding unary operator.

since Operand can be any expression, I wanted to have at least 2 different kind of expression there. if you have better expression to use there, let me know.

mavasani · 2017-03-24T15:20:19Z

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

+
+
+        [Fact]
+        public void Test_UnaryOperatorExpression_Method_BitwiseNot_System_SByte()


Again, I don't see much value add in these ~Method() tests...

all tests are generated ha ha, that is why some tests are invalid :)

mavasani · 2017-03-24T15:21:18Z

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

+}
+
+";
+            string expectedOperationTree =


Is this even valid code?

no valid code, so result shows OperationKind.Invalid. do you only cover valid case?

No you should add even invalid code tests.

mavasani · 2017-03-24T15:24:50Z

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

+}
+
+";
+            string expectedOperationTree =


This should also have 2 ILocalReferenceExpression for reference to locals x and y - seems like a bug that it is missing. Can you file a bug please?

@mavasani I couldn't find which test you are refering to.

I was referring to https://github.com/dotnet/roslyn/pull/18136/files/74264afab568fc249c1f2ea0ad5552d8a2d24493#diff-b08381441daffef52069a7ea362ff9faR2660. IOperation for x && y should have local references I believe

mavasani · 2017-03-24T15:32:17Z

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

+            Dim expectedOperationTree = <![CDATA[
+IUnaryOperatorExpression (UnaryOperationKind.OperatorMethodBitwiseNegation) (OperatorMethod: Function CustomType.op_OnesComplement(x As CustomType) As CustomType) (OperationKind.UnaryOperatorExpression, Type: CustomType)
+  IInvocationExpression ( Function A.Method() As CustomType) (OperationKind.InvocationExpression, Type: CustomType)
+    Instance Receiver: IInstanceReferenceExpression (InstanceReferenceKind.Implicit) (OperationKind.InstanceReferenceExpression, Type: A)


even if I implement IsTrue/IsFalse/Or/And Operators, it doesn't get returned by GetOperation

That is probably a bug. Can you add test case, and skip it with a bug?

heejaechang · 2017-03-24T21:56:23Z

@dotnet/roslyn-infrastructure is this known issue?

error VSSDK1081: Problem occurred while extracting the vsix to the experimental extensions path. [D:\j\workspace\windows_relea---8a39a417\src\VisualStudio\Setup.Dependencies\VisualStudioSetup.Dependencies.csproj]

https://ci.dot.net/job/dotnet_roslyn/job/master/job/windows_release_vs-integration_prtest/1668/

heejaechang · 2017-03-24T22:16:34Z

@AlekseyTs can you take a look?

heejaechang · 2017-03-24T22:28:05Z

@dotnet/analyzer-ioperation can someone else take a look as well?

jinujoseph · 2017-03-24T22:46:08Z

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

+}
+
+";
+            string expectedOperationTree =


nit: not all facts have workitem associated

it is all same workitem so I skipped it except the first one :)

heejaechang · 2017-03-27T19:00:25Z

@AlekseyTs I have 2 sign off, can you take a look as final decider?

AlekseyTs · 2017-03-27T23:16:56Z

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

+}
+
+";
+            string expectedOperationTree =


Test_UnaryOperatorExpression_Method_LogicalNot_CustomType [](start = 20, length = 57)

It would be good to have tests for a scenario when CustomType doesn't have the right user-defined operator.

AlekseyTs · 2017-03-27T23:18:54Z

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

+}
+
+";
+            string expectedOperationTree =


CustomType CustomType.op_UnaryPlus(CustomType x) [](start = 82, length = 48)

Is it possible to come up with a scenario when UnaryOperationKind is UnaryOperationKind.OperatorMethodPlus, but the corresponding OperatorMethod is null?

looks like without operatormethod, kind becomes invalid

AlekseyTs · 2017-03-27T23:22:02Z

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

+}
+
+";
+            string expectedOperationTree =


Test_UnaryOperatorExpression_Type_Plus_CustomType [](start = 20, length = 49)

Consider testing scenarios when input, or output types for user-defined operators do not match CustomType exactly, including scenarios when input conversion exists and doesn't exist.

AlekseyTs · 2017-03-27T23:28:33Z

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

+}
+
+";
+            string expectedOperationTree =


VerifyOperationTreeForTest(source, expectedOperationTree); [](start = 12, length = 77)

Consider also checking the tree for x && y expression rather than for the entire if. Also, could be useful to test expressions outside of if context.

tracked by #18160

AlekseyTs · 2017-03-27T23:31:18Z

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

+}
+
+";
+            string expectedOperationTree =


Test_UnaryOperatorExpression_Type_Or_TrueFalse [](start = 20, length = 46)

Consider testing scenarios:

involving conversions;

with missing or malformed operators (some, all, etc.).

AlekseyTs · 2017-03-27T23:38:47Z

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

+}
+
+";
+            string expectedOperationTree =


Consider testing:

increment/decrement operators;

nullable operators.

pointer operators.

I dont believe those are considered unary operator, but sure. I can add tests for those

AlekseyTs · 2017-03-28T00:01:10Z

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

+    ]]>.Value
+
+            Dim expectedOperationTree = <![CDATA[
+IConversionExpression (ConversionKind.Basic, Implicit) (OperationKind.ConversionExpression, Type: E)


Do we want this conversion in the tree? #Closed

Probably a design issue, it looks like language doesn't say explicitly that +/- are supported for enums (sections 11.13.1 Unary Plus Operator and 11.13.2 Unary Minus Operator).

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

Looks correct after all.

In reply to: 108309336 [](ancestors = 108309336,108308547)

AlekseyTs · 2017-03-28T03:59:17Z

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

+
+            VerifyOperationTreeForTest(Of MultiLineIfBlockSyntax)(source, expectedOperationTree)
+        End Sub
+    End Class


End Class [](start = 4, length = 9)

Many test suggestions for C# are applicable to VB as well.

AlekseyTs · 2017-03-28T04:01:04Z

Done with review pass.

added vb unary operator tests

heejaechang · 2017-07-03T10:06:20Z

@dotnet/roslyn-compiler @dotnet/analyzer-ioperation adding unary operator unit tests. can you take a look?

heejaechang · 2017-07-05T19:25:33Z

ping?

jcouv · 2017-07-05T19:31:41Z

public partial class IOperationTests : SemanticModelTestBase

There should probably be a Trait or CompilerFeature to tag all the IOperations tests.
[This can wait til another PR]

Refers to: src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IUnaryOperatorExpression.cs:9 in dd17e7b. [](commit_id = dd17e7b, deletion_comment = False)

jcouv · 2017-07-05T19:32:14Z

    [Fact]

Nit: double empty lines (here and below).

Refers to: src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IUnaryOperatorExpression.cs:34 in dd17e7b. [](commit_id = dd17e7b, deletion_comment = False)

heejaechang · 2017-07-05T19:34:35Z

tag #20652

jcouv · 2017-07-05T19:35:31Z

IConversionExpression (ConversionKind.CSharp, Implicit) (OperationKind.ConversionExpression, Type: System.Int32) (Syntax: 'i')

@roslyn-compiler Is this conversion expected? I didn't understand why it is there.

Refers to: src/Compilers/CSharp/Test/Semantic/IOperation/IOperationTests_IUnaryOperatorExpression.cs:386 in dd17e7b. [](commit_id = dd17e7b, deletion_comment = False)

jcouv

LGTM

heejaechang · 2017-07-05T20:42:41Z

@dotnet/roslyn-infrastructure windows_debug_vs-integration_prtest failed with no failed test

https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/windows_debug_vs-integration_prtest/154/

heejaechang · 2017-07-05T20:42:53Z

retest windows_debug_vs-integration_prtest please

cston · 2017-07-05T21:04:02Z

The two recent failures of windows_debug_vs-integration_prtest are known issues: #20567 and #20562.

heejaechang · 2017-07-05T21:18:48Z

integration test failure is known issue

dnfclas added the cla-already-signed label Mar 23, 2017

heejaechang commented Mar 24, 2017

View reviewed changes

mavasani approved these changes Mar 24, 2017

View reviewed changes

heejaechang mentioned this pull request Mar 24, 2017

IfStatement not having right Operations for conditions #18160

Closed

jinujoseph reviewed Mar 24, 2017

View reviewed changes

jinujoseph approved these changes Mar 24, 2017

View reviewed changes

AlekseyTs reviewed Mar 27, 2017

View reviewed changes

AlekseyTs reviewed Mar 28, 2017

View reviewed changes

Pilchie added the Area-Analyzers label Mar 29, 2017

jinujoseph mentioned this pull request May 31, 2017

IIncrementExpression exposes both UnaryOperatorKind and BinaryOperatorKind #18056

Closed

heejaechang changed the base branch from master to features/ioperation June 30, 2017 21:50

heejaechang changed the base branch from features/ioperation to master June 30, 2017 21:50

heejaechang force-pushed the unaryoperation branch from 3f1c235 to 962dd86 Compare June 30, 2017 23:01

added csharp unary operator expression test

602274b

added vb unary operator tests

heejaechang force-pushed the unaryoperation branch from 962dd86 to 602274b Compare June 30, 2017 23:10

heejaechang changed the base branch from master to features/ioperation June 30, 2017 23:11

heejaechang added 2 commits June 30, 2017 16:49

fixed failed tests

d17b2f1

added vb tests

c6679da

remove duplicated test

dd17e7b

heejaechang mentioned this pull request Jul 5, 2017

add tag for all IOperationTests #20652

Closed

jcouv approved these changes Jul 5, 2017

View reviewed changes

removed double empty line

1c79973

heejaechang merged commit 49a5693 into dotnet:features/ioperation Jul 5, 2017



		[Fact]
		public void Test_UnaryOperatorExpression_Type_Minus_System_SByte()

added unary operator expression tests #18136

added unary operator expression tests #18136

Conversation

heejaechang commented Mar 23, 2017

heejaechang commented Mar 23, 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

heejaechang commented Mar 24, 2017

mavasani left a 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 Mar 24, 2017

heejaechang commented Mar 24, 2017

heejaechang commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heejaechang commented Mar 27, 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

AlekseyTs Mar 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

Choose a reason for hiding this comment

AlekseyTs commented Mar 28, 2017

heejaechang commented Jul 3, 2017

heejaechang commented Jul 5, 2017

jcouv commented Jul 5, 2017

jcouv commented Jul 5, 2017

heejaechang commented Jul 5, 2017

jcouv commented Jul 5, 2017

jcouv left a comment

Choose a reason for hiding this comment

heejaechang commented Jul 5, 2017

heejaechang commented Jul 5, 2017

cston commented Jul 5, 2017

heejaechang commented Jul 5, 2017

AlekseyTs Mar 28, 2017 •

edited

Loading