Conversion expression rewrite #21040

333fred · 2017-07-21T21:49:22Z

This is the interface changes required to implement #20046. This is just the changes to CodeAnalysis, CSharpCodeAnalysis, and VisualBasic. I'd like to get comments from @dotnet/analyzer-ioperation @CyrusNajmabadi @cston for this initial implementation. Changes from the initial proposal are as follows:

Added ThrowsExceptionOnFailure for C# as and VB TryCast distiction.
Removed the generics. They felt really bad. They've been replaced with
- A property on IConversionExpression that returns the IConversion. This will result in a box, which is unfortunate, but the resulting object is castable directly to either C#'s or VB's Conversion.
- Extension methods for VB and C# on IConversionExpression that allow the user to directly retrieve the underlying Conversion. These are direct access, and require no boxing.
Added IsReference to IConversion.

Please leave comments and criticisms on the initial implementation.

cston · 2017-07-25T18:16:09Z

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

+            else
+            {
+                throw new InvalidCastException(string.Format(CSharpResources.IConversionExpression_Is_Not_A_Valid_CSharp_Conversion, conversionExpression));
+            }


Perhaps just return ((BaseCSharpConversionExpression)conversionExpression).ConversionInternal; since that will also throw an InvalidCastException on failure.

Personally, I think the explicit error message here will provide a better use experience.

Then the exception should probably be something other than InvalidCastException since casting seems like an implementation detail. Perhaps ArgumentException.

CyrusNajmabadi · 2017-07-25T18:21:31Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5108,4 +5108,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_InvalidDebugInfo" xml:space="preserve">
    <value>Unable to read debug information of method '{0}' (token 0x{1:X8}) from assembly '{2}'</value>
  </data>
+  <data name="IConversionExpression_Is_Not_A_Valid_CSharp_Conversion" xml:space="preserve">


IConversionExpression_0_is_not_a_valid_CSharp_conversion_expression or IConversionExpression_0_is_not_a_valid_CSharp_conversion

Other exception message ids avoid underscores. Please use IConversionExpressionIsNotAValidCSharpConversion or something shorter.

Since this is in the compilers project and not IDE, I went with Compilers-style.

CyrusNajmabadi · 2017-07-25T18:21:38Z

src/Compilers/CSharp/Portable/CSharpResources.resx

@@ -5108,4 +5108,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
  <data name="ERR_InvalidDebugInfo" xml:space="preserve">
    <value>Unable to read debug information of method '{0}' (token 0x{1:X8}) from assembly '{2}'</value>
  </data>
+  <data name="IConversionExpression_Is_Not_A_Valid_CSharp_Conversion" xml:space="preserve">
+    <value>IConversionExpression '{0}' is not a valid C# conversion expression.</value>


No period at the end.

cston · 2017-07-25T18:22:43Z

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

        /// <summary>
        /// True if and only if the operation is performed by an operator method.
        /// </summary>
-        public bool UsesOperatorMethod { get; }
+        public bool UsesOperatorMethod => Conversion.IsUserDefined;


To avoid boxing, this should be an abstract property, implemented by the derived types. Same for OperatorMethod.

Perhaps this is a teaching moment. I would not have expected any boxing to occur here. Why would it, since everything in this chain is bool?

Also, IMethodSymbol will already be boxed, so I don't see any value in duplicating the code.

Perhaps this is a teaching moment. I would not have expected any boxing to occur here. Why would it, since everything in this chain is bool?

If "Conversion" returns an IConversion, then this will box (it's an interface, they're always on the heap). If you can access something of type TConversion, then it will be a constrained call through that interface property and it will not box if the TConversion is actaully instantiated at runtime to be a struct type.

Sure, but we're not talking about conversion in this comment, right? I'm seeing this on public bool UsesOperatorMethod => Conversion.IsUserDefined;, and I'm not understanding how this can box.

Ah, wait, I understand. Another argument for going with the common Conversion struct, I guess.

What does your "Conversion" property return?

CommonConversion?

CyrusNajmabadi · 2017-07-25T18:23:07Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -1 +1,3 @@
 Microsoft.CodeAnalysis.CSharp.LanguageVersion.CSharp7_2 = 702 -> Microsoft.CodeAnalysis.CSharp.LanguageVersion
+Microsoft.CodeAnalysis.CSharp.IConversionExpressionExtensions
+static Microsoft.CodeAnalysis.CSharp.IConversionExpressionExtensions.GetCSharpConversion(this Microsoft.CodeAnalysis.Semantics.IConversionExpression conversionExpression) -> Microsoft.CodeAnalysis.CSharp.Conversion


I think the name would suffice as GetConversion. We don't have ClassifyCSharpConversion and ClassifyVisualBasicConversion. We just ahve the extension ClassifyConversion at the particular C# and VB layers.

CyrusNajmabadi · 2017-07-25T18:23:42Z

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

        /// Value to be converted.
        /// </summary>
        public abstract IOperation Operand { get; }
+#pragma warning disable CS1574 // XML comment has cref attribute that could not be resolved


instead of cref, you can just do see="T:fullname"

CyrusNajmabadi · 2017-07-25T18:24:19Z

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

        /// <summary>
        /// True if and only if the conversion is indicated explicity by a cast operation in the source code.
        /// </summary>
-        public bool IsExplicit { get; }
+        public bool IsExplicitInCode { get; }


is this redundant with an existing boolean on IOperation? i.e. don't we have an IsImplicit boolean on IOperation we can use for this?

I don't think we do currently. We do have #10214 tracking something for exposing whether a node was compiler generated. @cston I think that would be enough to cover this case, can you think of any exceptions I'm missing?

cston · 2017-07-25T18:24:36Z

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

@@ -1294,36 +1294,59 @@ public LazyConditionalChoiceExpression(Lazy<IOperation> condition, Lazy<IOperati
    /// <summary>
    /// Represents a conversion operation.
    /// </summary>
-    internal abstract partial class BaseConversionExpression : Operation, IHasOperatorMethodExpression, IConversionExpression
+    internal abstract partial class BaseConversionExpression<TConversion> : Operation, IHasOperatorMethodExpression, IConversionExpression
+        where TConversion : struct, IConversion


Is this base class providing value or could the language-specific derived types simply implement the various interfaces?

It provides the visitor implementation that both languages use, plus any common properties that don't need to be reimplemented. I'd rather leave it.

CyrusNajmabadi · 2017-07-25T18:24:55Z

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

+        /// <summary>
+        /// True if the conversion will fail with an exception at runtime if the cast fails. This is false for C#'s <code>as</code>
+        /// operator and for VB's <code>TryCast</code> operator.
+        /// </summary>


verify that this is also false for things like an (int) cast on a long in unchecked code, but true in checked code.

Perhaps we need a different wording here, maybe will throw a <see cref="InvalidCastException" /> at runtime? Checked vs unchecked is tracked by #18055, and I'm going to work a separate IsChecked boolean into this PR.

Since checked and unchecked are a different set of exceptions.

CyrusNajmabadi · 2017-07-25T18:25:47Z

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

+        }
+
+        public override string LanguageName => LanguageNames.CSharp;
+        // We override this here so that we don't return the method symbol if the internal conversion is a MethodGroup conversion, instead of


why not push this logic down?

Maybe I'm missing something, but what benefit would we get by forcing both implementing classes to duplicate this functionality?

sorry, my down is synonymous with "up" :) Push this into the most base conversion expressoin type.

CyrusNajmabadi · 2017-07-25T18:26:58Z

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

+            End If
+        End Function
+    End Module
+


remove newline

CyrusNajmabadi · 2017-07-25T18:27:17Z

src/Compilers/VisualBasic/Portable/Semantics/Conversions.vb

@@ -11,6 +11,7 @@ Imports Microsoft.CodeAnalysis.VisualBasic.Symbols
 Imports Microsoft.CodeAnalysis.VisualBasic.Syntax
 Imports Microsoft.CodeAnalysis.VisualBasic.Symbols.TypeSymbolExtensions
 Imports TypeKind = Microsoft.CodeAnalysis.TypeKind
+Imports Microsoft.CodeAnalysis.Semantics


CyrusNajmabadi · 2017-07-25T18:27:38Z

src/Compilers/VisualBasic/Portable/VBResources.resx

@@ -5500,5 +5500,7 @@
  <data name="ERR_InvalidDebugInfo" xml:space="preserve">
    <value>Unable to read debug information of method '{0}' (token 0x{1}) from assembly '{2}'</value>
  </data>
-</root>
-
+  <data name="IConversionExpression_Is_Not_Visual_Basic_Conversion" xml:space="preserve">


match key to value.

CyrusNajmabadi · 2017-07-25T18:28:32Z

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

+        ''' <exception cref="InvalidCastException">If the <see cref="IConversionExpression"/> was not created from Visual Basic code.</exception>
+        <Extension>
+        Public Function GetVisualBasicConversion(conversionExpression As IConversionExpression) As Conversion
+            If TypeOf conversionExpression Is BaseVisualBasicConversionExpression Then


can just do a TryCast to avoid the two typechecks.

CyrusNajmabadi · 2017-07-25T18:28:40Z

Done with first pass.

cston · 2017-07-25T18:59:44Z

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

+        /// Otherwise, returns null.
+        /// </summary>
+        IMethodSymbol MethodSymbol { get; }
+    }


Is there an advantage to having this as an interface rather than a struct? A struct would be lightweight (a set of bits and an IMethodSymbol) and could be constructed from a Conversion easily.

I think that would be a good idea. While there is a nicety to being able to cast from the IConversion to the Conversion struct, that doesn't seem necessary given that one can use the language specific GetConversion methods. Since any casting from IConversion to Conversion would have to happen at a language specific level, it would also just be possible to call GetConversion at that point.

Alright, I can look at changing this to a struct. @cston or @CyrusNajmabadi, any ideas for what this should be named, since we already have Conversion structs for VB and C#?

The only thing with the struct approach is that we're going to have provide a similar GetConversion operation for the In/Out conversion scenario we discussed previously.

A possible option would be to have IConversionExpression and IConversionExpression<TConversion>. Clients could cast the operation they had to somethin glike IConversionExpression<Conversion> to then get a strongly typed, non-boxing version of .Conversion.

Same would exist for the in/out conversion issue.

I think, from a user perspective, the extension method version is better. I'd prefer not to have direct casts in end user code if we can help it. I don't really see a problem with having the extensions I mentioned, I was just pointing it out.

cston · 2017-07-26T23:30:19Z

src/Compilers/CSharp/Portable/Binder/Semantics/Conversions/Conversion.cs

@@ -904,6 +904,17 @@ public override int GetHashCode()
            return !(left == right);
        }

+        public static implicit operator CommonConversion(Conversion csharpConversion)


Is the implicit operator necessary? Consider making this an explicit method.

cston · 2017-07-26T23:34:05Z

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

+            IsReference = isReference;
+            IsUserDefined = isUserDefined;
+            MethodSymbol = methodSymbol;
+        }


bool fields could be combined into a single int.

cston · 2017-08-04T23:10:58Z

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

@@ -284,23 +284,23 @@ Namespace Microsoft.CodeAnalysis.Semantics
        End Function

        Private Function CreateBoundMeReferenceOperation(boundMeReference As BoundMeReference) As IInstanceReferenceExpression
-            Dim instanceReferenceKind As InstanceReferenceKind = If(boundMeReference.WasCompilerGenerated, InstanceReferenceKind.Implicit, InstanceReferenceKind.Explicit)
+            Dim instanceReferenceKind As InstanceReferenceKind = If(boundMeReference.WasCompilerGenerated, instanceReferenceKind.Implicit, instanceReferenceKind.Explicit)


Unnecessary change to InstanceReferenceKind here and below.

Hmm, formatter must changed that, I didn't explicitly change it.

cston · 2017-08-04T23:22:14Z

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

+    Friend Class VisualBasicOperationCloner
+        Inherits OperationCloner
+
+        Public Shared Property Instance As OperationCloner = New VisualBasicOperationCloner()


ReadOnly Property, or perhaps a ReadOnly field here and in C#.

Fixed. I went with ReadOnly Property.

cston · 2017-08-04T23:28:25Z

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

@@ -791,7 +791,7 @@ Namespace Microsoft.CodeAnalysis.Semantics
            Dim clauseValue = GetSingleValueCaseClauseValue(boundSimpleCaseClause)
            Dim value As Lazy(Of IOperation) = New Lazy(Of IOperation)(Function() Create(clauseValue))
            Dim equality As BinaryOperationKind = GetSingleValueCaseClauseEquality(clauseValue)
-            Dim caseKind As CaseKind = CaseKind.SingleValue
+            Dim caseKind As CaseKind = caseKind.SingleValue


Revert changes to CaseKind and LoopKind.

cston · 2017-08-04T23:33:09Z

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

@@ -194,6 +207,8 @@ private void LogType(ITypeSymbol type, string header = "Type")
            LogString($"{header}: {typeStr}");
        }

+        private string FormatBoolProperty(string propertyName, bool value) => $"{propertyName}: {(value ? "True" : "False")}";


…donly, make FormatBool static.

cston · 2017-08-04T23:42:35Z

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

@@ -97,6 +97,11 @@ internal IOperation GetOperationInternal(SyntaxNode node, CancellationToken canc
        protected abstract IOperation GetOperationCore(SyntaxNode node, CancellationToken cancellationToken);

        /// <summary>
+        /// Deep Clone given IOperation
+        /// </summary>
+        internal abstract T CloneOperation<T>(T operation) where T : IOperation;


Can we avoid a generic virtual method, and generic methods in several classes, by introducing an abstract CloneOperationCore method?

internal abstract IOperation CloneOperationCore(IOperation op); internal T CloneOperation<T>(T op) where T : IOperation => (T)CloneOperationCore(op);

cston · 2017-08-04T23:44:53Z

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

+        /// The existence of a conversion does not necessarily imply that the conversion is valid.
+        /// For example, an ambiguous user-defined conversion may exist but may not be valid.
+        /// </remarks>
+        public bool Exists => _conversionKind.HasFlag(ConversionKind.Exists);


Where is HasFlag defined?

Looks like it's from System.Enum.

cston · 2017-08-05T00:08:34Z

LGTM

…nversions.vb

333fred · 2017-08-07T18:02:27Z

Ping @dotnet/roslyn-compiler @dotnet/analyzer-ioperation for a second review from both please.

…sion-expression-rewrite * dotnet/features/ioperation: fix file path casing for analyzer packate Use 15.3 Preview 7 for VSI CI machines

…sion-expression-rewrite * dotnet/features/ioperation: (31 commits) Fix lambda EnC (dotnet#21291) Use 15.3 Preview 7 for VSI CI machines Add cancellation between each document. Remove unused private method (dotnet#21268) Work around SDK 1.1 issue around netcoreapp2.0 Update Language Feature Status.md Const warnings shouldn't cause emit error (dotnet#20729) Fix the symbol test Choose test config based on TargetFramework Standardize on the portable ThrowingTraceListener Enforce using UnitTestPortable Correct the order of parameters to Version Clean up RunCsc/Vbc.cmd a tiny bit Remove usages of 'Foo' from our codebase. (dotnet#21224) Make RunCsc/Vbc scripts respect DOTNET_HOST_PATH Fix typo ignoring exceptions about not running on main thread Updating VisualStudioSymbolSearchProgressService to consume new API Updating visual studio references Accept pre-rel version number ...

…sion-expression-rewrite * dotnet/features/ioperation: Tagging VB/C# Ioperation tests (dotnet#21310)

genlu · 2017-08-12T00:52:19Z

        ConversionKind conversionKind = GetConversionKind(boundConversion.ConversionKind);

Is ConversionKind still used anywhere? Can we remove it now?

Refers to: src/Compilers/CSharp/Portable/Operations/CSharpOperationFactory.cs:629 in f2d8739. [](commit_id = f2d8739, deletion_comment = False)

333fred · 2017-08-12T03:39:21Z

I don't believe so, we should be able to remove it.

333fred added 2 commits July 21, 2017 14:09

Adds initial IConverisonExpression API rewrite.

bc55977

Updated documentation, added ThrowsExceptionOnFailure field

f2c328d

333fred added Area-Analyzers PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Jul 21, 2017

333fred requested review from CyrusNajmabadi, cston and a team July 21, 2017 21:49

dnfclas added the cla-already-signed label Jul 21, 2017

cston reviewed Jul 25, 2017

View reviewed changes

CyrusNajmabadi reviewed Jul 25, 2017

View reviewed changes

cston reviewed Jul 25, 2017

View reviewed changes

CyrusNajmabadi reviewed Jul 25, 2017

View reviewed changes

cston reviewed Jul 25, 2017

View reviewed changes

CyrusNajmabadi reviewed Jul 25, 2017

View reviewed changes

cston reviewed Jul 25, 2017

View reviewed changes

333fred added 4 commits July 26, 2017 15:55

Moved fromm an interface IConversion to a CommonConversion struct.

0158878

Added IsChecked bool property

c850505

Use TryCast for cleaner code

9815e73

Remove period from error message

eb9fdb7

cston reviewed Jul 26, 2017

View reviewed changes

Removed LanguageName from ConversionExpression

c1b0ca3

cston reviewed Aug 4, 2017

View reviewed changes

Revert case changes in VBOperationFactory, make VBOperationCloner Rea…

aed4959

…donly, make FormatBool static.

cston reviewed Aug 4, 2017

View reviewed changes

333fred added 3 commits August 4, 2017 16:58

Move CloneOperation to not be generic calls all the way down.

9c0a391

Removed boxing HasFlag usage from CommonConversion.

65f5f24

Fixed IConversionExpression comment.

5d2ffcb

CyrusNajmabadi approved these changes Aug 6, 2017

View reviewed changes

Fixed failing unit tests, sorted and removed unnecessary usings in Co…

ee30c32

…nversions.vb

333fred added the Area-Compilers label Aug 6, 2017

333fred mentioned this pull request Aug 7, 2017

TestExecuteInInteractiveWithoutSelection Hang #21347

Open

333fred added 2 commits August 7, 2017 13:26

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

8a61410

…sion-expression-rewrite * dotnet/features/ioperation: fix file path casing for analyzer packate Use 15.3 Preview 7 for VSI CI machines

jinujoseph approved these changes Aug 8, 2017

View reviewed changes

333fred added 2 commits August 8, 2017 10:42

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

d4d8834

…sion-expression-rewrite * dotnet/features/ioperation: Tagging VB/C# Ioperation tests (dotnet#21310)

Tagged new unit tests with IOperation tag.

f2d8739

jaredpar approved these changes Aug 8, 2017

View reviewed changes

333fred merged commit 6693d8a into dotnet:features/ioperation Aug 8, 2017

333fred deleted the conversion-expression-rewrite branch August 8, 2017 20:29

333fred mentioned this pull request Aug 8, 2017

IConversionExpression Rework #20046

Closed

genlu mentioned this pull request Aug 11, 2017

Fix IArgument.InConversion/OutConversion #21445

Merged

333fred mentioned this pull request Aug 11, 2017

IConversionExpression must expose IsChecked boolean #18055

Closed

Conversion expression rewrite #21040

Conversion expression rewrite #21040

Conversation

333fred commented Jul 21, 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

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

333fred Jul 26, 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

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 25, 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

333fred Aug 4, 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

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 commented Aug 5, 2017

333fred commented Aug 7, 2017

genlu commented Aug 12, 2017

333fred commented Aug 12, 2017

333fred Jul 26, 2017 •

edited

Loading

333fred Aug 4, 2017 •

edited

Loading