Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IConversionExpression Rework #20046

Closed
333fred opened this issue Jun 6, 2017 · 7 comments
Closed

IConversionExpression Rework #20046

333fred opened this issue Jun 6, 2017 · 7 comments
Assignees
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Bug Feature - IOperation IOperation Urgency-Soon
Milestone

Comments

@333fred
Copy link
Member

333fred commented Jun 6, 2017

using System;

class Program
{
    static void Main(string[] args)
    {
        IFormattable f1 = $"{1}";
    }
}
IOperation tree for "f1 = $"{1}""

IVariableDeclarationStatement (1 declarations) (OperationKind.VariableDeclarationStatement) (Syntax: 'IFormattabl ... 1 = $"{1}";')
  IVariableDeclaration (1 variables) (OperationKind.VariableDeclaration) (Syntax: 'IFormattabl ... 1 = $"{1}";')
    Variables: Local_1: System.IFormattable f1
    Initializer: IConversionExpression (ConversionKind.Invalid, Implicit) (OperationKind.ConversionExpression, Type: System.IFormattable) (Syntax: '$"{1}"')
        IInterpolatedStringExpression (OperationKind.InterpolatedStringExpression, Type: System.String) (Syntax: '$"{1}"')
          Parts(1): IInterpolation (OperationKind.Interpolation) (Syntax: '{1}')
              Expression: ILiteralExpression (Text: 1) (OperationKind.LiteralExpression, Type: System.Int32, Constant: 1) (Syntax: '1')

Note the ConversionKind.Invalid. This is because the underlying BoundConversion has ConversionKind.InterpolatedString, which is not in our public API. This conversion is part of the C# spec, and should be reflected as a conversion type in the IOperation tree. FYI @mavasani @dotnet/analyzer-ioperation.

@333fred 333fred added this to the 15.6 milestone Jun 6, 2017
@333fred 333fred changed the title Add InterplotedString Conversion to public ConversionKind Add InterpolatedString Conversion to public ConversionKind Jun 6, 2017
@333fred
Copy link
Member Author

333fred commented Jun 8, 2017

Note: this should probably be represented as ConversionKind.CSharp, unless we think that there is a plan to add interpolated strings with similar semantics to VB.

@333fred 333fred self-assigned this Jun 23, 2017
@333fred
Copy link
Member Author

333fred commented Jun 23, 2017

Design Team Decision:
We should do this.

@333fred
Copy link
Member Author

333fred commented Jun 26, 2017

@AlekseyTs had some comments on this in #20416 (comment).

@jinujoseph jinujoseph added 4 - In Review A fix for the issue is submitted for review. Urgency-Soon labels Jun 26, 2017
@333fred 333fred changed the title Add InterpolatedString Conversion to public ConversionKind IConversionExpression Rework Jul 19, 2017
@333fred
Copy link
Member Author

333fred commented Jul 19, 2017

Design Team Decision:
May be as a headstart , we should say for Iconversionexpression you can get conversion struct so you can do what ever you want , we can go and experiment to bring parity in both.

@CyrusNajmabadi
Copy link
Member

I think we should also start pulling together a common type that has values we know are always safe to get back to. This could be an object that also allows one to get back to the language specific structs (through extenesion methods at the individual language layers) for features that need exact, rich, language specific data. i.e. we could have:

public abstract class CommonConversion {
   // common stuff
}

// C# layer
internal class CSharpConversion : CommonConversion {
     public readonly Conversion UnderlyingConversion;
}

...

public static Conversion GetCSharpConversion(this CommonConversion conversion)
     => ((CSharpConversion)conversion).UnderlyingConversion;

@333fred
Copy link
Member Author

333fred commented Jul 19, 2017

@CyrusNajmabadi and I have talked about this offline, and here's what I'm proposing for V1:

  • IConversionExpression will become a wrapper around the VB and C# Conversion structs and the target of the conversion.
  • We will create an interface IConversion for the C# and VB Conversion structs. For right now, this will only have information that is directly equivalent between the languages, with no translation necessar. There are a few things that Cyrus and I think we should be able to add to this interface without too much controversy, such as Exists, IsIdentity, IsNumeric, IsUserDefined, MethodSymbol, and possibly IsNullable. More advanced checks can come from a later proposal.

New IConversionExpression:

    /// <summary>
    /// Represents a conversion operation.
    /// </summary>
    /// <remarks>
    /// This interface is reserved for implementation by its associated APIs. We reserve the right to
    /// change it in the future.
    /// </remarks>
    public interface IConversionExpression
    {
        /// <summary>
        /// Value to be converted.
        /// </summary>
        IOperation Operand { get; }
        /// <summary>
        /// Gets the underlying conversion. This will be either <see cref="Microsoft.CodeAnalysis.CSharp.Conversion"/>
        /// or <see cref="Microsoft.CodeAnalysis.VisualBasic.Conversion"/>.
        /// </summary>
        /// <typeparam name="TConversion">The specific conversion kind for the language of the code.</typeparam>
        /// <returns>The conversion struct. If the wrong conversion type is specified, a <see cref="ArgumentException"/> will be thrown.</returns>
        TConversion GetConversion<TConversion>() where TConversion : struct, IConversion;
        /// <summary>
        /// True if and only if the conversion is indicated explicity by a cast operation in the source code.
        /// </summary>
        bool IsExplicitInCode { get; }
        /// <summary>
        /// The language that defined this conversion. Possible values are <see cref="LanguageNames.CSharp"/> and
        /// <see cref="LanguageNames.VisualBasic"/>.
        /// </summary>
        string LanguageName { get; }
    }

New IConversion:

/// <summary>
/// Represents a conversion that is convertible to a language-specific conversion. This can be either
/// a <see cref="Microsoft.CodeAnalysis.CSharp.Conversion"/> or 
/// <see cref="Microsoft.CodeAnalysis.VisualBasic.Conversion"/>.
/// <remarks>
/// This interface is reserved for implementation by its associated APIs. We reserve the right to
/// change it in the future.
/// </remarks>
public interface IConversion
{
    /// <summary>
    /// Returns true if the conversion exists, as defined by the target language.
    /// </summary>
    /// <remarks>
    /// 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 { get; }
    /// <summary>
    /// Returns true if the conversion is an identity conversion.
    /// </summary>
    public bool IsIdentity { get; }
    /// <summary>
    /// Returns true if the conversion is a numeric conversion.
    /// </summary>
    public bool IsNumeric { get; } 
    /// <summary>
    /// Returns true if the conversion is a user-defined conversion.
    /// </summary>
    public bool IsUserDefined { get; }
    /// <summary>
    /// Returns the method used to perform the conversion for a user-defined conversion if <see cref="IsUserDefined"/> is true.
    /// Otherwise, returns null.
    /// </summary>
    public IMethodSymbol MethodSymbol { get; }
}

@333fred
Copy link
Member Author

333fred commented Aug 8, 2017

Fixed in #21040.

@333fred 333fred closed this as completed Aug 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - In Review A fix for the issue is submitted for review. Area-Analyzers Bug Feature - IOperation IOperation Urgency-Soon
Projects
None yet
Development

No branches or pull requests

4 participants