Fix IArgument.InConversion/OutConversion #21445

genlu · 2017-08-11T07:46:40Z

Fix #18548. Changed the In/OutConversion from IConversionExpression to CommonConversion (added in #21040 )

    public interface IArgument : IOperation
    {
        /// <summary>
        /// Kind of argument.
        /// </summary>
        ArgumentKind ArgumentKind { get; }
        /// <summary>
        /// Parameter the argument matches.
        /// </summary>
        IParameterSymbol Parameter { get; }
        /// <summary>
        /// Value supplied for the argument.
        /// </summary>
        IOperation Value { get; }
        /// <summary>
        /// Information of the conversion applied to the argument value passing it into the target method. Applicable only to VB Reference arguments.
        /// </summary>
        CommonConversion InConversion { get; }
        /// <summary>
        /// Information of the conversion applied to the argument value after the invocation. Applicable only to VB Reference arguments.
        /// </summary>
        CommonConversion OutConversion { get; }
    }

genlu · 2017-08-11T19:10:51Z

I think this is mostly done. Please review @AlekseyTs @cston @dotnet/analyzer-ioperation

genlu · 2017-08-11T21:11:25Z

windows_debug_unit64_prtest failure is probably another instance of #14592:
https://ci.dot.net/job/dotnet_roslyn/job/features_ioperation/job/windows_debug_unit64_prtest/463/

cston · 2017-08-12T04:12:59Z

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

+
+namespace Microsoft.CodeAnalysis.CSharp
+{
+    internal abstract class BaseCSharpArgument : BaseArgument, IArgument


BaseArgument already implements IArgument.

cston · 2017-08-12T04:22:23Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicArgument.vb

+
+        Public Overrides ReadOnly Property InConversion As [Optional](Of CommonConversion)
+            Get
+                Return If(InConversionInternal = Nothing, New [Optional](Of CommonConversion)(), InConversionInternal.ToCommonConversion())


Can use Nothing instead of New [Optional](Of CommonConversion)(). Same comment for OutConversion below.

cston · 2017-08-12T07:43:11Z

src/Compilers/VisualBasic/Portable/VisualBasicExtensions.vb

+        ''' </summary>
+        ''' <param name="argument">The argument to get original info from.</param>
+        ''' <returns>The underlying <see cref="Conversion"/> of the InConversion.</returns>
+        ''' <exception cref="InvalidCastException">If the <see cref="IArgument"/> was not created from Visual Basic code.</exception>


These methods throw ArgumentException rather than InvalidCastException.

AlekseyTs · 2017-08-14T21:34:49Z

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

        /// </summary>
-        IOperation InConversion { get; }
+        Optional<CommonConversion> InConversion { get; }


Optional [](start = 8, length = 26)

It doesn't look like it is a common pattern to use Optional<T> in IOperation API. Would it be better to use identity conversion for situations when no conversion is needed? #Closed

AlekseyTs · 2017-08-14T22:02:03Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicArgument.vb

+
+        Public Overrides ReadOnly Property InConversion As [Optional](Of CommonConversion)
+            Get
+                Return If(InConversionInternal = Nothing, New [Optional](Of CommonConversion)(), InConversionInternal.ToCommonConversion())


InConversionInternal = Nothing [](start = 26, length = 30)

Conversion is a structure. What does it mean to compare a structure with Nothing and why is it the right thing to do here? #Closed

AlekseyTs · 2017-08-14T23:16:23Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicArgument.vb

+
+        Public Overrides ReadOnly Property OutConversion As [Optional](Of CommonConversion)
+            Get
+                Return If(OutConversionInternal = Nothing, New [Optional](Of CommonConversion)(), OutConversionInternal.ToCommonConversion())


OutConversionInternal = Nothing [](start = 26, length = 31)

Same issue around Nothing. #Closed

AlekseyTs · 2017-08-14T23:24:54Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

+            If expression.Kind = BoundKind.Conversion Then
+                Dim conversion = DirectCast(expression, BoundConversion)
+                Dim method As MethodSymbol = Nothing
+                If conversion.Operand.Kind = BoundKind.UserDefinedConversion Then


conversion.Operand.Kind = BoundKind.UserDefinedConversion [](start = 19, length = 57)

It feels like we should also make sure that the conversion kind for the conversion node is user-defined conversion. #Closed

AlekseyTs · 2017-08-14T23:35:15Z

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

@@ -51,9 +51,7 @@ static void Test2(int y, params int[] x)
      IArgument (ArgumentKind.Explicit, Matching Parameter: x) (OperationKind.Argument) (Syntax: 'null')
        IConversionExpression (Implicit, TryCast: False, Unchecked) (OperationKind.ConversionExpression, Type: System.Int32[], Constant: null) (Syntax: 'null')
          Conversion: CommonConversion (Exists: True, IsIdentity: False, IsNumeric: False, IsReference: True, IsUserDefined: False) (MethodSymbol: null)
-          Operand: ILiteralExpression (Text: null) (OperationKind.LiteralExpression, Type: null, Constant: null) (Syntax: 'null')
-        InConversion: null
-        OutConversion: null");


It feels like these properties should still be validated. #Closed

AlekseyTs · 2017-08-14T23:41:53Z

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

@@ -835,5 +758,110 @@ BC30272: 'y' is not a parameter of 'Public Sub M2(x As Integer)'.
            VerifyOperationTreeAndDiagnosticsForTest(Of InvocationExpressionSyntax)(source, expectedOperationTree, expectedDiagnostics)
        End Sub

+
+
+        <CompilerTrait(CompilerFeature.IOperation)>


<CompilerTrait(CompilerFeature.IOperation)> [](start = 8, length = 43)

It looks like there are three new lines between this test and the one above. We usually use one line to separate tests. #Closed

AlekseyTs · 2017-08-14T23:47:51Z

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

+
+        <CompilerTrait(CompilerFeature.IOperation)>
+        <Fact>
+        Public Sub InOutConversionUserDefined()


InOutConversionUserDefined [](start = 19, length = 26)

It would be good to have tests for scenarios when standard conversions are involved into user-defined conversion, either for the input value, or for the method's return value. The cases when either BoundUserDefinedConversion.InConversionOpt, or BoundUserDefinedConversion.OutConversionOpt are not null. #Closed

I have added a test in the last commit. Is that what you suggested?

AlekseyTs · 2017-08-14T23:53:14Z

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

@@ -639,9 +639,23 @@ public override void VisitArgument(IArgument operation)
            LogString(")");
            LogCommonPropertiesAndNewLine(operation);

+            if (operation.InConversion.HasValue)


if (operation.InConversion.HasValue) [](start = 12, length = 36)

I think for the purpose of testing it is better to dump all properties regardless of their values. Usually people don't think about things they do not see. An absence of a value should be verified against expectations as well. #Closed

AlekseyTs · 2017-08-14T23:54:42Z

Done with review pass. #Closed

…onversion

genlu · 2017-09-05T17:41:35Z

Ping... @AlekseyTs @cston

genlu · 2017-09-07T00:34:07Z

@AlekseyTs Added tests for extension methods

AlekseyTs · 2017-09-07T17:15:09Z

@genlu It looks like there are some legitimate test failures.

AlekseyTs · 2017-09-07T17:24:00Z

src/Compilers/Test/Utilities/VisualBasic/BasicTestBase.vb

+            Return (Nothing, Nothing)
+        End If
+
+        Dim tree = (From t In compilation.SyntaxTrees Where t.FilePath = fileName).Single()


Dim tree = (From t In compilation.SyntaxTrees Where t.FilePath = fileName).Single() [](start = 8, length = 83)

Why so complex? Would node.SyntaxTree work?

AlekseyTs · 2017-09-07T17:31:10Z

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

+            Dim semanticModel = compilation.GetSemanticModel(compilation.SyntaxTrees(0))
+
+            Dim expectedInKind = ConversionKind.Widening Or ConversionKind.UserDefined
+            Dim exptectedInMethod = CType(semanticModel.GetDeclaredSymbolFromSyntaxNode(operatorNodes(0)), IMethodSymbol)


CType(semanticModel.GetDeclaredSymbolFromSyntaxNode(operatorNodes(0)), IMethodSymbol) [](start = 36, length = 85)

This looks more complicated than it should be. There is no need to use semantic model to find methods, I believe that could be done by name.

AlekseyTs · 2017-09-07T17:34:54Z

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

+
+        <CompilerTrait(CompilerFeature.IOperation)>
+        <Fact>
+        Public Sub GettingInOutConverionFromVBArgument()


GettingInOutConverionFromVBArgument [](start = 19, length = 35)

There is probably a value in testing clone operation, consider keeping the TestCloneInOutConversion unit-test from iteration 21.

AlekseyTs · 2017-09-07T17:35:56Z

Done with review pass (iteration 22). The only blocking issue is the failing test.

…onversion

genlu · 2017-09-07T21:54:46Z

@dotnet/roslyn-infrastructure It seems the tests are not triggered by new commits. Any ideas?

AlekseyTs · 2017-09-07T21:58:25Z

It seems the tests are not triggered by new commits. Any ideas?

Looking into this.

genlu · 2017-09-07T21:59:56Z

@cston Can I get another review from compiler team please?

cston · 2017-09-07T22:30:55Z

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

+
+        [CompilerTrait(CompilerFeature.IOperation)]
+        [Fact]
+        public void GettingInOutConverionFromCSharpArgumentShouldThrowException()


Typo: Conversion

cston · 2017-09-07T22:37:12Z

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

+
+            Assert.Throws<ArgumentException>(() => argument.GetInConversion());
+            Assert.Throws<ArgumentException>(() => argument.GetOutConversion());
+        }


Where are GetInConversion and GetOutConversion defined? And why do they throw ArgumentException in this case?

Those are extension methods for VB only (defined in MS.CA.VB.dll), as it returns VB Conversion. In this test we pass in an IArgument from C# code, therefore it throws.

Thanks. Perhaps add a comment here that we're calling VB methods so it's clear why we expect exceptions to be thrown.

cston · 2017-09-07T22:39:35Z

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

+        public override CommonConversion OutConversion => new CommonConversion(exists: true, isIdentity: true, isNumeric: false, isReference: false, methodSymbol: null);
+    }
+
+    internal sealed partial class CSharpArgument : BaseCSharpArgument


Do these CSharpArgument classes need to be partial?

cston · 2017-09-07T22:41:41Z

src/Compilers/Test/Utilities/VisualBasic/BasicTestBase.vb

+            Return (operation, node)
+        Else
+            Return (Nothing, Nothing)
+        End If


Is If operation IsNot Nothing necessary or could this method Return (operation, node) always?

cston · 2017-09-07T22:49:58Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

-                            inConversion:=Nothing,
-                            outConversion:=Nothing,
+                            inConversion:=New Conversion(Conversions.Identity),
+                            outConversion:=New Conversion(Conversions.Identity),
                            semanticModel:=_semanticModel,
                            syntax:=value.Syntax,
                            type:=Nothing,


It looks like the only difference between If and Else is kind. Consider calculating kind inside the If and move the remainder outside the If.

cston · 2017-09-08T04:40:43Z

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

@@ -2245,6 +2245,7 @@ static void M2(int x)
            var invocation = (IInvocationExpression)operation;
            var argument = invocation.ArgumentsInEvaluationOrder[0];

+            // We are calling VB extension methods on IArgument in C# code, therefore exception is expteced here.


Typo: expected

cston · 2017-09-08T04:41:38Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

+                    Return New VisualBasicArgument(
+                        kind,
+                        parameter,
+                        Create(argument),


cston · 2017-09-08T04:42:16Z

src/Compilers/VisualBasic/Portable/Operations/VisualBasicOperationFactory_Methods.vb

@@ -105,47 +105,37 @@ Namespace Microsoft.CodeAnalysis.Semantics
                        isImplicit:=isImplicit)
                Case Else
                    Dim lastParameterIndex = parameters.Length - 1
+                    Dim kind As ArgumentKind
+                    Dim parameter As ParameterSymbol


parameter = parameters(index) in both branches.

genlu added 3 commits August 10, 2017 19:09

Make InConversion and OutConversion CommonConversion

85ec530

Make InConversion and OutConversion optional

b3530c2

Fix tests

7840474

dnfclas added the cla-already-signed label Aug 11, 2017

genlu and others added 3 commits August 11, 2017 00:47

Change doc comment

7ac6b90

Unskip an IArgument test

eae54b9

Merge branch 'features/ioperation' into inoutconversion

1e67a4e

333fred approved these changes Aug 11, 2017

View reviewed changes

cston reviewed Aug 12, 2017

View reviewed changes

AlekseyTs reviewed Aug 14, 2017

View reviewed changes

333fred mentioned this pull request Aug 18, 2017

Fix VB IConversionExpression bug. #21462

Merged

genlu added 3 commits August 25, 2017 17:00

Address review comments

f2ce4b3

Merge remote-tracking branch 'dotnet/features/ioperation' into inoutc…

ac18ce4

…onversion

Fix tests

4fe6a81

333fred mentioned this pull request Aug 29, 2017

Update VisualBasicOperationFactory.CreateBoundConversion to use GetConversion #21797

Closed

genlu added 3 commits August 29, 2017 15:00

Make In/OutConversion not Optional

17f870a

Merge remote-tracking branch 'dotnet/features/ioperation' into inoutc…

95dbb15

…onversion

Add a test for In/OutConversion

ef2ef97

Add tests for In/OutConversion extension methods

4ab6b89

AlekseyTs reviewed Sep 7, 2017

View reviewed changes

genlu force-pushed the inoutconversion branch from 554cb12 to a6c0b36 Compare September 7, 2017 21:49

Merge remote-tracking branch 'dotnet/features/ioperation' into inoutc…

dbc57e3

…onversion

genlu force-pushed the inoutconversion branch from a6c0b36 to dbc57e3 Compare September 7, 2017 21:51

genlu closed this Sep 7, 2017

genlu reopened this Sep 7, 2017

cston reviewed Sep 7, 2017

View reviewed changes

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

cston reviewed Sep 7, 2017

View reviewed changes

Address review comments

bc6524f

cston reviewed Sep 8, 2017

View reviewed changes

Pilchie added the Area-Analyzers label Sep 8, 2017

Address review comments

ae94e42

cston approved these changes Sep 9, 2017

View reviewed changes

genlu merged commit b3cacb0 into dotnet:features/ioperation Sep 12, 2017

Fix IArgument.InConversion/OutConversion #21445

Fix IArgument.InConversion/OutConversion #21445

Conversation

genlu commented Aug 11, 2017 • edited Loading

genlu commented Aug 11, 2017

genlu commented Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Aug 14, 2017 • edited Loading

genlu commented Sep 5, 2017

genlu commented Sep 7, 2017

AlekseyTs commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlekseyTs commented Sep 7, 2017

genlu commented Sep 7, 2017

AlekseyTs commented Sep 7, 2017

genlu commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cston Sep 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

genlu commented Aug 11, 2017 •

edited

Loading

genlu commented Aug 11, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs Aug 14, 2017 •

edited

Loading

AlekseyTs commented Aug 14, 2017 •

edited

Loading

cston Sep 7, 2017 •

edited

Loading