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

heejaechang · 2017-09-22T21:03:01Z

main issue the PR is trying to fix (#18405) is addressed by this (#21857).

this mainly port over tests added by this pr (#21263) with some changes.

in some cases such as params array, argument node get removed while converted to IOperation since compiler generated array replaces the argument.

for those, I just made test to dump including statement. for now, the test doesn't show interesting info, but once @jinujoseph finishes the work to show syntax kind/isImplicit/parent in the test, it will show info such as what got injected (param array) and argument got removed.

heejaechang · 2017-09-22T21:03:19Z

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

mavasani · 2017-09-22T21:14:00Z

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

+}
+";
+            string expectedOperationTree = @"
+IArgument (ArgumentKind.Explicit, Matching Parameter: i) (OperationKind.Argument) (Syntax: 'ref i')


Just realized that we don't expose the RefKind of the argument on IArgument node. Should we do so? Can you please file a tracking bug? #Resolved

sure - #22297 #Resolved

mavasani · 2017-09-22T21:16:15Z

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

+  Instance Receiver: IInstanceReferenceExpression (OperationKind.InstanceReferenceExpression, Type: P) (Syntax: 'this')
+  Arguments(1):
+      IArgument (ArgumentKind.ParamArray, Matching Parameter: array) (OperationKind.Argument) (Syntax: 'this[1]')
+        IArrayCreationExpression (Element Type: System.Int32) (OperationKind.ArrayCreationExpression, Type: System.Int32[]) (Syntax: 'this[1]')


This is the synthesized one correct? It still looks like a bug that the associated syntax is this[1] and not 1. Can you file an issue to track this?

1 seems as weird as this[1] for me :) not sure what is right node to put here though :)

opened -#22298

mavasani · 2017-09-22T21:19:02Z

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

+IObjectCreationExpression (Constructor: Sub Program..ctor(ParamArray a As System.Int32())) (OperationKind.ObjectCreationExpression, Type: Program) (Syntax: 'New Program(1)')
+  Arguments(1):
+      IArgument (ArgumentKind.ParamArray, Matching Parameter: a) (OperationKind.Argument) (Syntax: 'Program')
+        IArrayCreationExpression (Element Type: System.Int32) (OperationKind.ArrayCreationExpression, Type: System.Int32()) (Syntax: 'Program')


Same here, associated syntax seems weird.

yep. the pattern looks like assigning node above itself regardless what that is.

jinujoseph · 2017-09-22T21:28:31Z

@dotnet/roslyn-compiler

heejaechang · 2017-09-22T21:31:01Z

jenkins issue - CSC : error CS0016: Could not write to output file 'D:\j\workspace\windows_deter---bd1d49d3\Binaries\Obj\TestUtilities\Debug\Roslyn.Test.Utilities.xml' -- 'The process cannot access the file 'D:\j\workspace\windows_deter---bd1d49d3\Binaries\Obj\TestUtilities\Debug\Roslyn.Test.Utilities.xml' because it is being used by another process.' [D:\j\workspace\windows_deter---bd1d49d3\src\Test\Utilities\Portable\TestUtilities.csproj]

heejaechang · 2017-09-22T21:31:13Z

retest windows_determinism_prtest please

jcouv · 2017-09-22T21:34:37Z

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

+        int i = 0;
+        M2(/*<bind>*/ref i/*</bind>*/);
+    }
+    static void M2(ref int i) { }


Consider adding a similar test for "ref readonly" parameter.

int i = 0; ref int refI = ref i; M2(refI);

FYI @VSadov #Resolved

sure. #Resolved

@jcouv what feature option do I need to turn on to make ref readonly to work? #Resolved

LangVersion.CSharp7_2 on latest master bits.

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

Sorry, had made a mistake above. No "ref readonly" at callsite. #Resolved

I added test but not sure what kind of info we are looking for from IArgument. the difference seems all in IParameter not on callsite.

We may need to a follow-up on that.

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

@mavasani @jinujoseph are we going to support the "ref readonly" feature in IOperation v1?

jcouv · 2017-09-22T21:36:13Z

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

+}
+";
+            string expectedOperationTree = @"
+IArgument (ArgumentKind.Explicit, Matching Parameter: i) (OperationKind.Argument) (Syntax: 'ref i')


That's pretty useful. I was just wondering about this a couple of days ago, do we have public API to show how arguments and parameters were matched... #Resolved

tagging @genlu who did argument work. #Resolved

jcouv · 2017-09-22T21:57:05Z

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

+{
+    static void M1()
+    {
+        /*<bind>*/M2(0, 1);/*</bind>*/


Maybe that is tested elsewhere: what happens if the "bind" tags are just around "0" or "1" in this scenario? #Resolved

depends on whether it is literal expression or argument syntax node given. if it is literal expression, then it will return ILiteralExpression. if it is argument syntax, then it will return nothing since that node doesn't exist in IOperation tree. #Resolved

this is what I meant in the description.

"for now, the test doesn't show interesting info, but once @jinujoseph finishes the work to show syntax kind/isImplicit/parent in the test, it will show info such as what got injected (param array) and argument got removed."

jcouv · 2017-09-22T22:05:49Z

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

+            VerifyOperationTreeAndDiagnosticsForTest<ElementAccessExpressionSyntax>(source, expectedOperationTree, expectedDiagnostics);
+        }
+
+        [Fact]


I don't know how attributes work with IOperation, but something like [MyAttribute(0, 1)] would be useful to test.
Attributes (in C#) can also initialize properties: [MyAttribute(arg1: 1, arg2: 2, Property1=1, Property2=2)].

added. looks like we don't support bound attribute fully yet.

jcouv · 2017-09-22T22:07:15Z

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

+            var expectedDiagnostics = DiagnosticDescription.None;
+
+            VerifyOperationTreeAndDiagnosticsForTest<ElementAccessExpressionSyntax>(source, expectedOperationTree, expectedDiagnostics);
+        }


Another scenario that may be worth testing is non-trailing named arguments: M(a: 1, 2) (C# 7.2). This should behave the same as if the argument name wasn't there.

jcouv · 2017-09-22T22:12:45Z

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

@@ -1074,5 +1074,269 @@ End Class]]>.Value

            VerifyClone(model)
        End Sub
+
+        <Fact>


If I remember correctly, VB not only has named, un-named and omitted arguments, it also has "range" arguments. I'm not sure the syntax.

added. but the range argument is added implicit operation so it won't return by GetOperation(range argument)

jcouv · 2017-09-22T23:18:59Z

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

+        public void DirectlyBindArgument_Attribute()
+        {
+            string source = @"
+[assembly: /*<bind>*/System.CLSCompliant(isCompliant: true)/*</bind>*/]


Is there a tracking issue for attributes?

tracked by this - #18198

jcouv

LGTM

heejaechang · 2017-09-22T23:34:18Z

do I need one more sign off for test only change from compiler team?

jinujoseph · 2017-09-22T23:36:20Z

No, test only changes 1 sign off needed.

heejaechang · 2017-09-23T05:07:07Z

@dotnet/roslyn-infrastructure test failed - Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicLineCommit.CommitOnSave

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

heejaechang · 2017-09-23T05:07:21Z

retest windows_release_vs-integration_prtest please

* dotnet/master: (351 commits) Avoid scheduling an unnecessary dispose task for EmptyAsyncToken Support for ref-readonly locals (dotnet#22269) ported tests from dotnet#21263 (dotnet#22296) NormalizeWhitespace should not add a space in nullable type (dotnet#22234) Fix the work item number associated with the unit test Add unit test Terminate a sentence Add instructions for upgrading PowerShell Unsafe local should require /unsafe flag on compilation (dotnet#22268) CR feedback Not poisoning restricted types known to be stack only. Typo Error messsage should mention C# 7.0 not C# 7 (dotnet#22255) Package descriptions for CSharp and VB should include summary blurb (dotnet#22166) Fix a trivial issue in how IsLifted is calculated for C# compound assignment operator Do not zero out "With" statement target expression locals referenced within a lambda. (dotnet#22223) Fix unit test failure from merge Mark "private protected", "ref readonly" and other C# 7.2 features as merged (dotnet#22209) Add unit tests for IAwaitExpression and make the API public again more test fixes due to merge conflicts ...

* dotnet/master: (309 commits) Avoid scheduling an unnecessary dispose task for EmptyAsyncToken Support for ref-readonly locals (dotnet#22269) ported tests from dotnet#21263 (dotnet#22296) NormalizeWhitespace should not add a space in nullable type (dotnet#22234) Fix the work item number associated with the unit test Add unit test Terminate a sentence Add instructions for upgrading PowerShell Unsafe local should require /unsafe flag on compilation (dotnet#22268) CR feedback Not poisoning restricted types known to be stack only. Typo Error messsage should mention C# 7.0 not C# 7 (dotnet#22255) Package descriptions for CSharp and VB should include summary blurb (dotnet#22166) Fix a trivial issue in how IsLifted is calculated for C# compound assignment operator Do not zero out "With" statement target expression locals referenced within a lambda. (dotnet#22223) Fix unit test failure from merge Mark "private protected", "ref readonly" and other C# 7.2 features as merged (dotnet#22209) Add unit tests for IAwaitExpression and make the API public again more test fixes due to merge conflicts ...

ported tests from dotnet#21263

b63e504

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

mavasani reviewed Sep 22, 2017

View reviewed changes

mavasani approved these changes Sep 22, 2017

View reviewed changes

heejaechang mentioned this pull request Sep 22, 2017

IArgument doesn't expose info on out or ref #22297

Closed

jinujoseph added Test Test failures in roslyn-CI Area-Compilers Feature - IOperation IOperation labels Sep 22, 2017

heejaechang mentioned this pull request Sep 22, 2017

put better syntax node for implicitly created param array #22298

Closed

jinujoseph approved these changes Sep 22, 2017

View reviewed changes

jcouv reviewed Sep 22, 2017

View reviewed changes

added ref readonly test

6cb57d7

jcouv reviewed Sep 22, 2017

View reviewed changes

added more tests following PR suggestion

8e263ff

jcouv reviewed Sep 22, 2017

View reviewed changes

jcouv approved these changes Sep 22, 2017

View reviewed changes

heejaechang merged commit 917edc1 into dotnet:master Sep 23, 2017

jcouv added the Approved to merge label Sep 29, 2017

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

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

Conversation

heejaechang commented Sep 22, 2017

heejaechang commented Sep 22, 2017

mavasani Sep 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

heejaechang Sep 22, 2017 • edited by jcouv 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

jinujoseph commented Sep 22, 2017

heejaechang commented Sep 22, 2017

heejaechang commented Sep 22, 2017

jcouv Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

heejaechang Sep 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Sep 22, 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

jcouv Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 22, 2017 • edited by jcouv Loading

Choose a reason for hiding this comment

jcouv Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

heejaechang Sep 22, 2017 • edited by jcouv 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

heejaechang Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

heejaechang commented Sep 22, 2017

jinujoseph commented Sep 22, 2017

heejaechang commented Sep 23, 2017

heejaechang commented Sep 23, 2017

mavasani Sep 22, 2017 •

edited by jcouv

Loading

heejaechang Sep 22, 2017 •

edited by jcouv

Loading

jcouv Sep 22, 2017 •

edited

Loading

heejaechang Sep 22, 2017 •

edited by jcouv

Loading

heejaechang Sep 22, 2017 •

edited by jcouv

Loading

jcouv Sep 22, 2017 •

edited

Loading

jcouv Sep 22, 2017 •

edited

Loading

heejaechang Sep 22, 2017 •

edited by jcouv

Loading

jcouv Sep 22, 2017 •

edited

Loading

heejaechang Sep 22, 2017 •

edited by jcouv

Loading

heejaechang Sep 22, 2017 •

edited

Loading