IOperation support for interpolated string handler conversions

333fred commented 3 years ago

Background and Motivation

C# 10 adds interpolated string handler conversions. These conversions have a number of new components that don't map to existing interpolated string nodes, so we need to add public API support for them.

Proposed API

namespace Microsoft.CodeAnalysis.Operations
{
    public enum OperationKind
    {
+       /// <summary>Indicates an <see cref="IInterpolatedStringHandlerCreationOperation"/>.</summary>
+       InterpolatedStringHandler = 0x72,
+       /// <summary>Indicates an <see cref="IInterpolatedStringAdditionOperation"/>.</summary>
+       InterpolatedStringAddition = 0x73,
+       /// <summary>Indicates an <see cref="IInterpolatedStringAppendOperation"/>. </summary>
+       InterpolatedStringAppendLiteral = 0x74,
+       /// <summary>Indicates an <see cref="IInterpolatedStringAppendOperation"/>. This append is of an interpolation component</summary>
+       InterpolatedStringAppendFormatted = 0x75,
+       /// <summary>Indicates an <see cref="IInterpolatedStringHandlerArgumentPlaceholderOperation"/>.</summary>
+       InterpolatedStringHandlerArgumentPlaceholder = 0x76,
    }

+   /// <summary>
+   /// Represents an interpolated string converted to a custom interpolated string handler type.
+   /// </summary>
+   public interface IInterpolatedStringHandlerCreationOperation : IOperation
+   {
+       /// <summary>
+       /// The construction of the interpolated string handler instance. This can be an <see cref="IObjectCreationOperation" /> for valid code, and
+       /// <see cref="IDynamicObjectCreationOperation" /> or <see cref="IInvalidOperation" /> for invalid code.
+       /// </summary>
+       IOperation HandlerCreation { get; }
+       /// <summary>
+       /// True if the last parameter of <see cref="HandlerCreation" /> is an out <see langword="bool" /> parameter that will be checked before executing the code in
+       /// <see cref="Content" />. False otherwise.
+       /// </summary>
+       bool HandlerCreationHasSuccessParameter { get; }
+       /// <summary>
+       /// True if the AppendLiteral or AppendFormatted calls in <see cref="Parts" /> return <see langword="bool" />. When that is true, each part will be conditional
+       /// on the return of the part before it, only being executed when the Append call returns true. False otherwise.
+       /// </summary>
+       /// <remarks>
+       /// when this is true and <see cref="HandlerCreationHasSuccessParameter" /> is true, then the first part in <see cref="Parts" /> is conditionally run. If this is
+       /// true and <see cref="HandlerCreationHasSuccessParameter" /> is false, then the first part is unconditionally run.
+       /// <br />
+       /// Just because this is true or false does not guarantee that all Append calls actually do return boolean values, as there could be dynamic calls or errors.
+       /// It only governs what the compiler was expecting, based on the first calls it did see.
+       /// </remarks>
+       bool HandlerAppendCallsReturnBool { get; }
+       /// <summary>
+       /// The interpolated string expression or addition operation that makes up the content of this string. This is either an <see cref="IInterpolatedStringOperation" />
+       /// or an <see cref="IInterpolatedStringAdditionOperation" /> operation.
+       /// </summary>
+       IOperation Content { get; }
+   }
+   /// <summary>
+   /// Represents an addition of multiple interpolated string literals being converted to an interpolated string handler type.
+   /// </summary>
+   public interface IInterpolatedStringAdditionOperation : IOperation
+   {
+       /// <summary>
+       /// The interpolated string expression or addition operation on the left side of the operator. This is either an <see cref="IInterpolatedStringOperation" />
+       /// or an <see cref="IInterpolatedStringAdditionOperation" /> operation.
+       /// </summary>
+       IOperation Left { get; }
+       /// <summary>
+       /// The interpolated string expression or addition operation on the right side of the operator. This is either an <see cref="IInterpolatedStringOperation" />
+       /// or an <see cref="IInterpolatedStringAdditionOperation" /> operation.
+       /// </summary>
+       IOperation Right { get; }
+   }
+   /// <summary>
+   /// Represents a call to either AppendLiteral or AppendFormatted as part of an interpolated string handler conversion.
+   /// </summary>
+   public interface IInterpolatedStringAppendOperation : IInterpolatedStringContentOperation
+   {
+       /// <summary>
+       /// If this interpolated string is subject to an interpolated string handler conversion, the construction of the interpolated string handler instance.
+       /// This can be an IInvocationOperation or IDynamicInvocationOperation for valid code, and IInvalidOperation for invalid code.
+       /// </summary>
+       IOperation? AppendCall { get; }
+   }
+   /// <summary>
+   /// Represents an argument from the method call, indexer access, or constructor invocation that is creating the containing <see cref="IInterpolatedStringHandlerCreationOperation" />
+   /// </summary>
+   public interface IInterpolatedStringHandlerArgumentPlaceholderOperation : IOperation
+   {
+       /// <summary>
+       /// The index of the argument of the method call containing the interpolated string handler conversion this placeholder is referencing. -1 if
+       /// <see cref="IsContainingMethodReceiver" /> or if the argument was not resolved.
+       /// </summary>
+       int ArgumentIndex { get; }
+       /// <summary>
+       /// True if this placeholder represents the instance receiver of the method call containing the interpolated string handler conversion.
+       /// </summary>
+       /// <remarks>
+       /// Extension method receivers are not treated as receivers here: they have an <see cref="ArgumentIndex" /> of 0.
+       /// </remarks>
+       bool IsContainingMethodReceiver { get; }
+   }

    public enum InstanceReferenceKind
    {
+       /// <summary>
+       /// Reference to the interpolated string handler instance created as part of a parent interpolated string handler conversion.
+       /// </summary>
+       InterpolatedStringHandler
    }
}

Alternative Designs

We could potentially introduce an IPlaceholderOperation instead of IInterpolatedStringHandlerArgumentPlaceholder, with an additional PlaceholderKind enum to differentiate the variants. However, that a) doesn't follow existing patterns, such as IInstanceReferenceOperation, and b) I don't believe that it's actually useful to operate on all placeholders regardless of what type of placeholder it is.

I debated for a while on whether we should have HandlerAppendCallsReturnBool and HandlerCreationHasSuccessParameter. The latter can be figured out by examining the call by hand, and the former could just be a single boolean that's true if there's any conditional evaluation of the holes. However, after implementing the BoundTree I found HandlerCreationHasSuccessParameter to be very valuable, and the single bool version is lossy. For full fidelity of the runtime semantics here, we do need both.

We could potentially add a flag to IInterpolatedStringAppendOperation to say whether it's AppendFormatted or AppendLiteral, but I think that's fine to grab the call and examine the type symbol for.

Examples of IOperation Trees

CustomHandler c = /*<bind>*/$"Hello world"/*</bind>*/;

TypeInfo
    ConvertedType: CustomHandler
    Type: null

IInterpolatedStringHandlerOperation (HandlerCreationHasSuccessParameter: true, HandlerAppendCallsReturnBool: true) (IsImplicit, Type: CustomHandler)
    BuilderCreation - IOperation
        IObjectCreationOperation
    Content
        IInterpolatedStringOperation (Explicit, Type: null)
            Parts
                IInterpolatedStringAppendOperation
                    IsLiteral True
                    AppendCall
                        IInvocationOperation
                            Receiver
                                IInstanceReferenceOperation (InstanceReferenceKind.InterpolatedStringHandler)
                            Arguments
                                ILiteralOperation "Hello world"

CustomHandler c = /*<bind>*/$"Hello" + $" " + $"world"/*</bind>*/;

TypeInfo
    ConvertedType: CustomHandler
    Type: null

IInterpolatedStringHandlerCreationOperation (HandlerCreationHasSuccessParameter: true, HandlerAppendCallsReturnBool: true) (IsImplicit, Type: CustomHandler)
    BuilderCreation:
        IObjectCreationOperation
    Content:
        IInterpolatedStringAdditionOperation (Type: null)
            Left
                InterpolatedStringAdditionOperation (Type: null)
                    Left:
                        IInterpolatedStringOperation (Type: null)
                            Parts
                                IInterpolatedStringAppendOperation
                                    IsLiteral True
                                    AppendCall
                                        IInvocationOperation
                                            Receiver
                                                IInstanceReferenceOperation (InstanceReferenceKind.InterpolatedStringHandler)
                                            Arguments
                                                ILiteralOperation "Hello"
                    Right:
                        IInterpolatedStringOperation (Type: null)
                            Parts
                                IInterpolatedStringAppendOperation
                                    IsLiteral True
                                    AppendCall
                                        IInvocationOperation
                                            Receiver
                                                IInstanceReferenceOperation (InstanceReferenceKind.InterpolatedStringHandler)
                                            Arguments
                                                ILiteralOperation " "
            Right
                IInterpolatedStringOperation (Type: null)
                    Parts
                        IInterpolatedStringAppendOperation
                            IsLiteral True
                            AppendCall
                                IInvocationOperation
                                    Receiver
                                        IInstanceReferenceOperation (InstanceReferenceKind.InterpolatedStringHandler)
                                    Arguments
                                        ILiteralOperation "world"

Followup Questions

In our original design, we did not approve a way to distinguish between placeholders for the receiver of the containing method, and the optional trailing out parameter placeholder.

namespace Microsoft.CodeAnalysis.Operations
{
+    public enum InterpolatedStringArgumentPlaceholderKind
+    {
+        Argument,
+        ContainingMethodReceiver,
+        TrailingValidityParameter,
+    }
    public interface IInterpolatedStringHandlerArgumentPlaceholderOperation : IOperation
    {
        /// <summary>
        /// The index of the argument of the method call containing the interpolated string handler conversion this placeholder is referencing. -1 if <see cref="PlaceholderKind" /> is
        /// anything other than <see cref="InterpolatedStringArgumentPlaceholderKind.Argument" />.
        /// </summary>
        int ArgumentIndex { get; }

+        InterpolatedStringArgumentPlaceholderKind PlaceholderKind { get; }
    }
}

333fred commented 3 years ago

API Review

We discussed this in depth today, and some members are worried that we're over-specifying the API here. We'll dig into this again in a future API review session with more concrete tree examples.

333fred commented 3 years ago

I've added some examples of the trees to the original comment. Note that some of the properties will be a bit off (such as syntax) as I've bodged together a bunch of different examples, but it should at least be the correct general shape.

333fred commented 3 years ago

API Review

We've generally agreed that the interface should expose the Append calls as children directly, rather than trying to add the information to IInterpolatedTextOperation or IInterpolatedContentOperation. We don't want to expose the data on IBinaryOperation directly: we feel it's surprising and a violation of layering, and that the IInterpolatedStringOperation should be the top node. We have a few options we're considering:

We collapse all the parts into a single IInterpolatedStringOperation. While this is the most faithful to the semantics of the language, we're concerned about the ergonomics of not having IOperation nodes that correspond to nested BinaryExpressionSyntaxes or InterpolatedStringExpressionSyntaxes, which this approach would necessitate.
We have dedicated IBinaryContentOperation or similar nodes that represent the binary operations themselves, and are part of the Parts array on the root IInterpolatedStringOperation node. While this is more complex and more of a "syntactic" view, it will alleviate the concerns about missing explicit syntax nodes.

A smaller team will explore this space and update the design.

One other note is that we should have a flag on IInterpolatedStringAppendOperation that indicates whether the content is a literal or an interpolation placeholder.

333fred commented 3 years ago

API Review

We'll use IInvalidOperation for bad arguments in the handler creation, rather than IInterpolatedStringHandlerArgumentPlaceholderOperation. For the receiver node, IInterpolatedStringHandlerArgumentPlaceholderOperation.ArgumentIndex will be -1, and IsContainingMethodReceiver will be removed. Otherwise, the API looks good as currently defined.

333fred commented 3 years ago

API Review

The update is generally approved, with the following names for the enum:

    public enum InterpolatedStringArgumentPlaceholderKind
    {
        CallsiteArgument,
        CallsiteReceiver,
        TrailingValidityArgument,
    }

dotnet / roslyn