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

[API proposal] Collection expressions: IOperation support #70631

Closed
cston opened this issue Oct 30, 2023 · 5 comments
Closed

[API proposal] Collection expressions: IOperation support #70631

cston opened this issue Oct 30, 2023 · 5 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request New Feature - Collection Expressions
Milestone

Comments

@cston
Copy link
Member

cston commented Oct 30, 2023

Background and Motivation

Provide IOperation support for collection expressions.

Proposed API

namespace Microsoft.CodeAnalysis.Operations
{
    /// <summary>
    /// Represents a collection expression.
    /// <para>
    ///   Current usage:
    ///   (1) C# collection expression.
    /// </para>
    /// </summary>
    public interface ICollectionExpressionOperation : IOperation
    {
        /// <summary>
        /// Method used to construct the collection.
        /// <para>
        ///   If the collection type is an array, span, array interface, or type parameter, the method is null;
        ///   if the collection type has a [CollectionBuilder] attribute, the method is the builder method;
        ///   otherwise, the method is the collection type constructor.
        /// </para>
        /// </summary>
        IMethodSymbol? ConstructMethod { get; }

        /// <summary>
        /// Collection expression elements.
        /// <para>
        ///   If the element is an expression, the entry is converted to the target element type;
        ///   otherwise, the entry is an ISpreadOperation.
        /// </para>
        /// </summary>
        ImmutableArray<IOperation> Elements { get; }
    }

    /// <summary>
    /// Represents a collection expression spread element.
    /// <para>
    ///   Current usage:
    ///   (1) C# spread element.
    /// </para>
    /// </summary>
    public interface ISpreadOperation : IOperation
    {
        /// <summary>
        /// Collection being spread.
        /// </summary>
        IOperation Operand { get; }

        /// <summary>
        /// Type of the collection iterator item.
        /// </summary>
        ITypeSymbol ItemType { get; }

        /// <summary>
        /// Conversion from the type of the iterator item to the target element type
        /// of the containing collection expression.
        /// </summary>
        CommonConversion ItemConversion { get; }
    }
}

namespace Microsoft.CodeAnalysis.CSharp
{
    public readonly struct Conversion
    {
        // ...

        public bool IsCollectionExpression { get; }
    }

    public static class CSharpExtensions
    {
        // ...

        /// <summary>
        /// Gets the underlying item <see cref="Conversion"/> information from this <see cref="ISpreadOperation"/>.
        /// </summary>
        public static Conversion GetSpreadItemConversion(this ISpreadOperation spread);
    }
}

Usage Examples

Span:

// Span<object> x = [1];

ICollectionExpressionOperation (1 elements, ConstructMethod: null) (OperationKind.CollectionExpression, Type: System.Span<System.Object>) (Syntax: '[1]')
    Elements(1):
        IConversionOperation (TryCast: False, Unchecked) (OperationKind.Conversion, Type: System.Object, IsImplicit) (Syntax: '1')
        Conversion: CommonConversion (Exists: True, IsIdentity: False, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
        Operand:
            ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1')

Collection initializer type:

// List<object> x = [1];

ICollectionExpressionOperation (1 elements, ConstructMethod: System.Collections.Generic.List<System.Object>..ctor()) (OperationKind.CollectionExpression, Type: System.Collections.Generic.List<System.Object>) (Syntax: '[1]')
  Elements(1):
      IConversionOperation (TryCast: False, Unchecked) (OperationKind.Conversion, Type: System.Object, IsImplicit) (Syntax: '1')
        Conversion: CommonConversion (Exists: True, IsIdentity: False, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
        Operand:
          ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1')

Builder method:

// MyCollection<object> x = [1];

// [CollectionBuilder(typeof(MyCollectionBuilder), "Create")]
// struct MyCollection<T> : IEnumerable<T>  { ... }

// class MyCollectionBuilder
// {
//     public static MyCollection<T> Create<T>(ReadOnlySpan<T> items) { ... }
// }

ICollectionExpressionOperation (1 elements, ConstructMethod: MyCollection<System.Object> MyCollectionBuilder.Create<System.Object>(System.ReadOnlySpan<System.Object> items)) (OperationKind.CollectionExpression, Type: MyCollection<System.Object>) (Syntax: '[1]')
  Elements(1):
      IConversionOperation (TryCast: False, Unchecked) (OperationKind.Conversion, Type: System.Object, IsImplicit) (Syntax: '1')
        Conversion: CommonConversion (Exists: True, IsIdentity: False, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
        Operand:
          ILiteralOperation (OperationKind.Literal, Type: System.Int32, Constant: 1) (Syntax: '1')

Spread:

// int[] x = ...;
// object[] y = [..x];

ICollectionExpressionOperation (1 elements, ConstructMethod: null) (OperationKind.CollectionExpression, Type: System.Object[]) (Syntax: '[..x]')
  Elements(1):
      ISpreadOperation (ItemType: System.Int32) (OperationKind.Spread, Type: null, IsImplicit) (Syntax: '..x')
        Operand:
          ILocalReferenceOperation: x (OperationKind.LocalReference, Type: System.Int32[]) (Syntax: 'x')
        ItemConversion: CommonConversion (Exists: True, IsIdentity: False, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
          (Boxing)

Alternative Designs

Risks

Relates to test plan: #66418

@cston cston added Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request labels Oct 30, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 30, 2023
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@333fred
Copy link
Member

333fred commented Nov 9, 2023

API Review

  • What level do we want these to exist on? How much abstraction?
    • Do we even want to show the Add calls in this API?
    • It's in a weird middle ground: Not quite the specification representation, not quite the lowered form.
      • Span is a good example: we may not wrap an array, and the spec doesn't say we wrap an array.
    • Do we even want to have the Add calls?
      • We do for regular collection initializers
  • Should ISpreadOperation's element be named Element?
    • Or Operand
  • For the CFG, how far do we lower?
    • await doesn't lower at all
    • foreach lowers to gotos, collection initializers lower to the series of add calls
    • Perhaps this should just resurface the collection expression back?
      • We have looser specifications on when enumerations occur, for example, and it's a bit of a disservice if the CFG conveys an ordering

Conclusion: We'll simplify the proposal to remove these implementation details and bring it back.

@333fred 333fred added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 9, 2023
@cston
Copy link
Member Author

cston commented Nov 13, 2023

Updated proposal based on API review feedback.

@cston cston added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-needs-work API needs work before it is approved, it is NOT ready for implementation labels Nov 13, 2023
@333fred
Copy link
Member

333fred commented Nov 17, 2023

API Review

  • Is there always a conversion, even if it's Identity?
    • If it is true, perhaps we should doc it? Not sure if it is true.
    • We should probably remove the conversion phrasing, we don't doc it anywhere else and as it written, it could imply that there always is.
      • Or add "if necessary"?
  • Naming
    • GetItemConversion? Spread isn't really needed in the name.
    • Is Item the right name?
      • Use Element instead

Conclusion: Approved. Remove Spread from GetSpreadItemConversion, and change Item->Element across the board for consistency with ForEachStatementInfo.

@333fred 333fred added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 17, 2023
@cston
Copy link
Member Author

cston commented Nov 30, 2023

Implemented in #70650.

@cston cston closed this as completed Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented Concept-API This issue involves adding, removing, clarification, or modification of an API. Feature Request New Feature - Collection Expressions
Projects
None yet
Development

No branches or pull requests

3 participants