IInvocationExpression and IArgument refinement and testing #11450

msJohnHamby · 2016-05-20T06:20:07Z

This change set removes syntactic concerns from IInvocationExpression and IArgument. Specifically, the ArgumentsInSourceOrder property of IInvocationExpression has been replaced with ArgumentsInEvaluationOrder, and the ArgumentKind property of IArgument has been removed.

Those interested only in the API surface are can confine their attention to changes to the IExpression.cs file.

This change set also includes new tests to cover the surface areas of these interfaces for a variety of circumstances, along with a set of bug fixes to make the tests pass.

This change set does not includes fixes for all previously-reported issues related to invocations. For example, there are still some problems with matching array arguments to paramarray parameters, and the reported issues remain open for the time being.

@dotnet/roslyn-compiler @dotnet/roslyn-analysis @AlekseyTs @jcouv @genlu @jaredpar @DustinCampbell @nguerrera @srivatsn @gafter @mattwar @CyrusNajmabadi @mavasani @ManishJayaswal and anyone else eager to read 1600 lines of new tests are requested to review.

…luationOrder.

nguerrera · 2016-05-20T10:32:43Z

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

        /// </summary>
-        ImmutableArray<IArgument> ArgumentsInSourceOrder { get; }
+        ImmutableArray<IArgument> ArgumentsInEvaluationOrder { get; }


Why isn't this on IHasArgumentsExpression?

It should be.

nguerrera · 2016-05-20T10:39:37Z

API change looks good. (I didn't review the tests.)

Now that you have the hood up on argument handling, it might be a good time to look into vararg (__arglist) handling. See #8394.

…gumentsExpression.

msJohnHamby · 2016-05-20T20:01:28Z

I have pushed a commit that moves ArgumentsInEvaluationOrder from IInvocationExpresion to IHasArgumentsExpression. Tests for the additional cases in other interfaces derived from IHasArgumentsExpression will be added later in the test wave when those interfaces receive exhaustive testing.

msJohnHamby · 2016-05-20T20:03:22Z

retest windows_debug_unit32_prtest please

msJohnHamby · 2016-05-20T20:03:51Z

retest windows_debug_unit64_prtest please

msJohnHamby · 2016-05-20T22:15:45Z

retest windows_debug_unit32_prtest please
retest windows_debug_unit64_prtest please
retest windows_eta_open_prtest please

msJohnHamby · 2016-05-20T22:40:53Z

Adding @VSadov @cston in hopes of attracting compiler team members.

genlu · 2016-05-20T23:30:10Z

    private abstract class ArgumentBase : IArgument

I think ArgumentBase is not used anywhere else, maybe merge Argument and ArgumentBase into one concrete type instead.

Refers to: src/Compilers/CSharp/Portable/BoundTree/Expression.cs:240 in 4e5617e. [](commit_id = 4e5617e, deletion_comment = False)

msJohnHamby · 2016-05-20T23:55:10Z

I've pushed a new commit that removes the ArgumentBase class.

genlu · 2016-05-20T23:55:13Z

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

@@ -83,7 +67,7 @@ public override void Accept(OperationVisitor visitor)
            return visitor.VisitInvocationExpression(this, argument);
        }

-        internal static ImmutableArray<IArgument> DeriveArguments(ImmutableArray<BoundExpression> boundArguments, ImmutableArray<string> argumentNames, ImmutableArray<int> argumentsToParameters, ImmutableArray<RefKind> argumentRefKinds, ImmutableArray<Symbols.ParameterSymbol> parameters, SyntaxNode invocationSyntax)
+        internal static ImmutableArray<IArgument> DeriveArgumentsInParameterOrder(ImmutableArray<BoundExpression> boundArguments, ImmutableArray<string> argumentNames, ImmutableArray<int> argumentsToParameters, ImmutableArray<RefKind> argumentRefKinds, ImmutableArray<Symbols.ParameterSymbol> parameters, SyntaxNode invocationSyntax)


Consider moving DeriveArgumentsInParameterOrder and DeriveArgumentsInEvaluationOrder both to Expression class at the end of this file, since they are called by indexer and object creation as well.

genlu · 2016-05-21T07:14:18Z

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

+    {
+        int x = 10;
+        int y;
+        F(ref x, out y);


It seems this is also valid:

F(yy: out y, xx: ref x);

Added tests for named out and reference arguments.

AlekseyTs · 2016-05-23T04:20:26Z

                ' Apparently the VB bound trees don't encode named arguments, which seems unnecesarily lossy.

Consider removing this comment as argument names in VB do not affect the order of evaluation. Evaluation is performed in parameter declaration order.

Refers to: src/Compilers/VisualBasic/Portable/BoundTree/Expression.vb:375 in 4a94d83. [](commit_id = 4a94d83, deletion_comment = False)

AlekseyTs · 2016-05-23T04:25:14Z

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

@@ -379,9 +377,9 @@ Namespace Microsoft.CodeAnalysis.VisualBasic
                        argument,
                        Function(argumentValue)
                            If index >= parameters.Length - 1 AndAlso parameters.Length > 0 AndAlso parameters(parameters.Length - 1).IsParamArray Then


I do not understand why this conditional logic is needed.

This logic probably isn't needed. It's left over from before I discovered that the VB bound trees already include paramarray matching. I deleted the If statement and no tests failed.

msJohnHamby · 2016-05-23T20:01:48Z

I will take care of fixing up TestOperationWalker.

msJohnHamby · 2016-05-24T22:58:55Z

The commit just pushed is for purposes of backing up work in progress. The change set is not yet ready for further review.

…rationRescue Conflicts: src/Compilers/CSharp/Portable/BoundTree/Expression.cs

CyrusNajmabadi · 2016-06-03T05:06:55Z

Why was ArgumentKind removed? It seems super useful.

msJohnHamby · 2016-06-03T05:23:26Z

ArgumentKind is removed because:

It's a syntactic concept, not a semantic concept.
The VB implementation would have been difficult to make correct, because the VB compiler does not presently preserve information like named arguments and paramarray arguments in the bound trees. Rummaging through the syntax trees to try to reconstruct the original form wasn't appealing.

CyrusNajmabadi · 2016-06-03T05:33:17Z

It's a syntactic concept, not a semantic concept.

This is unfortunate. Right now it's a bit hard for me to tell what the design around IOperation should be. Right now it seems very oriented at exposing bound nodes. And that's def a good thing. However, at the same time it serves as a great way of providing a language neutral way of viewing C# and VB code.

For many IDE features we'd like to use IOperation as a mechanism to allow us to write features without having to have duplicated logic for VB and C# to perform the same syntactic check. IOperation has provided us with that neutral introspection API. For example, PopulateSwitch is written without any C# of VB specific code at all: https://github.com/dotnet/roslyn/blob/master/src/Features/Core/Portable/PopulateSwitch/PopulateSwitchCodeFixProvider.cs

These two designs end up causing tension though. There are things that VB and C# syntactically share which IOperation still does not necessarily represent. While i see the benefit of just exposing the BoundNode data, i would still very much like it if we can push things further and allow this to be the language neutral way of operating over VB and C#.

So, for something like IArgument, having hte ArgumentKind is very nice. It's true that it's a syntactic concept. But, at the same time, it's common to C# and VB and otherwise requires duplicated code in a feature to ask the exact same for each language.

msJohnHamby · 2016-06-03T05:46:34Z

Wow. I love the PopulateSwitch example, and would very much like to make more things like this possible.

Certainly I think ArgumentKind would be useful, else it wouldn't have been part of the original concept. If VB made supporting it easy I probably wouldn't be pushing to remove it.

Having a part of the API that is not supported correctly in VB is not tenable, and getting the VB implementation correct would require time that I don't have. (Anyone else want to take a whack at it?) The whole IOperation feature is under severe time pressure to meet correctness requirements. We can restore ArgumentKind to the API when there is time to implement it correctly in all supported languages.

CyrusNajmabadi · 2016-06-03T05:48:03Z

The whole IOperation feature is under severe time pressure to meet correctness requirements.

Is it an option to spend more time on this, and release as a more stable API later? I'd rather spend more up front time on this than rush something through.

…the logic from LocalRewriter for params arguments and default arguments.

msJohnHamby · 2016-06-10T06:49:44Z

The commit I just pushed will cause this PR to fail to build--I pushed the commit to save some work in progress.

msJohnHamby added 17 commits April 20, 2016 16:11

Beginning work on serious IOperation testing.

95eed9f

More testing.

f09e703

More testing.

22f4439

Progress on testing IInvocationExpression and IArgument.

f8fcd33

Rename IInvocationExpression.ArgumentsInSourceOrder to ArgumentsInEva…

8be60e4

…luationOrder.

Renamed a test class.

8b915a8

Finished renaming the test class.

ed039e1

Added a delegate invocation test.

e089f4e

More.

c15bc77

Merge remote-tracking branch 'upstream/master' into IOperationRescue

da75981

More

b6e3d29

Merge remote-tracking branch 'upstream/master' into IOperationRescue

dcb51b2

Removed ArgumentKind from IArgument.

4f358d6

Progress on VB tests.

6843108

Made virtual invocations test work for VB.

297466f

Added more VB tests.

a340980

Viable unit of C# and VB tests.

f0805f9

dnfclas added the cla-already-signed label May 20, 2016

nguerrera reviewed May 20, 2016
View reviewed changes

Moved ArgumentsInEvaluationOrder from IInvocationExpression to IHasAr…

4e5617e

…gumentsExpression.

Removed ArgumentBase class.

4a94d83

genlu reviewed May 20, 2016
View reviewed changes

genlu reviewed May 21, 2016
View reviewed changes

AlekseyTs reviewed May 23, 2016
View reviewed changes

Partial responses to review feedback. Not ready for further review.

dc65eaf

msJohnHamby added 2 commits May 25, 2016 13:14

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

375839d

…rationRescue Conflicts: src/Compilers/CSharp/Portable/BoundTree/Expression.cs

Employ helper methods to make tests smaller and easier to understand.

8a72012

msJohnHamby added 2 commits June 2, 2016 23:20

Review feedback now addressed, except for the issue of trying to use …

5e97607

…the logic from LocalRewriter for params arguments and default arguments.

Beginning work to leverate LocalRewriter in argument processing.

b1c39ce

Pilchie added the Area-Analyzers label Mar 29, 2017

jaredpar closed this Apr 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IInvocationExpression and IArgument refinement and testing #11450

IInvocationExpression and IArgument refinement and testing #11450

msJohnHamby commented May 20, 2016

nguerrera May 20, 2016

msJohnHamby May 20, 2016

nguerrera commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

genlu commented May 20, 2016

msJohnHamby commented May 20, 2016

genlu May 20, 2016 •

edited

Loading

genlu May 21, 2016

msJohnHamby Jun 2, 2016

AlekseyTs commented May 23, 2016 •

edited

Loading

AlekseyTs May 23, 2016

msJohnHamby Jun 3, 2016

msJohnHamby commented May 23, 2016

msJohnHamby commented May 24, 2016

CyrusNajmabadi commented Jun 3, 2016

msJohnHamby commented Jun 3, 2016

CyrusNajmabadi commented Jun 3, 2016

msJohnHamby commented Jun 3, 2016

CyrusNajmabadi commented Jun 3, 2016

msJohnHamby commented Jun 10, 2016

IInvocationExpression and IArgument refinement and testing #11450

IInvocationExpression and IArgument refinement and testing #11450

Conversation

msJohnHamby commented May 20, 2016

nguerrera May 20, 2016

Choose a reason for hiding this comment

msJohnHamby May 20, 2016

Choose a reason for hiding this comment

nguerrera commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

msJohnHamby commented May 20, 2016

genlu commented May 20, 2016

msJohnHamby commented May 20, 2016

genlu May 20, 2016 • edited Loading

Choose a reason for hiding this comment

genlu May 21, 2016

Choose a reason for hiding this comment

msJohnHamby Jun 2, 2016

Choose a reason for hiding this comment

AlekseyTs commented May 23, 2016 • edited Loading

AlekseyTs May 23, 2016

Choose a reason for hiding this comment

msJohnHamby Jun 3, 2016

Choose a reason for hiding this comment

msJohnHamby commented May 23, 2016

msJohnHamby commented May 24, 2016

CyrusNajmabadi commented Jun 3, 2016

msJohnHamby commented Jun 3, 2016

CyrusNajmabadi commented Jun 3, 2016

msJohnHamby commented Jun 3, 2016

CyrusNajmabadi commented Jun 3, 2016

msJohnHamby commented Jun 10, 2016

genlu May 20, 2016 •

edited

Loading

AlekseyTs commented May 23, 2016 •

edited

Loading